Commit graph

2932 commits

Author SHA1 Message Date
Matt Caswell
28ff8ef3f7 Convert CertificateRequest construction to WPACKET
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-09-29 14:52:55 +01:00
Matt Caswell
25849a8f8b Address style feedback comments
Merge declarations of same type together.

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-09-29 10:06:46 +01:00
Matt Caswell
7facdbd66f Fix a bug in the construction of the ClienHello SRTP extension
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-09-29 10:06:46 +01:00
Matt Caswell
7507e73d40 Fix heartbeat compilation error
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-09-29 10:06:46 +01:00
Matt Caswell
150e298551 Delete some unneeded code
Some functions were being called from both code that used WPACKETs and code
that did not. Now that more code has been converted to use WPACKETs some of
that duplication can be removed.

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-09-29 10:06:46 +01:00
Matt Caswell
8157d44b62 Convert ServerHello construction to WPACKET
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-09-29 10:06:46 +01:00
Matt Caswell
2f2d6e3e3c Fix an Uninit read in DTLS
If we have a handshake fragment waiting then dtls1_read_bytes() was not
correctly setting the value of recvd_type, leading to an uninit read.

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-09-29 09:58:14 +01:00
Matt Caswell
2f97192c78 Fix a bug in Renegotiation extension construction
The conversion to WPACKET broke the construction of the renegotiation
extension.

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-09-28 09:15:07 +01:00
Matt Caswell
0086ca4e9b Convert HelloRequest construction to WPACKET
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-09-28 09:15:07 +01:00
Matt Caswell
98c1f5b429 Fix HelloVerifyRequest construction
commit c536b6be1a introduced a bug that causes a reachable assert. This fixes
it.

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-09-26 14:40:33 +01:00
Matt Caswell
0d698f6696 Fix Use After Free for large message sizes
The buffer to receive messages is initialised to 16k. If a message is
received that is larger than that then the buffer is "realloc'd". This can
cause the location of the underlying buffer to change. Anything that is
referring to the old location will be referring to free'd data. In the
recent commit c1ef7c97 (master) and 4b390b6c (1.1.0) the point in the code
where the message buffer is grown was changed. However s->init_msg was not
updated to point at the new location.

CVE-2016-6309

Reviewed-by: Emilia Käsper <emilia@openssl.org>
2016-09-26 08:52:48 +01:00
Matt Caswell
f789b04f40 Fix a WPACKET bug
If we request more bytes to be allocated than double what we have already
written, then we grow the buffer by the wrong amount.

Reviewed-by: Emilia Käsper <emilia@openssl.org>
2016-09-26 08:52:48 +01:00
Matt Caswell
c536b6be1a Convert HelloVerifyRequest construction to WPACKET
We actually construct a HelloVerifyRequest in two places with common code
pulled into a single function. This one commit handles both places.

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-09-22 23:12:38 +01:00
Matt Caswell
4b0fc9fc7a Add warning about a potential pitfall with WPACKET_allocate_bytes()
If the underlying BUF_MEM gets realloc'd then the pointer returned could
become invalid. Therefore we should always ensure that the allocated
memory is filled in prior to any more WPACKET_* calls.

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-09-22 23:12:38 +01:00
Rich Salz
f3b3d7f003 Add -Wswitch-enum
Change code so when switching on an enumeration, have case's for all
enumeration values.

Reviewed-by: Andy Polyakov <appro@openssl.org>
2016-09-22 08:36:26 -04:00
Dmitry Belyavsky
41b4280772 Avoid KCI attack for GOST
Russian GOST ciphersuites are vulnerable to the KCI attack because they use
long-term keys to establish the connection when ssl client authorization is
on. This change brings the GOST implementation into line with the latest
specs in order to avoid the attack. It should not break backwards
compatibility.

Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
2016-09-22 09:27:45 +01:00
Matt Caswell
b8d2439562 Fix a hang with SSL_peek()
If while calling SSL_peek() we read an empty record then we go into an
infinite loop, continually trying to read data from the empty record and
never making any progress. This could be exploited by a malicious peer in
a Denial Of Service attack.

CVE-2016-6305

GitHub Issue #1563

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-09-22 09:27:45 +01:00
Matt Caswell
c31dbed70c Fix a mem leak in NPN handling
If a server sent multiple NPN extensions in a single ClientHello then a
mem leak can occur. This will only happen where the client has requested
NPN in the first place. It does not occur during renegotiation. Therefore
the maximum that could be leaked in a single connection with a malicious
server is 64k (the maximum size of the ServerHello extensions section). As
this is client side, only occurs if NPN has been requested and does not
occur during renegotiation this is unlikely to be exploitable.

Issue reported by Shi Lei.

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-09-22 09:27:45 +01:00
Matt Caswell
e408c09bbf Fix OCSP Status Request extension unbounded memory growth
A malicious client can send an excessively large OCSP Status Request
extension. If that client continually requests renegotiation,
sending a large OCSP Status Request extension each time, then there will
be unbounded memory growth on the server. This will eventually lead to a
Denial Of Service attack through memory exhaustion. Servers with a
default configuration are vulnerable even if they do not support OCSP.
Builds using the "no-ocsp" build time option are not affected.

I have also checked other extensions to see if they suffer from a similar
problem but I could not find any other issues.

CVE-2016-6304

Issue reported by Shi Lei.

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-09-22 09:27:45 +01:00
Richard Levitte
a449b47c7d Fix error message typo, wrong function code
Reviewed-by: Matt Caswell <matt@openssl.org>
2016-09-22 09:24:49 +01:00
Matt Caswell
48c054fec3 Excessive allocation of memory in dtls1_preprocess_fragment()
This issue is very similar to CVE-2016-6307 described in the previous
commit. The underlying defect is different but the security analysis and
impacts are the same except that it impacts DTLS.

A DTLS message includes 3 bytes for its length in the header for the
message.
This would allow for messages up to 16Mb in length. Messages of this length
are excessive and OpenSSL includes a check to ensure that a peer is sending
reasonably sized messages in order to avoid too much memory being consumed
to service a connection. A flaw in the logic of version 1.1.0 means that
memory for the message is allocated too early, prior to the excessive
message length check. Due to way memory is allocated in OpenSSL this could
mean an attacker could force up to 21Mb to be allocated to service a
connection. This could lead to a Denial of Service through memory
exhaustion. However, the excessive message length check still takes place,
and this would cause the connection to immediately fail. Assuming that the
application calls SSL_free() on the failed conneciton in a timely manner
then the 21Mb of allocated memory will then be immediately freed again.
Therefore the excessive memory allocation will be transitory in nature.
This then means that there is only a security impact if:

1) The application does not call SSL_free() in a timely manner in the
event that the connection fails
or
2) The application is working in a constrained environment where there
is very little free memory
or
3) The attacker initiates multiple connection attempts such that there
are multiple connections in a state where memory has been allocated for
the connection; SSL_free() has not yet been called; and there is
insufficient memory to service the multiple requests.

Except in the instance of (1) above any Denial Of Service is likely to
be transitory because as soon as the connection fails the memory is
subsequently freed again in the SSL_free() call. However there is an
increased risk during this period of application crashes due to the lack
of memory - which would then mean a more serious Denial of Service.

This issue does not affect TLS users.

Issue was reported by Shi Lei (Gear Team, Qihoo 360 Inc.).

CVE-2016-6308

Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-09-21 20:37:53 +01:00
Matt Caswell
c1ef7c971d Excessive allocation of memory in tls_get_message_header()
A TLS message includes 3 bytes for its length in the header for the message.
This would allow for messages up to 16Mb in length. Messages of this length
are excessive and OpenSSL includes a check to ensure that a peer is sending
reasonably sized messages in order to avoid too much memory being consumed
to service a connection. A flaw in the logic of version 1.1.0 means that
memory for the message is allocated too early, prior to the excessive
message length check. Due to way memory is allocated in OpenSSL this could
mean an attacker could force up to 21Mb to be allocated to service a
connection. This could lead to a Denial of Service through memory
exhaustion. However, the excessive message length check still takes place,
and this would cause the connection to immediately fail. Assuming that the
application calls SSL_free() on the failed conneciton in a timely manner
then the 21Mb of allocated memory will then be immediately freed again.
Therefore the excessive memory allocation will be transitory in nature.
This then means that there is only a security impact if:

1) The application does not call SSL_free() in a timely manner in the
event that the connection fails
or
2) The application is working in a constrained environment where there
is very little free memory
or
3) The attacker initiates multiple connection attempts such that there
are multiple connections in a state where memory has been allocated for
the connection; SSL_free() has not yet been called; and there is
insufficient memory to service the multiple requests.

Except in the instance of (1) above any Denial Of Service is likely to
be transitory because as soon as the connection fails the memory is
subsequently freed again in the SSL_free() call. However there is an
increased risk during this period of application crashes due to the lack
of memory - which would then mean a more serious Denial of Service.

This issue does not affect DTLS users.

Issue was reported by Shi Lei (Gear Team, Qihoo 360 Inc.).

CVE-2016-6307

Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-09-21 20:37:53 +01:00
Matt Caswell
af58be768e Don't allow too many consecutive warning alerts
Certain warning alerts are ignored if they are received. This can mean that
no progress will be made if one peer continually sends those warning alerts.
Implement a count so that we abort the connection if we receive too many.

Issue reported by Shi Lei.

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-09-21 20:17:04 +01:00
Rich Salz
4588cb4443 Revert "Constify code about X509_VERIFY_PARAM"
This reverts commit 81f9ce1e19.

