Commit graph

188 commits

Author SHA1 Message Date
Matt Caswell
4bf086005f Fix a leak in SSL_clear()
SSL_clear() was resetting numwpipes to 0, but not freeing any allocated
memory for existing write buffers.

Fixes #2026

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-12-12 13:12:25 +00:00
Matt Caswell
44e58f3b7b Change various repeated wr[someindex]/pkt[someindex] references to a pointer
Improves the readability of the code, and reduces the liklihood of errors.
Also made a few minor style changes.

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-12-05 17:05:40 +00:00
Matt Caswell
829754a622 Various style fixes from the TLSv1.3 record changes review
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-12-05 17:05:40 +00:00
Matt Caswell
88858868ab Change various repeated rr[someindex] references to a pointer
Improves the readability of the code, and reduces the liklihood of errors.

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-12-05 17:05:40 +00:00
Matt Caswell
e8eb224b8c Ensure compressdata is always initialised
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-12-05 17:05:40 +00:00
Matt Caswell
f31d5e1005 Add a TLS1.3 TODO for the msg callback
At the moment the msg callback only received the record header with the
outer record type in it. We never pass the inner record type - we probably
need to at some point.

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-12-05 17:05:40 +00:00
Matt Caswell
e60ce9c451 Update the record layer to use TLSv1.3 style record construction
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-12-05 17:05:40 +00:00
Matt Caswell
6a149cee78 Convert TLS Record receipt to use PACKET
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-12-05 17:05:40 +00:00
Matt Caswell
c7c42022b9 Convert TLS record construction to use WPACKET
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-12-05 17:05:40 +00:00
Matt Caswell
6606d60054 Fix some style issues in the TLSv1.3 nonce construction code
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-11-29 23:31:10 +00:00
Matt Caswell
bebc0c7d85 Use the TLSv1.3 nonce construction
This updates the record layer to use the TLSv1.3 style nonce construciton.
It also updates TLSProxy and ossltest to be able to recognise the new
layout.

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-11-29 23:31:10 +00:00
Kurt Roeckx
beacb0f0c1 Make SSL_read and SSL_write return the old behaviour and document it.
This reverts commit 4880672a9b.

Fixes: #1903

Reviewed-by: Matt Caswell <matt@openssl.org>

GH: #1931
2016-11-21 21:54:28 +01:00
Matt Caswell
657a43f662 Fix missing SSL_IS_TLS13(s) usage
We should use the macro for testing if we are using TLSv1.3 rather than
checking s->version directly.

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-11-17 11:03:16 +00:00
Richard Levitte
e72040c1dc Remove heartbeat support
Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Tim Hudson <tjh@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/1669)
2016-11-13 16:24:02 -05:00
Matt Caswell
3c9539d294 Ignore the record version in TLS1.3
The record layer version field must be ignored in TLSv1.3, so we remove the
check when using that version.

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-11-07 15:52:33 +00:00
Matt Caswell
ff04799d90 Fix some style issues from libssl size_tify review
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-11-04 12:09:46 +00:00
Matt Caswell
54105ddd23 Rename all "read" variables with "readbytes"
Travis is reporting one file at a time shadowed variable warnings where
"read" has been used. This attempts to go through all of libssl and replace
"read" with "readbytes" to fix all the problems in one go.

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-11-04 12:09:46 +00:00
Matt Caswell
02ba18a63e Fix a shadowed variable declaration warning picked up by Travis
Rename "read" to "readbytes"

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-11-04 12:09:46 +00:00
Matt Caswell
2b7363ecf1 Ensure SSL_DEBUG works following size_t changes
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-11-04 12:09:46 +00:00
Matt Caswell
f0ca8f89f8 Fix some bogus warnings about uninitialised variables
Travis was failing in some builds due to a bogus complaint
about uninit variables.

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-11-04 12:09:46 +00:00
Matt Caswell
699ae85915 Remove a stray TODO that has already been fixed
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-11-04 12:09:46 +00:00
Matt Caswell
2688e7a0be Provide some constant time functions for dealing with size_t values
Also implement the using of them

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-11-04 12:09:46 +00:00
Matt Caswell
348240c676 Fix misc size_t issues causing Windows warnings in 64 bit
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-11-04 12:09:46 +00:00
Matt Caswell
a14aa99be8 Convert the mac functions to just return 1 for success and 0 for failure
Previously they return -1 for failure or the size of the mac. But the size
was never used anywhere.

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-11-04 12:09:46 +00:00
Matt Caswell
c08d12ca40 Fix some ssl3_record code witch converstion to/from size_t
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-11-04 12:09:46 +00:00
Matt Caswell
8b0e934afb Fix some missed size_t updates
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-11-04 12:09:45 +00:00
Matt Caswell
e3c9727fec Resolve some outstanding size_t related TODOs
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-11-04 12:09:45 +00:00
Matt Caswell
72716e79bf Convert some misc record layer functions for size_t
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-11-04 12:09:45 +00:00
Matt Caswell
5607b2759a Convert SSL3_RECORD_clear() and SSL3_RECORD_release() to size_t
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-11-04 12:09:45 +00:00
Matt Caswell
7ee8627f6e Convert libssl writing for size_t
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-11-04 12:09:45 +00:00
Matt Caswell
eda757514e Further libssl size_t-ify of reading
Writing still to be done

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-11-04 12:09:45 +00:00
Matt Caswell
8e6d03cac4 Convert record layer to use size_t
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-11-04 12:09:45 +00:00
Matt Caswell
436a2a0179 Fail if an unrecognised record type is received
TLS1.0 and TLS1.1 say you SHOULD ignore unrecognised record types, but
TLS 1.2 says you MUST send an unexpected message alert. We swap to the
TLS 1.2 behaviour for all protocol versions to prevent issues where no
progress is being made and the peer continually sends unrecognised record
types, using up resources processing them.

