Commit graph

2757 commits

Author SHA1 Message Date
David Benjamin
609b0852e4 Remove trailing whitespace from some files.
The prevailing style seems to not have trailing whitespace, but a few
lines do. This is mostly in the perlasm files, but a few C files got
them after the reformat. This is the result of:

  find . -name '*.pl' | xargs sed -E -i '' -e 's/( |'$'\t'')*$//'
  find . -name '*.c' | xargs sed -E -i '' -e 's/( |'$'\t'')*$//'
  find . -name '*.h' | xargs sed -E -i '' -e 's/( |'$'\t'')*$//'

Then bn_prime.h was excluded since this is a generated file.

Note mkerr.pl has some changes in a heredoc for some help output, but
other lines there lack trailing whitespace too.

Reviewed-by: Kurt Roeckx <kurt@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
2016-10-10 23:36:21 +01:00
Matt Caswell
b90506e995 Fix linebreaks in the tls_construct_client_certificate function
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-10-03 16:25:48 +01:00
Matt Caswell
a15c953f77 Add a typedef for the construction function
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-10-03 16:25:48 +01:00
Matt Caswell
6392fb8e2a Move setting of the handshake header up one more level
We now set the handshake header, and close the packet directly in the
write_state_machine. This is now possible because it is common for all
messages.

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-10-03 16:25:48 +01:00
Matt Caswell
229185e668 Remove the special case processing for finished construction
tls_construct_finished() used to have different arguments to all of the
other construction functions. It doesn't anymore, so there is no neeed to
treat it as a special case.

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-10-03 16:25:48 +01:00
Matt Caswell
4a01c59f36 Harmonise setting the header and closing construction
Ensure all message types work the same way including CCS so that the state
machine doesn't need to know about special cases. Put all the special logic
into ssl_set_handshake_header() and ssl_close_construct_packet().

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-10-03 16:25:48 +01:00
Matt Caswell
5923ad4bbe Don't set the handshake header in every message
Move setting the handshake header up a level into the state machine code
in order to reduce boilerplate.

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-10-03 16:25:48 +01:00
Matt Caswell
7cea05dcc7 Move init of the WPACKET into write_state_machine()
Instead of initialising, finishing and cleaning up the WPACKET in every
message construction function, we should do it once in
write_state_machine().

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-10-03 16:25:48 +01:00
Matt Caswell
a29fa98ceb Rename ssl_set_handshake_header2()
ssl_set_handshake_header2() was only ever a temporary name while we had
to have ssl_set_handshake_header() for code that hadn't been converted to
WPACKET yet. No code remains that needed that so we can rename it.

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-10-02 20:25:57 +01:00
Matt Caswell
e2726ce64d Remove ssl_set_handshake_header()
Remove the old ssl_set_handshake_header() implementations. Later we will
rename ssl_set_handshake_header2() to ssl_set_handshake_header().

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-10-02 20:25:57 +01:00
Matt Caswell
42cde22f48 Remove the tls12_get_sigandhash_old() function
This is no longer needed now that all messages use WPACKET

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-10-02 20:25:57 +01:00
Dr. Stephen Henson
bcaad8094e fix memory leak
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-10-02 15:59:26 +01:00
Matt Caswell
a00d75e1b2 Convert NewSessionTicket construction to WPACKET
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-09-29 18:00:37 +01:00
Matt Caswell
b36017fe5f Fix an error in packet_locl.h
A convenience macro was using the wrong underlying function.

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-09-29 18:00:01 +01:00
Matt Caswell
cc59ad1073 Convert CertStatus message construction to WPACKET
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-09-29 17:07:45 +01:00
Matt Caswell
f308416e27 Fix mis-named macro in packet_locl.h
A couple of the WPACKET_sub_memcpy* macros were mis-named.

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-09-29 17:07:45 +01:00
Matt Caswell
4346a8faa7 Convert SeverDone construction to WPACKET
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-09-29 17:07:45 +01:00
Matt Caswell
83ae466131 Fix missing NULL checks in NewSessionTicket construction
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-09-29 16:15:16 +01:00
Matt Caswell
e4e1aa903e Fix an mis-matched function code so that "make update" doesn't fail
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-09-29 15:32:35 +01:00
Matt Caswell
0023baffb8 Add an example of usage to the WPACKET_reserve_bytes() documentation
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-09-29 15:09:02 +01:00
Matt Caswell
ff8194774c Address style feedback comments
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-09-29 15:09:02 +01:00
Matt Caswell
4a424545c4 Fix a bug in CKE construction for PSK
In plain PSK we don't need to do anymore construction after the preamble.
We weren't detecting this case and treating it as an unknown cipher.

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-09-29 15:09:02 +01:00
Matt Caswell
c13d2a5be7 Convert ServerKeyExchange construction to WPACKET
Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-09-29 15:09:02 +01:00
Matt Caswell
1ff8434040 Add the WPACKET_reserve_bytes() function
WPACKET_allocate_bytes() requires you to know the size of the data you
are allocating for, before you create it. Sometimes this isn't the case,
for example we know the maximum size that a signature will be before we
create it, but not the actual size. WPACKET_reserve_bytes() enables us to
reserve bytes in the WPACKET, but not count them as written yet. We then
subsequently need to acall WPACKET_allocate_bytes to actually count them as
written.

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-09-29 15:09:02 +01:00
Matt Caswell
ac8cc3efb2 Remove tls12_copy_sigalgs_old()
This was a temporary function needed during the conversion to WPACKET. All
callers have now been converted to the new way of doing this so this
function is no longer required.

Reviewed-by: Rich Salz <rsalz@openssl.org>
2016-09-29 14:52:55 +01:00
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