Reviewed-by: Matt Caswell <matt@openssl.org>
2016-09-21 10:37:03 -04:00
Matt Caswell
3c10632529 make update and fix some associated mis-matched error codes
Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-09-21 14:31:30 +01:00
Richard Levitte
5a008ff6c5 Quiet compiler warning about uninitialised variable
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-09-20 21:59:48 +02:00
Matt Caswell
08029dfa03 Convert WPACKET_put_bytes to use convenience macros
All the other functions that take an argument for the number of bytes
use convenience macros for this purpose. We should do the same with
WPACKET_put_bytes().

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-09-20 14:47:44 +01:00
Matt Caswell
85a7a5e6ef Convert CCS construction to WPACKET
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-09-20 14:17:50 +01:00
Matt Caswell
4f89bfbf15 Convert Finished construction to WPACKET
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-09-20 12:26:38 +01:00
Matt Caswell
418a18a2de Style tweaks following review feedback
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-09-20 10:16:56 +01:00
Matt Caswell
15e6be6c5c Convert NextProto message construction to WPACKET
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-09-20 10:16:56 +01:00
Matt Caswell
c49e191230 Convert Certificate message construction to WPACKET
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-09-20 10:16:56 +01:00
Matt Caswell
6400f33818 Convert ClientVerify Construction to WPACKET
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-09-20 10:16:56 +01:00
FdaSilvaYY
81f9ce1e19 Constify code about X509_VERIFY_PARAM
Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/1594)
2016-09-18 00:22:00 -04:00
Alessandro Ghedini
4f8a5f4da9 Use switch instead of multiple ifs
Makes the logic a little bit clearer.

Reviewed-by: Andy Polyakov <appro@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/1571)
2016-09-16 11:07:08 -04:00
Matt Caswell
3c0c68ae46 Revert "Abort on unrecognised warning alerts"
This reverts commit 77a6be4dfc.

There were some unexpected side effects to this commit, e.g. in SSLv3 a
warning alert gets sent "no_certificate" if a client does not send a
Certificate during Client Auth. With the above commit this causes the
connection to abort, which is incorrect. There may be some other edge cases
like this so we need to have a rethink on this.

Reviewed-by: Tim Hudson <tjh@openssl.org>
2016-09-15 22:48:37 +01:00
Matt Caswell
dd8710dc54 Fix OCSP_RESPID processing bug introduced by WPACKET changes
An OCSP_RESPID in a status request extension has 2 bytes for the length
not 1.

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-09-14 10:26:36 +01:00
Matt Caswell
869d0a37cf Encourage use of the macros for the various "sub" functions
Don't call WPACKET_sub_memcpy(), WPACKET_sub_allocation_bytes() and
WPACKET_start_sub_packet_len() directly.

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-09-14 00:02:34 +01:00
Matt Caswell
b2b3024e0e Add a WPACKET_sub_allocate_bytes() function
Updated the construction code to use the new function. Also added some
convenience macros for WPACKET_sub_memcpy().

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-09-14 00:02:34 +01:00
Matt Caswell
f1ec23c0bc Convert CKE construction to use the WPACKET API
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-09-14 00:02:34 +01:00
Matt Caswell
77a6be4dfc Abort on unrecognised warning alerts
A peer continually sending unrecognised warning alerts could mean that we
make no progress on a connection. We should abort rather than continuing if
we receive an unrecognised warning alert.

Thanks to Shi Lei for reporting this issue.

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-09-13 11:51:00 +01:00
Matt Caswell
c0f9e23c6b Fix a few style nits in the wpacket code
Addressing more feedback comments.

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-09-13 09:41:21 +01:00
Matt Caswell
df065a2b3b Remove else after a return in packet code
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-09-13 09:41:21 +01:00
Matt Caswell
826573559d Pull out some common packet code into a function
Two locations had the same loop for writing out a value. Pull it out into
a function.

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-09-13 09:41:21 +01:00
Matt Caswell
c39609aa6a Add some soft asserts where applicable
This is an internal API. Some of the tests were for programmer erorr and
"should not happen" situations, so a soft assert is reasonable.

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-09-13 09:41:21 +01:00
Matt Caswell
de451856f0 Address WPACKET review comments
A few style tweaks here and there. The main change is that curr and
packet_len are now offsets into the buffer to account for the fact that
the pointers can change if the buffer grows. Also dropped support for the
WPACKET_set_packet_len() function. I thought that was going to be needed
but so far it hasn't been. It doesn't really work any more due to the
offsets change.

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-09-13 09:41:21 +01:00
Matt Caswell
6ae4f5e087 Simplify the overflow checks in WPACKET_allocate_bytes()
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-09-13 09:41:21 +01:00
Matt Caswell
9bf85bf9c5 Move the WPACKET documentation comments to packet_locl.h
The PACKET documentation is already in packet_locl.h so it makes sense to
have the WPACKET documentation there as well.

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-09-13 09:41:21 +01:00
Matt Caswell
796a627e0a Ensure the WPACKET gets cleaned up in the event of an error
Otherwise a mem leak can occur.

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-09-13 09:41:21 +01:00
Matt Caswell
871bc59bc1 Various bug fixes and tweaks to WPACKET implementation
Also added the WPACKET_cleanup() function to cleanup a WPACKET if we hit
an error.

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-09-13 09:41:21 +01:00
Matt Caswell
fb790f1673 Add WPACKET_sub_memcpy() function
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-09-13 09:41:21 +01:00
Matt Caswell
0217dd19c0 Move from explicit sub-packets to implicit ones
No need to declare an explicit sub-packet. Just start one.

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-09-13 09:41:21 +01:00
Matt Caswell
ae2f7b37da Rename PACKETW to WPACKET
To avoid confusion with the read PACKET structure.

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-09-13 09:41:21 +01:00
Matt Caswell
2c7b4dbc1a Convert tls_construct_client_hello() to use PACKETW
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-09-13 09:41:21 +01:00
Matt Caswell
b7273855ac First pass at writing a writeable packets API
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-09-13 09:41:21 +01:00
Matt Caswell
2d11f5b2ca Ensure trace recognises X25519
Using the -trace option to s_server or s_client was incorrectly printing
UNKNOWN for the X25519 curve.

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-09-08 12:34:02 +01:00
Rich Salz
252cfef151 Add missing debug strings.
Found by turning -Wswitch-enum on.

Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-09-07 16:08:38 -04:00
Matt Caswell
f046afb066 Ensure the CertStatus message adds a DTLS message header where needed
The function tls_construct_cert_status() is called by both TLS and DTLS
code. However it only ever constructed a TLS message header for the message
which obviously failed in DTLS.

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-08-30 11:32:49 +01:00
Rich Salz
e5f969a82f Remove trailing zeros
Reviewed-by: Andy Polyakov <appro@openssl.org>
2016-08-26 15:18:07 -04:00
Rich Salz
ef28891bab Put DES into "not default" category.
Add CVE to CHANGES

Reviewed-by: Emilia Käsper <emilia@openssl.org>
2016-08-24 14:05:52 +01:00
Rich Salz
d33726b92e To avoid SWEET32 attack, move 3DES to weak
Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
2016-08-24 14:05:52 +01:00
Rob Percival
6b13bd1dc2 Fix comment about return value of ct_extract_tls_extension_scts
Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
2016-08-24 13:58:19 +01:00
Matt Caswell
c42b8a6e4b Remove some dead code from rec_layer_s3.c
It is never valid to call ssl3_read_bytes with
type == SSL3_RT_CHANGE_CIPHER_SPEC, and in fact we check for valid values
for type near the beginning of the function. Therefore this check will never
be true and can be removed.

Reviewed-by: Tim Hudson <tjh@openssl.org>
2016-08-24 11:28:58 +01:00
Dr. Stephen Henson
e97763c92c Sanity check ticket length.
If a ticket callback changes the HMAC digest to SHA512 the existing
sanity checks are not sufficient and an attacker could perform a DoS
attack with a malformed ticket. Add additional checks based on
HMAC size.

Thanks to Shi Lei for reporting this bug.

CVE-2016-6302

Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
2016-08-23 23:16:24 +01:00
Matt Caswell
2f3930bc0e Fix leak on error in tls_construct_cke_gost
Don't leak pke_ctx on error.

Reviewed-by: Tim Hudson <tjh@openssl.org>
2016-08-23 00:19:15 +01:00
Matt Caswell
5cb4d6466a Prevent DTLS Finished message injection
Follow on from CVE-2016-2179

The investigation and analysis of CVE-2016-2179 highlighted a related flaw.

This commit fixes a security "near miss" in the buffered message handling
code. Ultimately this is not currently believed to be exploitable due to
the reasons outlined below, and therefore there is no CVE for this on its
own.

The issue this commit fixes is a MITM attack where the attacker can inject
a Finished message into the handshake. In the description below it is
assumed that the attacker injects the Finished message for the server to
receive it. The attack could work equally well the other way around (i.e
where the client receives the injected Finished message).

The MITM requires the following capabilities:
- The ability to manipulate the MTU that the client selects such that it
is small enough for the client to fragment Finished messages.
- The ability to selectively drop and modify records sent from the client
- The ability to inject its own records and send them to the server

The MITM forces the client to select a small MTU such that the client
will fragment the Finished message. Ideally for the attacker the first
fragment will contain all but the last byte of the Finished message,
with the second fragment containing the final byte.

During the handshake and prior to the client sending the CCS the MITM
injects a plaintext Finished message fragment to the server containing
all but the final byte of the Finished message. The message sequence
number should be the one expected to be used for the real Finished message.

OpenSSL will recognise that the received fragment is for the future and
will buffer it for later use.

After the client sends the CCS it then sends its own Finished message in
two fragments. The MITM causes the first of these fragments to be
dropped. The OpenSSL server will then receive the second of the fragments
and reassemble the complete Finished message consisting of the MITM
fragment and the final byte from the real client.

The advantage to the attacker in injecting a Finished message is that
this provides the capability to modify other handshake messages (e.g.
the ClientHello) undetected. A difficulty for the attacker is knowing in
advance what impact any of those changes might have on the final byte of
the handshake hash that is going to be sent in the "real" Finished
message. In the worst case for the attacker this means that only 1 in
256 of such injection attempts will succeed.

It may be possible in some situations for the attacker to improve this such
that all attempts succeed. For example if the handshake includes client
authentication then the final message flight sent by the client will
include a Certificate. Certificates are ASN.1 objects where the signed
portion is DER encoded. The non-signed portion could be BER encoded and so
the attacker could re-encode the certificate such that the hash for the
whole handshake comes to a different value. The certificate re-encoding
would not be detectable because only the non-signed portion is changed. As
this is the final flight of messages sent from the client the attacker
knows what the complete hanshake hash value will be that the client will
send - and therefore knows what the final byte will be. Through a process
of trial and error the attacker can re-encode the certificate until the
modified handhshake also has a hash with the same final byte. This means
that when the Finished message is verified by the server it will be
correct in all cases.