Issue reported by 郭志攀

Reviewed-by: Tim Hudson <tjh@openssl.org>
2016-11-02 23:22:48 +00:00
Matt Caswell
a7faa6da31 Fix read_ahead
The function ssl3_read_n() takes a parameter |clearold| which, if set,
causes any old data in the read buffer to be forgotten, and any unread data
to be moved to the start of the buffer. This is supposed to happen when we
first read the record header.

However, the data move was only taking place if there was not already
sufficient data in the buffer to satisfy the request. If read_ahead is set
then the record header could be in the buffer already from when we read the
preceding record. So with read_ahead we can get into a situation where even
though |clearold| is set, the data does not get moved to the start of the
read buffer when we read the record header. This means there is insufficient
room in the read buffer to consume the rest of the record body, resulting in
an internal error.

This commit moves the |clearold| processing to earlier in ssl3_read_n()
to ensure that it always takes place.

Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-11-02 16:47:14 +00:00
Matt Caswell
4880672a9b A zero return from BIO_read()/BIO_write() could be retryable
A zero return from BIO_read()/BIO_write() could mean that an IO operation
is retryable. A zero return from SSL_read()/SSL_write() means that the
connection has been closed down (either cleanly or not). Therefore we
should not propagate a zero return value from BIO_read()/BIO_write() back
up the stack to SSL_read()/SSL_write(). This could result in a retryable
failure being treated as fatal.

Reviewed-by: Richard Levitte <levitte@openssl.org>
2016-10-28 09:13:49 +01:00
David Woodhouse
e23d5071ec Fix encrypt-then-mac implementation for DTLS
OpenSSL 1.1.0 will negotiate EtM on DTLS but will then not actually *do* it.

If we use DTLSv1.2 that will hopefully be harmless since we'll tend to use
an AEAD ciphersuite anyway. But if we're using DTLSv1, then we certainly
will end up using CBC, so EtM is relevant — and we fail to interoperate with
anything that implements EtM correctly.

Fixing it in HEAD and 1.1.0c will mean that 1.1.0[ab] are incompatible with
1.1.0c+... for the limited case of non-AEAD ciphers, where they're *already*
incompatible with other implementations due to this bug anyway. That seems
reasonable enough, so let's do it. The only alternative is just to turn it
off for ever... which *still* leaves 1.0.0[ab] failing to communicate with
non-OpenSSL implementations anyway.

Tested against itself as well as against GnuTLS both with and without EtM.

Reviewed-by: Tim Hudson <tjh@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
2016-10-17 23:17:39 +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
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
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
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
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
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
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
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
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