In practice the MITM would need to be able to perform the same attack
against both the client and the server. If the attack is only performed
against the server (say) then the server will not detect the modified
handshake, but the client will and will abort the connection.
Fortunately, although OpenSSL is vulnerable to Finished message
injection, it is not vulnerable if *both* client and server are OpenSSL.
The reason is that OpenSSL has a hard "floor" for a minimum MTU size
that it will never go below. This minimum means that a Finished message
will never be sent in a fragmented form and therefore the MITM does not
have one of its pre-requisites. Therefore this could only be exploited
if using OpenSSL and some other DTLS peer that had its own and separate
Finished message injection flaw.

The fix is to ensure buffered messages are cleared on epoch change.

Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-08-22 10:53:55 +01:00
Matt Caswell
f5c7f5dfba Fix DTLS buffered message DoS attack
DTLS can handle out of order record delivery. Additionally since
handshake messages can be bigger than will fit into a single packet, the
messages can be fragmented across multiple records (as with normal TLS).
That means that the messages can arrive mixed up, and we have to
reassemble them. We keep a queue of buffered messages that are "from the
future", i.e. messages we're not ready to deal with yet but have arrived
early. The messages held there may not be full yet - they could be one
or more fragments that are still in the process of being reassembled.

The code assumes that we will eventually complete the reassembly and
when that occurs the complete message is removed from the queue at the
point that we need to use it.

However, DTLS is also tolerant of packet loss. To get around that DTLS
messages can be retransmitted. If we receive a full (non-fragmented)
message from the peer after previously having received a fragment of
that message, then we ignore the message in the queue and just use the
non-fragmented version. At that point the queued message will never get
removed.

Additionally the peer could send "future" messages that we never get to
in order to complete the handshake. Each message has a sequence number
(starting from 0). We will accept a message fragment for the current
message sequence number, or for any sequence up to 10 into the future.
However if the Finished message has a sequence number of 2, anything
greater than that in the queue is just left there.

So, in those two ways we can end up with "orphaned" data in the queue
that will never get removed - except when the connection is closed. At
that point all the queues are flushed.

An attacker could seek to exploit this by filling up the queues with
lots of large messages that are never going to be used in order to
attempt a DoS by memory exhaustion.

I will assume that we are only concerned with servers here. It does not
seem reasonable to be concerned about a memory exhaustion attack on a
client. They are unlikely to process enough connections for this to be
an issue.

A "long" handshake with many messages might be 5 messages long (in the
incoming direction), e.g. ClientHello, Certificate, ClientKeyExchange,
CertificateVerify, Finished. So this would be message sequence numbers 0
to 4. Additionally we can buffer up to 10 messages in the future.
Therefore the maximum number of messages that an attacker could send
that could get orphaned would typically be 15.

The maximum size that a DTLS message is allowed to be is defined by
max_cert_list, which by default is 100k. Therefore the maximum amount of
"orphaned" memory per connection is 1500k.

Message sequence numbers get reset after the Finished message, so
renegotiation will not extend the maximum number of messages that can be
orphaned per connection.

As noted above, the queues do get cleared when the connection is closed.
Therefore in order to mount an effective attack, an attacker would have
to open many simultaneous connections.

Issue reported by Quan Luo.

CVE-2016-2179

Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-08-22 10:53:55 +01:00
Matt Caswell
1fb9fdc302 Fix DTLS replay protection
The DTLS implementation provides some protection against replay attacks
in accordance with RFC6347 section 4.1.2.6.

A sliding "window" of valid record sequence numbers is maintained with
the "right" hand edge of the window set to the highest sequence number we
have received so far. Records that arrive that are off the "left" hand
edge of the window are rejected. Records within the window are checked
against a list of records received so far. If we already received it then
we also reject the new record.

If we have not already received the record, or the sequence number is off
the right hand edge of the window then we verify the MAC of the record.
If MAC verification fails then we discard the record. Otherwise we mark
the record as received. If the sequence number was off the right hand edge
of the window, then we slide the window along so that the right hand edge
is in line with the newly received sequence number.

Records may arrive for future epochs, i.e. a record from after a CCS being
sent, can arrive before the CCS does if the packets get re-ordered. As we
have not yet received the CCS we are not yet in a position to decrypt or
validate the MAC of those records. OpenSSL places those records on an
unprocessed records queue. It additionally updates the window immediately,
even though we have not yet verified the MAC. This will only occur if
currently in a handshake/renegotiation.

This could be exploited by an attacker by sending a record for the next
epoch (which does not have to decrypt or have a valid MAC), with a very
large sequence number. This means the right hand edge of the window is
moved very far to the right, and all subsequent legitimate packets are
dropped causing a denial of service.

A similar effect can be achieved during the initial handshake. In this
case there is no MAC key negotiated yet. Therefore an attacker can send a
message for the current epoch with a very large sequence number. The code
will process the record as normal. If the hanshake message sequence number
(as opposed to the record sequence number that we have been talking about
so far) is in the future then the injected message is bufferred to be
handled later, but the window is still updated. Therefore all subsequent
legitimate handshake records are dropped. This aspect is not considered a
security issue because there are many ways for an attacker to disrupt the
initial handshake and prevent it from completing successfully (e.g.
injection of a handshake message will cause the Finished MAC to fail and
the handshake to be aborted). This issue comes about as a result of trying
to do replay protection, but having no integrity mechanism in place yet.
Does it even make sense to have replay protection in epoch 0? That
issue isn't addressed here though.

This addressed an OCAP Audit issue.

CVE-2016-2181

Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-08-19 13:52:40 +01:00
Matt Caswell
738ad946dd Fix DTLS unprocessed records bug
During a DTLS handshake we may get records destined for the next epoch
arrive before we have processed the CCS. In that case we can't decrypt or
verify the record yet, so we buffer it for later use. When we do receive
the CCS we work through the queue of unprocessed records and process them.

Unfortunately the act of processing wipes out any existing packet data
that we were still working through. This includes any records from the new
epoch that were in the same packet as the CCS. We should only process the
buffered records if we've not got any data left.

Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-08-19 13:52:40 +01:00
Emilia Kasper
a230b26e09 Indent ssl/
Run util/openssl-format-source on ssl/

Some comments and hand-formatted tables were fixed up
manually by disabling auto-formatting.

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-08-18 14:02:29 +02:00
Dr. Stephen Henson
2e5ead831b Constify ssl_cert_type()
Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-08-17 15:49:44 +01:00
Dr. Stephen Henson
8900f3e398 Convert X509* functions to use const getters
Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-08-17 13:59:04 +01:00
Remi Gacogne
fddfc0afc8 Add missing session id and tlsext_status accessors
* SSL_SESSION_set1_id()
 * SSL_SESSION_get0_id_context()
 * SSL_CTX_get_tlsext_status_cb()
 * SSL_CTX_get_tlsext_status_arg()

Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
2016-08-17 10:38:20 +01:00
Matt Caswell
48593cb12a Convert SSL_SESSION* functions to use const getters
Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
Reviewed-by: Stephen Henson <steve@openssl.org>
2016-08-16 23:36:28 +01:00
Matt Caswell
f9cf774cbd Ensure we unpad in constant time for read pipelining
The read pipelining code broke constant time unpadding. See GitHub
issue #1438

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-08-16 16:53:17 +01:00
David Woodhouse
31c34a3e2f Fix satsub64be() to unconditionally use 64-bit integers
Now we support (u)int64_t this can be very much simpler.

Reviewed-by: Andy Polyakov <appro@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
2016-08-16 10:24:57 +01:00
Matt Caswell
78fcddbb8d Address feedback on SSLv2 ClientHello processing
Reviewed-by: Tim Hudson <tjh@openssl.org>
2016-08-15 23:14:30 +01:00
Matt Caswell
a01c86a251 Send an alert if we get a non-initial record with the wrong version
If we receive a non-initial record but the version number isn't right then
we should send an alert.

Reviewed-by: Tim Hudson <tjh@openssl.org>
2016-08-15 23:14:30 +01:00
Matt Caswell
44efb88a21 Address feedback on SSLv2 ClientHello processing
Feedback on the previous SSLv2 ClientHello processing fix was that it
breaks layering by reading init_num in the record layer. It also does not
detect if there was a previous non-fatal warning.

This is an alternative approach that directly tracks in the record layer
whether this is the first record.

GitHub Issue #1298

Reviewed-by: Tim Hudson <tjh@openssl.org>
2016-08-15 23:14:30 +01:00
Rob Percival
a1bb7708ce Improves CTLOG_STORE setters
Changes them to have clearer ownership semantics, as suggested in
https://github.com/openssl/openssl/pull/1372#discussion_r73232196.

Reviewed-by: Emilia Käsper <emilia@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/1408)
2016-08-15 12:56:47 -04:00
Dr. Stephen Henson
0a699a0723 Fix no-ec
Fix no-ec builds by having separate functions to create keys based on
an existing EVP_PKEY and a curve id.

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-08-15 14:07:33 +01:00
Dr. Stephen Henson
ec24630ae2 Modify TLS support for new X25519 API.
When handling ECDH check to see if the curve is "custom" (X25519 is
currently the only curve of this type) and instead of setting a curve
NID just allocate a key of appropriate type.

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-08-13 14:11:05 +01:00
Rich Salz
e928132343 GH1446: Add SSL_SESSION_get0_cipher
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/1451)
2016-08-12 15:23:48 -04:00
Adam Langley
eea8723cd0 Fix test of first of 255 CBC padding bytes.
Thanks to Peter Gijsels for pointing out that if a CBC record has 255
bytes of padding, the first was not being checked.

(This is an import of change 80842bdb from BoringSSL.)

Reviewed-by: Emilia Käsper <emilia@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/1431)
2016-08-08 13:36:55 -07:00
JimC
a4a18b2f89 Fix CIPHER_DEBUG
Commit 3eb2aff renamed a field of ssl_cipher_st from algorithm_ssl -> min_tls but neglected to update the fprintf reference which is included by -DCIPHER_DEBUG

Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/1417)
2016-08-06 10:03:25 -04:00
klemens
6025001707 spelling fixes, just comments and readme.
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/1413)
2016-08-05 19:07:30 -04:00
Richard Levitte
e38da82047 Remove OPENSSL_NO_STDIO guards around certain SSL cert/key functions
These functions are:

    SSL_use_certificate_file
    SSL_use_RSAPrivateKey_file
    SSL_use_PrivateKey_file
    SSL_CTX_use_certificate_file
    SSL_CTX_use_RSAPrivateKey_file
    SSL_CTX_use_PrivateKey_file
    SSL_use_certificate_chain_file

Internally, they use BIO_s_file(), which is defined and implemented at
all times, even when OpenSSL is configured no-stdio.

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-08-05 21:18:15 +02:00
David Woodhouse
2e94723c1b Fix ubsan 'left shift of negative value -1' error in satsub64be()
Baroque, almost uncommented code triggers behaviour which is undefined
by the C standard. You might quite reasonably not care that the code was
broken on ones-complement machines, but if we support a ubsan build then
we need to at least pretend to care.

It looks like the special-case code for 64-bit big-endian is going to
behave differently (and wrongly) on wrap-around, because it treats the
values as signed. That seems wrong, and allows replay and other attacks.
Surely you need to renegotiate and start a new epoch rather than
wrapping around to sequence number zero again?

Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
2016-08-04 20:56:24 +01:00
David Woodhouse
032924c4b4 Make DTLS1_BAD_VER work with DTLS_client_method()
DTLSv1_client_method() is deprecated, but it was the only way to obtain
DTLS1_BAD_VER support. The SSL_OP_CISCO_ANYCONNECT hack doesn't work with
DTLS_client_method(), and it's relatively non-trivial to make it work without
expanding the hack into lots of places.

So deprecate SSL_OP_CISCO_ANYCONNECT with DTLSv1_client_method(), and make
it work with SSL_CTX_set_{min,max}_proto_version(DTLS1_BAD_VER) instead.

Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
2016-08-04 20:56:24 +01:00
David Woodhouse
387cf21345 Fix cipher support for DTLS1_BAD_VER
Commit 3eb2aff40 ("Add support for minimum and maximum protocol version
supported by a cipher") disabled all ciphers for DTLS1_BAD_VER.

That wasn't helpful. Give them back.

Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
2016-08-04 20:56:23 +01:00
David Woodhouse
ff4952896e Fix DTLS_VERSION_xx() comparison macros for DTLS1_BAD_VER
DTLS version numbers are strange and backwards, except DTLS1_BAD_VER so
we have to make a special case for it.

This does leave us with a set of macros which will evaluate their arguments
more than once, but it's not a public-facing API and it's not like this is
the kind of thing where people will be using DTLS_VERSION_LE(x++, y) anyway.

Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
2016-08-04 20:56:23 +01:00
David Woodhouse
e6027420b7 Fix ossl_statem_client_max_message_size() for DTLS1_BAD_VER
The Change Cipher Spec message in this ancient pre-standard version of DTLS
that Cisco are unfortunately still using in their products, is 3 bytes.

Allow it.

Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
2016-08-04 20:56:23 +01:00
David Woodhouse
c8a18468ca Fix SSL_export_keying_material() for DTLS1_BAD_VER
Commit d8e8590e ("Fix missing return value checks in SCTP") made the
DTLS handshake fail, even for non-SCTP connections, if
SSL_export_keying_material() fails. Which it does, for DTLS1_BAD_VER.

Apply the trivial fix to make it succeed, since there's no real reason
why it shouldn't even though we never need it.

Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
2016-08-04 20:56:23 +01:00
Ben Laurie
3260adf190 peer_tmp doesn't exist if no-ec no-dh.
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-08-01 11:30:33 +01:00
Matt Caswell
58c27c207d Fix crash as a result of MULTIBLOCK
The MULTIBLOCK code uses a "jumbo" sized write buffer which it allocates
and then frees later. Pipelining however introduced multiple pipelines. It
keeps track of how many pipelines are initialised using numwpipes.
Unfortunately the MULTIBLOCK code was not updating this when in deallocated
its buffers, leading to a buffer being marked as initialised but set to
NULL.

RT#4618

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-07-30 11:46:20 +01:00
Matt Caswell
65e2d67254 Simplify and rename SSL_set_rbio() and SSL_set_wbio()
SSL_set_rbio() and SSL_set_wbio() are new functions in 1.1.0 and really
should be called SSL_set0_rbio() and SSL_set0_wbio(). The old
implementation was not consistent with what "set0" means though as there
were special cases around what happens if the rbio and wbio are the same.
We were only ever taking one reference on the BIO, and checking everywhere
whether the rbio and wbio are the same so as not to double free.

A better approach is to rename the functions to SSL_set0_rbio() and
SSL_set0_wbio(). If an existing BIO is present it is *always* freed
regardless of whether the rbio and wbio are the same or not. It is
therefore the callers responsibility to ensure that a reference is taken
for *each* usage, i.e. one for the rbio and one for the wbio.

The legacy function SSL_set_bio() takes both the rbio and wbio in one go
and sets them both. We can wrap up the old behaviour in the implementation
of that function, i.e. previously if the rbio and wbio are the same in the
call to this function then the caller only needed to ensure one reference
was passed. This behaviour is retained by internally upping the ref count.

This commit was inspired by BoringSSL commit f715c423224.

RT#4572

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-07-29 14:09:57 +01:00
Matt Caswell
b46fe860fe Fix BIO_pop for SSL BIOs
The BIO_pop implementation assumes that the rbio still equals the next BIO
in the chain. While this would normally be the case, it is possible that it
could have been changed directly by the application. It also does not
properly cater for the scenario where the buffering BIO is still in place
for the write BIO.

Most of the existing BIO_pop code for SSL BIOs can be replaced by a single
call to SSL_set_bio(). This is equivalent to the existing code but
additionally handles the scenario where the rbio has been changed or the
buffering BIO is still in place.

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-07-29 14:09:57 +01:00
Matt Caswell
eddef30589 Fix BIO_push ref counting for SSL BIO
When pushing a BIO onto an SSL BIO we set the rbio and wbio for the SSL
object to be the BIO that has been pushed. Therefore we need to up the ref
count for that BIO. The existing code was uping the ref count on the wrong
BIO.

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-07-29 14:09:57 +01:00
Matt Caswell
8e3854ac88 Don't double free the write bio
When setting the read bio we free up any old existing one. However this can
lead to a double free if the existing one is the same as the write bio.

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-07-29 14:09:57 +01:00
Matt Caswell
0647719d80 Make the checks for an SSLv2 style record stricter
SSLv2 is no longer supported in 1.1.0, however we *do* still accept an SSLv2
style ClientHello, as long as we then subsequently negotiate a protocol
version >= SSLv3. The record format for SSLv2 style ClientHellos is quite
different to SSLv3+. We only accept this format in the first record of an
initial ClientHello. Previously we checked this by confirming
s->first_packet is set and s->server is true. However, this really only
tells us that we are dealing with an initial ClientHello, not that it is
the first record (s->first_packet is badly named...it really means this is
the first message). To check this is the first record of the initial
ClientHello we should also check that we've not received any data yet
(s->init_num == 0), and that we've not had any empty records.

GitHub Issue #1298

Reviewed-by: Emilia Käsper <emilia@openssl.org>
2016-07-29 12:42:40 +01:00
russor
78a01b3f69 zero pad DHE public key in ServerKeyExchange message for interop
Some versions of the Microsoft TLS stack have problems when the DHE public key
is encoded with fewer bytes than the DHE prime.

There's some public acknowledgement of the bug at these links:

https://connect.microsoft.com/IE/feedback/details/1253526/tls-serverkeyexchange-with-1024-dhe-may-encode-dh-y-as-127-bytes-breaking-internet-explorer-11
https://connect.microsoft.com/IE/feedback/details/1104905/wininet-calculation-of-mac-in-tls-handshake-intermittently-fails-for-dhe-rsa-key-exchange

This encoding issue also causes the same errors with 2048-bit DHE, if the
public key is encoded in fewer than 256 bytes and includes the TLS stack on
Windows Phone 8.x.

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/1320)
2016-07-25 13:41:33 -04:00
FdaSilvaYY
d3d5dc607a Enforce and explicit some const casting
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/1300)
2016-07-25 08:20:00 -04:00
Richard Levitte
8b9546c708 Correct misspelt OPENSSL_NO_SRP
RT#4619

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-07-23 10:47:52 +02:00
Dr. Stephen Henson
31a7d80d0d Send alert for bad DH CKE
RT#4511

Reviewed-by: Matt Caswell <matt@openssl.org>
2016-07-22 15:55:38 +01:00
Richard Levitte
912c258fc9 Have load_buildtin_compression in ssl/ssl_ciph.c return RUN_ONCE result
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-07-22 11:56:45 +02:00
Kurt Roeckx
69588edbaa Check for errors allocating the error strings.
Reviewed-by: Richard Levitte <levitte@openssl.org>
GH: #1330
2016-07-20 19:20:53 +02:00
Matt Caswell
2e7dc7cd68 Never expose ssl->bbio in the public API.
This is adapted from BoringSSL commit 2f87112b963.

This fixes a number of bugs where the existence of bbio was leaked in the
public API and broke things.

- SSL_get_wbio returned the bbio during the handshake. It must always return
  the BIO the consumer configured. In doing so, some internal accesses of
  SSL_get_wbio should be switched to ssl->wbio since those want to see bbio.

- The logic in SSL_set_rfd, etc. (which I doubt is quite right since
  SSL_set_bio's lifetime is unclear) would get confused once wbio got
  wrapped. Those want to compare to SSL_get_wbio.

- If SSL_set_bio was called mid-handshake, bbio would get disconnected and
  lose state. It forgets to reattach the bbio afterwards. Unfortunately,
  Conscrypt does this a lot. It just never ended up calling it at a point
  where the bbio would cause problems.

- Make more explicit the invariant that any bbio's which exist are always
  attached. Simplify a few things as part of that.

RT#4572

Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-07-20 13:08:08 +01:00
FdaSilvaYY
e8aa8b6c8f Fix a few if(, for(, while( inside code.
Fix some indentation at the same time

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/1292)
2016-07-20 07:21:53 -04:00
Dr. Stephen Henson
52eede5a97 Sanity check in ssl_get_algorithm2().
RT#4600

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-07-20 00:09:46 +01:00
Dr. Stephen Henson
fb9339827b Send alert on CKE error.
RT#4610

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-07-20 00:03:43 +01:00
Richard Levitte
c2e4e5d248 Change all our uses of CRYPTO_THREAD_run_once to use RUN_ONCE instead
That way, we have a way to check if the init function was successful
or not.

Reviewed-by: Kurt Roeckx <kurt@openssl.org>
2016-07-19 23:49:54 +02:00
Emilia Kasper
70c22888c1 Fix two bugs in clienthello processing
- Always process ALPN (previously there was an early return in the
  certificate status handling)
- Don't send a duplicate alert. Previously, both
  ssl_check_clienthello_tlsext_late and its caller would send an
  alert. Consolidate alert sending code in the caller.

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-07-19 14:18:03 +02:00
Emilia Kasper
ce2cdac278 SSL test framework: port NPN and ALPN tests
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-07-19 14:17:48 +02:00
Matt Caswell
4fa88861ee Update error codes following tls_process_key_exchange() refactor
Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-07-19 12:18:46 +01:00
Matt Caswell
e1e588acae Tidy up tls_process_key_exchange()
After the refactor of tls_process_key_exchange(), this commit tidies up
some loose ends.

Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-07-19 12:18:46 +01:00
Matt Caswell
ff74aeb1fa Split out ECDHE from tls_process_key_exchange()
Continuing from the previous commit. Refactor tls_process_key_exchange() to
split out into a separate function the ECDHE aspects.

Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-07-19 12:18:46 +01:00
Matt Caswell
e01a610db8 Split out DHE from tls_process_key_exchange()
Continuing from the previous commit. Refactor tls_process_key_exchange() to
split out into a separate function the DHE aspects.

Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-07-19 12:18:46 +01:00
Matt Caswell
25c6c10cd7 Split out SRP from tls_process_key_exchange()
Continuing from the previous commit. Refactor tls_process_key_exchange() to
split out into a separate function the SRP aspects.

Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-07-19 12:18:46 +01:00
Matt Caswell
7dc1c64774 Split out the PSK preamble from tls_process_key_exchange()
The tls_process_key_exchange() function is too long. This commit starts
the process of splitting it up by moving the PSK preamble code to a
separate function.

Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-07-19 12:18:46 +01:00
Matt Caswell
02a74590bb Move the PSK preamble for tls_process_key_exchange()
The function tls_process_key_exchange() is too long. This commit moves
the PSK preamble processing out to a separate function.

Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-07-19 12:18:46 +01:00
Matt Caswell
be8dba2c92 Narrow scope of locals vars in tls_process_key_exchange()
Narrow the scope of the local vars in preparation for split up this
function.

Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-07-19 12:18:46 +01:00
Matt Caswell
e4612d02c5 Remove sessions from external cache, even if internal cache not used.
If the SSL_SESS_CACHE_NO_INTERNAL_STORE cache mode is used then we weren't
removing sessions from the external cache, e.g. if an alert occurs the
session is supposed to be automatically removed.

Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-07-19 12:08:49 +01:00
Richard Levitte
bbba0a7dff make update
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-07-19 11:50:42 +02:00
Richard Levitte
340a282853 Fixup a few SSLerr calls in ssl/statem/
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-07-19 11:50:31 +02:00
Matt Caswell
e3ea3afd6d Refactor Identity Hint handling
Don't call strncpy with strlen of the source as the length. Don't call
strlen multiple times. Eventually we will want to replace this with a proper
PACKET style handling (but for construction of PACKETs instead of just
reading them as it is now). For now though this is safe because
PSK_MAX_IDENTITY_LEN will always fit into the destination buffer.

This addresses an OCAP Audit issue.

Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-07-18 23:18:46 +01:00
Matt Caswell
05ec6a25f8 Fix up error codes after splitting up tls_construct_key_exchange()
Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-07-18 23:05:15 +01:00
Matt Caswell
a7a752285a Some tidy ups after the CKE construction refactor
Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-07-18 23:05:15 +01:00
Matt Caswell
840a2bf8ec Split out SRP CKE construction into a separate function
Continuing previous commit to break up the
tls_construct_client_key_exchange() function. This splits out the SRP
code.

Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-07-18 23:05:14 +01:00
Matt Caswell
e00e0b3d84 Split out GOST CKE construction into a separate function
Continuing previous commit to break up the
tls_construct_client_key_exchange() function. This splits out the GOST
code.

Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-07-18 23:05:14 +01:00
Matt Caswell
67ad5aabe6 Split out DHE CKE construction into a separate function
Continuing previous commit to break up the
tls_construct_client_key_exchange() function. This splits out the ECDHE
code.

Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-07-18 23:05:14 +01:00
Matt Caswell
a8c1c7040a Split out DHE CKE construction into a separate function
Continuing previous commit to break up the
tls_construct_client_key_exchange() function. This splits out the DHE
code.

Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-07-18 23:05:14 +01:00
Matt Caswell
13c0ec4ad4 Split out CKE construction PSK pre-amble and RSA into a separate function
The tls_construct_client_key_exchange() function is too long. This splits
out the construction of the PSK pre-amble into a separate function as well
as the RSA construction.

Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-07-18 23:05:14 +01:00
Matt Caswell
0bce0b02d8 Narrow the scope of local variables in tls_construct_client_key_exchange()
This is in preparation for splitting up this over long function.

Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-07-18 23:05:14 +01:00
Matt Caswell
d4450e4bb9 Fix bug with s2n et al macros
The parameters should have parens around them when used.

Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-07-18 23:05:14 +01:00
Matt Caswell
c76a4aead2 Errors fix up following break up of CKE processing
Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-07-18 22:55:07 +01:00
Matt Caswell
9059eb711f Remove the f_err lable from tls_process_client_key_exchange()
The f_err label is no longer needed so it can be removed.

Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-07-18 22:55:07 +01:00
Matt Caswell
c437eef60a Split out GOST from process CKE code
Continuing from the previous commits, this splits out the GOST code into
a separate function from the process CKE code.

Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-07-18 22:55:07 +01:00
Matt Caswell
19ed1ec12e Split out ECDHE from process CKE code
Continuing from the previous commits, this splits out the ECDHE code into
a separate function from the process CKE code.

Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-07-18 22:55:07 +01:00
Matt Caswell
642360f9a3 Split out DHE from process CKE code
Continuing from the previous commit, this splits out the DHE code into
a separate function from the process CKE code.

Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-07-18 22:55:07 +01:00
Matt Caswell
0907d7105c Split out PSK preamble and RSA from process CKE code
The tls_process_client_key_exchange() function is far too long. This
splits out the PSK preamble processing, and the RSA processing into
separate functions.

Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-07-18 22:55:07 +01:00
Matt Caswell
bb5592dd7b Reduce the scope of some variables in tls_process_client_key_exchange()
In preparation for splitting this function up into smaller functions this
commit reduces the scope of some of the variables to only be in scope for
the algorithm specific parts. In some cases that makes the error handling
more verbose than it needs to be - but we'll clean that up in a later
commit.

Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-07-18 22:55:07 +01:00
Matt Caswell
23dd09b5e9 Fix formatting in statem_srvr.c based on review feedback
Also elaborated a comment based on feedback.

Reviewed-by: Emilia Käsper <emilia@openssl.org>
2016-07-18 14:30:14 +01:00
Matt Caswell
0f512756e2 Try and make the transition tests for CKE message clearer
The logic testing whether a CKE message is allowed or not was a little
difficult to follow. This tries to clean it up.

Reviewed-by: Emilia Käsper <emilia@openssl.org>
2016-07-18 14:30:14 +01:00
Matt Caswell
7d2c13a705 Simplify key_exchange_expected() logic
The static function key_exchange_expected() used to return -1 on error.
Commit 361a119127 changed that so that it can never fail. This means that
some tidy up can be done to simplify error handling in callers of that
function.

Reviewed-by: Emilia Käsper <emilia@openssl.org>
2016-07-18 14:30:14 +01:00
Matt Caswell
149c2ef5ec Make sure we call ssl3_digest_cached_records() when necessary
Having received a ClientKeyExchange message instead of a Certificate we
know that we are not going to receive a CertificateVerify message. This
means we can free up the handshake_buffer. However we better call
ssl3_digest_cached_records() instead of just freeing it up, otherwise we
later try and use it anyway and a core dump results. This could happen,
for example, in SSLv3 where we send a CertificateRequest but the client
sends no Certificate message at all. This is valid in SSLv3 (in TLS
clients are required to send an empty Certificate message).

Found using the BoringSSL test suite.

Reviewed-by: Emilia Käsper <emilia@openssl.org>
2016-07-18 14:30:14 +01:00
Matt Caswell
672f3337c3 Fix SSLv3 alert if no Client Ceritifcate sent after a request for one
In TLS if the server sends a CertificateRequest and the client does not
provide one, if the server cannot continue it should send a
HandshakeFailure alert. In SSLv3 the same should happen, but instead we
were sending an UnexpectedMessage alert. This is incorrect - the message
isn't unexpected - it is valid for the client not to send one - its just
that we cannot continue without one.

Reviewed-by: Emilia Käsper <emilia@openssl.org>
2016-07-18 14:30:14 +01:00
Matt Caswell
05c4f1d563 Prepare the client certificate earlier
Move the preparation of the client certificate to be post processing work
after reading the CertificateRequest message rather than pre processing
work prior to writing the Certificate message. As part of preparing the
client certificate we may discover that we do not have one available. If
we are also talking SSLv3 then we won't send the Certificate message at
all. However, if we don't discover this until we are about to send the
Certificate message it is too late and we send an empty one anyway. This
is wrong for SSLv3.

Reviewed-by: Emilia Käsper <emilia@openssl.org>
2016-07-18 14:30:14 +01:00
Miroslav Franc
563c1ec618 fix memory leaks
Reviewed-by: Kurt Roeckx <kurt@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/1313)
2016-07-16 12:32:34 -04:00
Dr. Stephen Henson
d166ed8c11 check return values for EVP_Digest*() APIs
Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-07-15 14:09:05 +01:00
David Benjamin
e99ab8ffd7 Fix DH error-handling in tls_process_key_exchange.
The set0 setters take ownership of their arguments, so the values should
be set to NULL to avoid a double-free in the cleanup block should
ssl_security(SSL_SECOP_TMP_DH) fail. Found by BoringSSL's WeakDH test.

Reviewed-by: Kurt Roeckx <kurt@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/1299)
2016-07-12 15:39:42 -04:00
Viktor Dukhovni
5ae4ceb92c Perform DANE-EE(3) name checks by default
In light of potential UKS (unknown key share) attacks on some
applications, primarily browsers, despite RFC761, name checks are
by default applied with DANE-EE(3) TLSA records.  Applications for
which UKS is not a problem can optionally disable DANE-EE(3) name
checks via the new SSL_CTX_dane_set_flags() and friends.

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-07-12 10:16:34 -04:00
Rich Salz
54478ac92a GH1278: Removed error code for alerts
Commit aea145e removed some error codes that are generated
algorithmically: mapping alerts to error texts.  Found by
Andreas Karlsson.  This restores them, and adds two missing ones.

Reviewed-by: Matt Caswell <matt@openssl.org>
2016-07-08 13:28:33 -04:00
Andreas Karlsson
9d6daf99c2 Fix broken loading of client CAs
The SSL_load_client_CA_file() failed to load any CAs due to an
inccorrect assumption about the return value of lh_*_insert(). The
return value when inserting into a hash is the old value of the key.

The bug was introduced in 3c82e437bb.

Reviewed-by: Kurt Roeckx <kurt@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/1279)
2016-07-02 15:30:13 -04:00
Matt Caswell
1e16987fc1 Avoid an overflow in constructing the ServerKeyExchange message
We calculate the size required for the ServerKeyExchange message and then
call BUF_MEM_grow_clean() on the buffer. However we fail to take account of
2 bytes required for the signature algorithm and 2 bytes for the signature
length, i.e. we could overflow by 4 bytes. In reality this won't happen
because the buffer is pre-allocated to a large size that means it should be
big enough anyway.

Addresses an OCAP Audit issue.

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-07-01 19:23:29 +01:00
FdaSilvaYY
0485d5406a Whitespace cleanup in ssl folder
Reviewed-by: Andy Polyakov <appro@openssl.org>
Reviewed-by: Kurt Roeckx <kurt@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/1264)
2016-06-29 09:56:39 -04:00
FdaSilvaYY
9d22666eb8 Spelling
Reviewed-by: Andy Polyakov <appro@openssl.org>
Reviewed-by: Kurt Roeckx <kurt@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/1264)
2016-06-29 09:56:39 -04:00
Matt Caswell
63916e9a23 Ensure read records are marked as read
In some situations (such as when we receive a fragment of an alert)
we try to get the next packet but did not mark the current one as read,
meaning that we got the same record back again - leading to an infinite
loop.

Found using the BoringSSL test suite.

Reviewed-by: Andy Polyakov <appro@openssl.org>
2016-06-27 14:51:03 +01:00
FdaSilvaYY
3c82e437bb Add checks on sk_TYPE_push() returned result
Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
2016-06-23 14:03:29 +01:00
FdaSilvaYY
f430ba31ac Spelling... and more spelling
Reviewed-by: Kurt Roeckx <kurt@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/1245)
2016-06-22 00:26:10 +02:00
David Benjamin
5b8fa431ae Make RSA key exchange code actually constant-time.
Using RSA_PKCS1_PADDING with RSA_private_decrypt is inherently unsafe.
The API requires writing output on success and touching the error queue
on error. Thus, although the padding check itself is constant-time as of
294d1e36c2, and the logic after the
decryption in the SSL code is constant-time as of
adb46dbc6d, the API boundary in the middle
still leaks whether the padding check succeeded, giving us our
much-loved Bleichenbacher padding oracle.

Instead, PKCS#1 padding must be handled by the caller which uses
RSA_NO_PADDING, in timing-sensitive code integrated with the
Bleichenbacher mitigation. Removing PKCS#1 padding in constant time is
actually much simpler when the expected length is a constant (and if
it's not a constant, avoiding a padding oracle seems unlikely), so just
do it inline.

Signed-off-by: Kurt Roeckx <kurt@roeckx.be>
Reviewed-by: Rich Salz <rsalz@openssl.org>

GH: #1222
2016-06-21 20:55:54 +02:00
FdaSilvaYY
823146d65f Useless header include of openssl/rand.h
Reviewed-by: Kurt Roeckx <kurt@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/1168)
2016-06-18 16:30:24 -04:00
Richard Levitte
2ac6115d9e Deal with the consequences of constifying getters
Reviewed-by: Stephen Henson <steve@openssl.org>
Reviewed-by: Emilia Käsper <emilia@openssl.org>
2016-06-15 20:09:27 +02:00
Kurt Roeckx
947f3156ec Initialize the session_id
ssl_session_hash() always looks at the first 4 bytes, regardless of the length.
A client can send a session id that's shorter, and the callback could also
generate one that's shorter.  So we make sure that the rest of the buffer is
initliazed to 0 so that we always calculate the same hash.

Found by tis-interpreter, also previously reported as RT #2871

Reviewed-by: Rich Salz <rsalz@openssl.org>

MR: #2911
2016-06-14 19:30:36 +02:00
Matt Caswell
e7653f3bab Fix comment
Fix a comment following commit c2c49969e2.

RT2388

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-06-14 10:41:00 +01:00
Matt Caswell
d356dc5619 Add some missing return value checks
Some misc return value checks

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-06-13 17:38:39 +01:00
Matt Caswell
e70656cf1c Ensure SSL_set_session clears the old session from cache if it is bad
SSL_clear() and SSL_free() will remove a session from the cache if it is
considered "bad". However SSL_set_session() does not do this for the session
it is replacing.

SSL_clear() clears an SSL object ready for reuse. It does not clear the
session though. This means that:

  SSL_clear(s)
  SSL_set_session(s, sess);

and
  SSL_set_session(s, sess);
  SSL_clear(s);

do not do the same thing, although logically you would expect that they
would.

The failure of SSL_set_session() to remove bad sessions from the cache
should be considered a bug, so this commit fixes it.

RT#597

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-06-13 17:35:18 +01:00
Kurt Roeckx
325cfa8531 Don't compare a just free()d pointer
Found by tis-interpreter

Reviewed-by: Rich Salz <rsalz@openssl.org>

GH: #1173
2016-06-11 16:43:49 +02:00
Laszlo Kovacs
4f6eaa592a RT3720 Increment session miss counter properly
Reviewed-by: Kurt Roeckx <kurt@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
2016-06-09 23:34:18 +01:00
Todd Short
5c753de668 Fix session ticket and SNI
When session tickets are used, it's possible that SNI might swtich the
SSL_CTX on an SSL. Normally, this is not a problem, because the
initial_ctx/session_ctx are used for all session ticket/id processes.

However, when the SNI callback occurs, it's possible that the callback
may update the options in the SSL from the SSL_CTX, and this could
cause SSL_OP_NO_TICKET to be set. If this occurs, then two bad things
can happen:

1. The session ticket TLSEXT may not be written when the ticket expected
flag is set. The state machine transistions to writing the ticket, and
the client responds with an error as its not expecting a ticket.
2. When creating the session ticket, if the ticket key cb returns 0
the crypto/hmac contexts are not initialized, and the code crashes when
trying to encrypt the session ticket.

To fix 1, if the ticket TLSEXT is not written out, clear the expected
ticket flag.
To fix 2, consider a return of 0 from the ticket key cb a recoverable
error, and write a 0 length ticket and continue. The client-side code
can explicitly handle this case.

Fix these two cases, and add unit test code to validate ticket behavior.

Reviewed-by: Emilia Käsper <emilia@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/1098)
2016-06-09 13:07:51 -04:00
Rich Salz
e417070c9f Add some accessor API's
GH1098: Add X509_get_pathlen() (and a test)
GH1097:  Add SSL_is_dtls() function.

Documented.

Reviewed-by: Matt Caswell <matt@openssl.org>
2016-06-08 11:37:06 -04:00
Todd Short
e2bb9b9bf3 Always use session_ctx when removing a session
Sessions are stored on the session_ctx, which doesn't change after
SSL_set_SSL_CTX().

Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
2016-06-08 15:22:41 +01:00
Matt Caswell
255cfeacd8 Reject out of context empty records
Previously if we received an empty record we just threw it away and
ignored it. Really though if we get an empty record of a different content
type to what we are expecting then that should be an error, i.e. we should
reject out of context empty records. This commit makes the necessary changes
to achieve that.

RT#4395

Reviewed-by: Andy Polyakov <appro@openssl.org>
2016-06-07 22:07:36 +01:00
Matt Caswell
0aac3a6b19 Fix pipelining bug
The number of read pipelines should be reset in the event of reuse of an
SSL object.

Reviewed-by: Andy Polyakov <appro@openssl.org>
2016-06-07 22:07:36 +01:00
Matt Caswell
30b967651c Add SSL_CTX_get_tlsext_status_type()
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-06-07 17:05:52 +01:00
Matt Caswell
93a9d5975e Return the value of tlsext_status_type in the return not arg
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-06-07 17:05:52 +01:00
Alessandro Ghedini
4300aaf351 Add SSL_get_tlsext_status_type() method
The tlsext_status_type field in SSL is used by e.g. OpenResty to determine
if the client requested the certificate status, but SSL is now opaque.

Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
2016-06-07 17:05:52 +01:00
Rich Salz
255cf605d6 RT3895: Remove fprintf's from SSL library.
Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-06-04 07:08:29 -04:00
Matt Caswell
2c4a056f59 Handle a memory allocation failure in ssl3_init_finished_mac()
The ssl3_init_finished_mac() function can fail, in which case we need to
propagate the error up through the stack.

RT#3198

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-06-03 20:29:04 +01:00
TJ Saunders
80c630f657 Remove null check, per review feedback. Note this in the docs.
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/1135)
2016-05-31 17:16:29 -04:00
TJ Saunders
bd01f6498c Add an SSL_SESSION accessor for obtaining the protocol version number, with
accompanying documentation.

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/1135)
2016-05-31 17:16:29 -04:00
Dr. Stephen Henson
cc7113e8de return error in ct_move_scts()
Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
2016-05-31 13:02:20 +01:00
Todd Short
4379d5ce78 Fix ssl_cert_set0_chain invalid pointer
When setting the certificate chain, if a certificate doesn't pass
security checks, then chain may point to a freed STACK_OF(X509)

Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
2016-05-27 17:20:10 +01:00
Matt Caswell
753be41d59 Fix some suspect warnings on Windows
Windows was complaining about a unary minus operator being applied to an
unsigned type. It did seem to go on and do the right thing anyway, but the
code does look a little suspect. This fixes it.

Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
2016-05-26 17:18:39 +01:00
Matt Caswell
ea32151f7b The ssl3_digest_cached_records() function does not handle errors properly
The ssl3_digest_cached_records() function was failing to handle errors
that might be returned from EVP_DigestSignInit() and
EVP_DigestSignUpdate().

RT#4180

Reviewed-by: Stephen Henson <steve@openssl.org>
2016-05-26 15:47:33 +01:00
Rich Salz
0cd0a820ab Remove unused error/function codes.
Add script to find unused err/reason codes
Remove unused reason codes.
Remove entries for unused functions

Reviewed-by: Matt Caswell <matt@openssl.org>
2016-05-23 15:04:23 -04:00
Dr. Stephen Henson
77ab2b0193 remove encrypt then mac ifdefs
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-05-23 19:15:20 +01:00
FdaSilvaYY
a98810bfac Fix some malloc failure crashes on X509_STORE_CTX_set_ex_data
from BoringSSL 306ece31bcaaed49e0240a2e5555f8901ebb2d45

Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
2016-05-23 13:42:37 +01:00
Rich Salz
739a1eb196 Rename lh_xxx,sk_xxx tp OPENSSL_{LH,SK}_xxx
Rename sk_xxx to OPENSSL_sk_xxx and _STACK to OPENSSL_STACK
Rename lh_xxx API to OPENSSL_LH_xxx and LHASH_NODE to OPENSSL_LH_NODE
Make lhash stuff opaque.
Use typedefs for function pointers; makes the code simpler.
Remove CHECKED_xxx macros.
Add documentation; remove old X509-oriented doc.
Add API-compat names for entire old API

Reviewed-by: Dr. Stephen Henson <steve@openssl.org>
2016-05-20 10:48:29 -04:00
Matt Caswell
1689e7e688 Ensure async IO works with new state machine
In the new state machine if using nbio and we get the header of a
handshake message is one record with the body in the next, with an nbio
event in the middle, then the connection was failing. This is because
s->init_num was getting reset. We should only reset it after we have
read the whole message.

RT#4394

Reviewed-by: Andy Polyakov <appro@openssl.org>
2016-05-20 14:39:07 +01:00
David Benjamin
1257adecd4 Tighten up logic around ChangeCipherSpec.
ChangeCipherSpec messages have a defined value. They also may not occur
in the middle of a handshake message. The current logic will accept a
ChangeCipherSpec with value 2. It also would accept up to three bytes of
handshake data before the ChangeCipherSpec which it would discard
(because s->init_num gets reset).

Instead, require that s->init_num is 0 when a ChangeCipherSpec comes in.

RT#4391

Reviewed-by: Andy Polyakov <appro@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
2016-05-20 14:20:11 +01:00
Matt Caswell
464175692f Simplify SSL BIO buffering logic
The write BIO for handshake messages is bufferred so that we only write
out to the network when we have a complete flight. There was some
complexity in the buffering logic so that we switched buffering on and
off at various points through out the handshake. The only real reason to
do this was historically it complicated the state machine when you wanted
to flush because you had to traverse through the "flush" state (in order
to cope with NBIO). Where we knew up front that there was only going to
be one message in the flight we switched off buffering to avoid that.

In the new state machine there is no longer a need for a flush state so
it is simpler just to have buffering on for the whole handshake. This
also gives us the added benefit that we can simply call flush after every
flight even if it only has one message in it. This means that BIO authors
can implement their own buffering strategies and not have to be aware of
the state of the SSL object (previously they would have to switch off
their own buffering during the handshake because they could not rely on
a flush being received when they really needed to write data out). This
last point addresses GitHub Issue #322.

Reviewed-by: Andy Polyakov <appro@openssl.org>
2016-05-20 14:11:11 +01:00
Viktor Dukhovni
f75b34c8c8 When strict SCT fails record verification failure
Since with SSL_VERIFY_NONE, the connection may continue and the
session may even be cached, we should save some evidence that the
chain was not sufficiently verified and would have been rejected
with SSL_VERIFY_PEER.  To that end when a CT callback returs failure
we set the verify result to X509_V_ERR_NO_VALID_SCTS.

Note: We only run the CT callback in the first place if the verify
result is still X509_V_OK prior to start of the callback.

RT #4502

Reviewed-by: Tim Hudson <tjh@openssl.org>
2016-05-19 00:25:42 -04:00
Viktor Dukhovni
f3e235ed6f Ensure verify error is set when X509_verify_cert() fails
Set ctx->error = X509_V_ERR_OUT_OF_MEM when verificaiton cannot
continue due to malloc failure.  Also, when X509_verify_cert()
returns <= 0 make sure that the verification status does not remain
X509_V_OK, as a last resort set it it to X509_V_ERR_UNSPECIFIED,
just in case some code path returns an error without setting an
appropriate value of ctx->error.

Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-05-18 15:16:37 -04:00
Rich Salz
846e33c729 Copyright consolidation 01/10
Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Kurt Roeckx <kurt@openssl.org>
2016-05-17 14:19:19 -04:00
Matt Caswell
be9c8deb7d Add a comment to explain the use of |num_recs|
In the SSLV2ClientHello processing code in ssl3_get_record, the value of
|num_recs| will always be 0. This isn't obvious from the code so a comment
is added to explain it.

Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
2016-05-17 16:37:45 +01:00
Matt Caswell
de0717ebcc Use the current record offset in ssl3_get_record
The function ssl3_get_record() can obtain multiple records in one go
as long as we are set up for pipelining and all the records are app
data records. The logic in the while loop which reads in each record is
supposed to only continue looping if the last record we read was app data
and we have an app data record waiting in the buffer to be processed. It
was actually checking that the first record had app data and we have an
app data record waiting. This actually amounts to the same thing so wasn't
wrong - but it looks a bit odd because it uses the |rr| array without an
offset.

Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
2016-05-17 16:37:45 +01:00
Matt Caswell
6da5739215 There is only one read buffer
Pipelining introduced the concept of multiple records being read in one
go. Therefore we work with an array of SSL3_RECORD objects. The pipelining
change erroneously made a change in ssl3_get_record() to apply the current
record offset to the SSL3_BUFFER we are using for reading. This is wrong -
there is only ever one read buffer. This reverts that change. In practice
this should make little difference because the code block in question is
only ever used when we are processing a single record.

Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
2016-05-17 16:37:45 +01:00
Matt Caswell
d4d7894379 Fix some out of date comments
Fix various references to s3_clnt.c and s3_srvr.c which don't exist
any more.

GitHub Issue #765

Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-05-17 14:34:30 +01:00
Kurt Roeckx
d139723b0e session tickets: use more sizeof
Reviewed-by: Matt Caswell <matt@openssl.org>

MR: #2153
2016-05-16 20:43:20 +02:00
TJ Saunders
05df5c2036 Use AES256 for the default encryption algoritm for TLS session tickets
This involves providing more session ticket key data, for both the cipher and
the digest

Signed-off-by: Kurt Roeckx <kurt@roeckx.be>
Reviewed-by: Matt Caswell <matt@openssl.org>

GH: #515, MR: #2153
2016-05-16 20:43:06 +02:00
TJ Saunders
4e2e1ec9d5 session tickets: Use sizeof() for the various fields
Signed-off-by: Kurt Roeckx <kurt@roeckx.be>
Reviewed-by: Matt Caswell <matt@openssl.org>

GH: #515, MR: #2153
2016-05-16 20:42:21 +02:00
Viktor Dukhovni
5c4328f04f Fold threads.h into crypto.h making API public
Document thread-safe lock creation

Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-05-16 12:16:26 -04:00
Alessandro Ghedini
6546e9b221 Add SSL_client_version() getter function
Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Tim Hudson <tjh@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
2016-05-16 15:58:52 +01:00
jfigus
ba261f718b Propagate tlsext_status_type from SSL_CTX to SSL
To allow OCSP stapling to work with libcurl.

Github PR #200

Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
2016-05-16 14:42:30 +01:00
Kazuki Yamaguchi
b04f947941 Fix NPN protocol name list validation
Since 50932c4 "PACKETise ServerHello processing",
ssl_next_proto_validate() incorrectly allows empty protocol name.
draft-agl-tls-nextprotoneg-04[1] says "Implementations MUST ensure that
the empty string is not included and that no byte strings are
truncated."
This patch restores the old correct behavior.

[1] https://tools.ietf.org/html/draft-agl-tls-nextprotoneg-04

Reviewed-by: Emilia Käsper <emilia@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
2016-05-16 11:45:25 +01:00
FdaSilvaYY
c5ebfcab71 Unify <TYPE>_up_ref methods signature and behaviour.
Add a status return value instead of void.
Add some sanity checks on reference counter value.
Update the docs.

Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
2016-05-16 10:17:33 +01:00
Alessandro Ghedini
8a18bc2588 Increment size limit for ClientHello messages
The current limit of 2^14 bytes is too low (e.g. RFC 5246 specifies the
maximum size of just the extensions field to be 2^16-1), and may cause
bogus failures.

RT#4063

Reviewed-by: Kurt Roeckx <kurt@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/413)
2016-05-14 07:51:28 -04:00
David Benjamin
c45d6b2b0d The NewSessionTicket message is not optional.
Per RFC 4507, section 3.3:

   This message [NewSessionTicket] MUST be sent if the
   server included a SessionTicket extension in the ServerHello.  This
   message MUST NOT be sent if the server did not include a
   SessionTicket extension in the ServerHello.

The presence of the NewSessionTicket message should be determined
entirely from the ServerHello without probing.

RT#4389

Reviewed-by: Emilia Käsper <emilia@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
2016-05-13 13:04:46 +01:00
Dmitry Belyavsky
48c16012e7 Don't use GOST ciphersuites with DTLS.
RT#4438

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Stephen Henson <steve@openssl.org>
2016-05-12 12:02:38 +01:00
Dr. Stephen Henson
7c0ef84318 Don't leak memory if realloc fails.
RT#4403

Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
2016-05-12 12:02:38 +01:00
Matt Caswell
6e3ff63228 Make null_compression const
Reviewed-by: Stephen Henson <steve@openssl.org>
2016-05-11 13:43:41 +01:00
David Benjamin
cb21df3229 Fix V2ClientHello handling.
The V2ClientHello code creates an empty compression list, but the
compression list must explicitly contain the null compression (and later
code enforces this).

RT#4387

Reviewed-by: Stephen Henson <steve@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
2016-05-11 13:43:41 +01:00
Andy Polyakov
c21c7830ac IRIX fixes.
Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-05-10 08:44:51 +02:00
David Benjamin
f7aa318552 Don't send signature algorithms when client_version is below TLS 1.2.
Per RFC 5246,

    Note: this extension is not meaningful for TLS versions prior to 1.2.
    Clients MUST NOT offer it if they are offering prior versions.
    However, even if clients do offer it, the rules specified in [TLSEXT]
    require servers to ignore extensions they do not understand.

Although second sentence would suggest that there would be no interop
problems in always offering the extension, WebRTC has reported issues
with Bouncy Castle on < TLS 1.2 ClientHellos that still include
signature_algorithms. See also
https://bugs.chromium.org/p/webrtc/issues/detail?id=4223

RT#4390

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Stephen Henson <steve@openssl.org>
2016-05-09 17:46:23 +01:00
Matt Caswell
fbdf0299dc Free any existing SRTP connection profile
When setting a new SRTP connection profile using
SSL_CTX_set_tlsext_use_srtp() or SSL_set_tlsext_use_srtp() we should
free any existing profile first to avoid a memory leak.

Reviewed-by: Kurt Roeckx <kurt@openssl.org>
2016-05-09 10:25:34 +01:00
FdaSilvaYY
dccd20d1b5 fix tab-space mixed indentation
No code change

Reviewed-by: Kurt Roeckx <kurt@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
2016-05-09 09:09:55 +01:00
Rich Salz
4a8e9c22f4 Move 3DES from HIGH to MEDIUM
Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
2016-05-05 17:31:53 -04:00
Sergio Garcia Murillo
50b4a9ba13 GH356: Change assert to normal error
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-05-05 17:27:30 -04:00
Matt Caswell
fc7f190c73 Handle no async jobs in libssl
If the application has limited the size of the async pool using
ASYNC_init_thread() then we could run out of jobs while trying to start a
libssl io operation. However libssl was failing to handle this and treating
it like a fatal error. It should not be fatal...we just need to retry when
there are jobs available again.

Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-05-05 19:39:14 +01:00
Matt Caswell
485b78ddaa Improve heartbeats coding style
Based on an orignal commit by GitHub user BertramScharpf. Rebased and
updated to take account of all the updates since this was first raised.

GH PR#62

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-05-05 16:30:35 +01:00
TJ Saunders
5c4b8c6f61 Remove confusing comment.
Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-05-02 16:55:14 -04:00
TJ Saunders
5f18bc5898 Issue #719:
If no serverinfo extension is found in some cases, do not abort the handshake,
but simply omit/skip that extension.

Check for already-registered serverinfo callbacks during serverinfo
registration.

Update SSL_CTX_use_serverinfo() documentation to mention the need to reload the
same serverinfo per certificate, for servers with multiple server certificates.

Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-05-02 16:55:14 -04:00
Andy Polyakov
fbaf30d087 ssl/record/rec_layer_s3.c: fix typo from previous commit.
Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-05-02 15:23:22 +02:00
Andy Polyakov
b1a07c3854 Remove obsolete defined(__INTEL__) condition.
This macro was defined by no-longer-supported __MWERKS__ compiler.

Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-05-02 12:35:01 +02:00
Matt Caswell
5fd1478df3 Fix building with -DCHARSET_EBCDIC
Building with -DCHARSET_EBCDIC and using --strict-warnings resulted in
lots of miscellaneous errors. This fixes it.

Reviewed-by: Andy Polyakov <appro@openssl.org>
2016-04-29 15:04:15 +01:00
Matt Caswell
6f137370dd Client side CKE processing can double free on error
The tls_client_key_exchange_post_work() frees the pms on error. It also
calls ssl_generate_master_secret() which also free the pms. If an error
occurs after ssl_generate_master_secret() has been called then a double
free can occur.

Reviewed-by: Andy Polyakov <appro@openssl.org>
2016-04-29 09:17:51 +01:00
FdaSilvaYY
25a807bcb9 Add checks on CRYPTO_new_ex_data return value
Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/996)
2016-04-28 14:37:41 -04:00
FdaSilvaYY
8fdc99cb5d Fix an error code spelling.
Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/952)
2016-04-28 14:22:26 -04:00
FdaSilvaYY
8483a003bf various spelling fixes
Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/952)
2016-04-28 14:22:26 -04:00
Matt Caswell
b3bd3d5af8 Don't leak memory on error path in dane_ctx_enable()
The function dane_ctx_enable() allocated some memory that it did not
free in an error path.

Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-04-28 13:13:09 +01:00
Ben Laurie
5c001c326d Fix no-gost no-srp no-ec no-dh.
Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-04-27 12:36:30 +01:00
Ben Laurie
d94ce4100f Fix enable-ssl-trace no-nextprotoneg.
Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-04-27 12:36:30 +01:00
Matt Caswell
bfb155c187 split_send_fragment should always be less than or equal to max_send_fragment
A bug meant that SSL_CTRL_SET_MAX_SEND_FRAGMENT was not adjusting
split_send_fragment properly.

Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-04-27 09:22:40 +01:00
Matt Caswell
dbd5c34f18 Fix BIO_CTRL_DUP for an SSL BIO
The variables in the BIO weren't being duplicated properly.

Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
2016-04-27 09:20:11 +01:00
Matt Caswell
446ba8de9a Ensure we check i2d_X509 return val
The i2d_X509() function can return a negative value on error. Therefore
we should make sure we check it.

Issue reported by Yuan Jochen Kang.

Reviewed-by: Emilia Käsper <emilia@openssl.org>
2016-04-26 14:29:54 +01:00
Viktor Dukhovni
e2ab7fb343 make update
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-04-22 10:41:57 -04:00
Viktor Dukhovni
9f6b22b814 Enabled DANE only when at least one TLSA RR was added
It is up to the caller of SSL_dane_tlsa_add() to take appropriate
action when no records are added successfully or adding some records
triggers an internal error (negative return value).

With this change the caller can continue with PKIX if desired when
none of the TLSA records are usable, or take some appropriate action
if DANE is required.

Also fixed the internal ssl_dane_dup() function to properly initialize
the TLSA RR stack in the target SSL handle.  Errors in ssl_dane_dup()
are no longer ignored.

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-04-22 10:41:57 -04:00
Matt Caswell
ee85fc1dd6 Don't set peer_tmp until we have finished constructing it
If we fail halfway through constructing the peer_tmp EVP_PKEY but we have
already stored it in s->s3->peer_tmp then if anything tries to use it then
it will likely fail. This was causing s_client to core dump in the
sslskewith0p test. s_client was trying to print out the connection
parameters that it had negotiated so far. Arguably s_client should not do
that if the connection has failed...but given it is existing functionality
it's easier to fix libssl.

Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
2016-04-22 15:37:17 +01:00
Matt Caswell
5951e840d9 Fix no-ocsp on Windows (and probably VMS)
The ocsp.h file did not have appropriate guards causing link failures on
Windows.

GH Issue 900

Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-04-21 17:03:02 +01:00
Richard Levitte
45c6e23c97 Remove --classic build entirely
The Unix build was the last to retain the classic build scheme.  The
new unified scheme has matured enough, even though some details may
need polishing.

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-04-20 16:04:56 +02:00
Rich Salz
9021a5dfb3 Rename some lowercase API's
Make OBJ_name_cmp internal
Rename idea_xxx to IDEA_xxx
Rename get_rfc_xxx to BN_get_rfc_xxx
Rename v3_addr and v3_asid functions to X509v3_...

Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-04-18 08:22:00 -04:00
Rich Salz
f0e0fd51fd Make many X509_xxx types opaque.
Make X509_OBJECT, X509_STORE_CTX, X509_STORE, X509_LOOKUP,
and X509_LOOKUP_METHOD opaque.
Remove unused X509_CERT_FILE_CTX

Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Dr. Stephen Henson <steve@openssl.org>
2016-04-15 13:21:43 -04:00
Lyon Chen
4b6b848785 Add SSL_SESSION_get0_hostname()
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-04-14 08:41:29 -04:00
Matt Caswell
d064e6ab52 Remove OPENSSL_NO_SHA guards
no-sha is no longer an option so remove OPENSSL_NO_SHA guards.

Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-04-13 21:25:24 +01:00
Matt Caswell
5158c763f5 Remove OPENSSL_NO_AES guards
no-aes is no longer a Configure option and therefore the OPENSSL_NO_AES
guards can be removed.

Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-04-13 21:25:24 +01:00
Matt Caswell
b3599dbb6a Rename int_*() functions to *_int()
There is a preference for suffixes to indicate that a function is internal
rather than prefixes. Note: the suffix is only required to disambiguate
internal functions and public symbols with the same name (but different
case)

Reviewed-by: Tim Hudson <tjh@openssl.org>
Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-04-13 08:59:03 +01:00
Matt Caswell
342c21cd8b Rename lots of *_intern or *_internal function to int_*
There was a lot of naming inconsistency, so we try and standardise on
one form.

Reviewed-by: Tim Hudson <tjh@openssl.org>
Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-04-13 08:52:34 +01:00
Matt Caswell
03b0e73555 Deprecate SSL_COMP_free_compression_methods() and make it a no-op
SSL_COMP_free_compression_methods() should not be called expicitly - we
should leave auto-deinit to clean this up instead.

Reviewed-by: Tim Hudson <tjh@openssl.org>
Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-04-13 08:52:33 +01:00
Matt Caswell
6827cb3610 Deprecate ERR_free_strings() and make it a no-op
ERR_free_strings() should not be called expicitly - we should leave
auto-deinit to clean this up instead.

Reviewed-by: Tim Hudson <tjh@openssl.org>
Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-04-13 08:52:33 +01:00
Kazuki Yamaguchi
9d5ac9532c Add SSL_CTX_get_ciphers()
Add an accessor for SSL_CTX.

Since libssl was made opaque, there is no way for users to access the
cipher_list, while users can set the cipher_list by
SSL_CTX_set_cipher_list().

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-04-11 09:59:04 -04:00