Fix key share negotiation in HRR case

In the ClientHello retrying the handshake after a HelloRetryRequest, the
client must send a single key share matching the group selected by the
server in the HR

Fix key share negotiation in HRR case

In the ClientHello retrying the handshake after a HelloRetryRequest, the
client must send a single key share matching the group selected by the
server in the HRR. This is not necessarily the mutually preferred group.
Incorrect logic added in ssl_tlsect.c r1.134 would potentially reject
such a key share because of that.

Instead, add logic to ensure on the server side that there is a single
share matching the group we selected in the HRR.

Fixes a regress test in p5-IO-Socket-SSL where server is configured
with P-521:P-384 and the client with P-256:P-384:P-521. Since the
client sends an initial P-256 key share, a HRR is triggered which
the faulty logic rejected because it was not the mutually preferred
P-384 but rather matching the server-selected P-521.

This will need some deduplication in subsequent commits. We may also
want to consider honoring the mutual preference and request a key
accordingly in the HRR.

reported by bluhm, fix suggested by jsing
ok beck jsing

show more ...

Recommit a better version of the removal of the F5 workaround

Unlike for previous TLS versions, TLSv1.3 servers can send the supported
groups extension to inform a client of the server's preferences

Recommit a better version of the removal of the F5 workaround

Unlike for previous TLS versions, TLSv1.3 servers can send the supported
groups extension to inform a client of the server's preferences. The
intention is that a client can adapt for subsequent commits. We ignore
this info for now, but sthen ran into java-based servers that do this.

Thus, rejecting the extension outright was incorrect. Instead, only allow
the extension in TLSv1.3 encrypted extensions. This way the F5 workaround
is also disabled, but we continue to interoperate with TLSv1.3 servers that
do follow the last paragraph of RFC 8446, section 4.2.7.

This mostly adjusts outdated/misleading comments.

ok jsing sthen

show more ...

Backout previous commit (intending that libressl client rejects a supported
groups extension from the server). It triggers 'CONNECT_CR_SRVR_HELLO:tlsv1
alert decode error' when connecting to a (moder

Backout previous commit (intending that libressl client rejects a supported
groups extension from the server). It triggers 'CONNECT_CR_SRVR_HELLO:tlsv1
alert decode error' when connecting to a (modern) java server (tomcat 10.1.18
on openjdk 17.0.10).

"please revert" tb@

show more ...

Stop pandering to the loadbalancer industrial complex.

So we initially kept this hack around for f5 boxes that
should have been patched in 2014, and were not as of 2017.

The f5 article for the bug

Stop pandering to the loadbalancer industrial complex.

So we initially kept this hack around for f5 boxes that
should have been patched in 2014, and were not as of 2017.

The f5 article for the bug archived on their web site,
and any of these devices on the public internet will have
since been upgraded to deal with a host of record layer, TLS,
and other bugs, or they likely won't be talking to modern
stacks, since as of this point the software with the bug
would not have been updated in 10 years.

So just make this spec compliant and reject a supported groups
extension that should not have been sent by a server.

ok tb@ jsing@

show more ...

Fix up server processing of key shares.

Ensure that the client can not provide a duplicate key share
for any group, or send more key shares than groups they support.

Ensure that the key shares must

Fix up server processing of key shares.

Ensure that the client can not provide a duplicate key share
for any group, or send more key shares than groups they support.

Ensure that the key shares must be provided in the same order
as the client preference order specified in supported_groups.

Ensure we only will choose to use a key share that is for the
most preferred group by the client that we also support,
to avoid the client being downgraded by sending a less preferred
key share. If we do not end up with a key share for the most preferred
mutually supported group, will then do a hello retry request
selecting that group.

Add regress for this to regress/tlsext/tlsexttest.c

ok jsing@

show more ...

Do not allow duplicate groups in supported groups.

While we are here refactor this to single return.

ok jsing@ tb@

Add an indicator that an extension has been processed.

ok jsing@

Process supported groups before key share.

This will allow us to know the client preferences for an upcoming
change to key share processing.

ok jsing@

Simplify TLS extension parsing and processing.

Rather than having a separate parse and process step for each TLS
extension, do a first pass that parses all of the TLS outer extensions and
retains th

Simplify TLS extension parsing and processing.

Rather than having a separate parse and process step for each TLS
extension, do a first pass that parses all of the TLS outer extensions and
retains the extension data, before running a second pass that calls the TLS
extension processing code.

ok beck@ tb@

show more ...

Fix typo msg_types -> msg_type

from jsing

Split TLS extension parsing from processing.

The TLS extension parsing and processing order is currently dependent on
the order of the extensions in the handshake message. This means that the
proces

Split TLS extension parsing from processing.

The TLS extension parsing and processing order is currently dependent on
the order of the extensions in the handshake message. This means that the
processing order (and callback order) is not under our control. Split the
parsing from the processing such that the processing (and callbacks) are
run in a defined order.

Convert ALPN to the new model - other extensions will be split into
separate parse/process in following diffs.

ok beck@ tb@

show more ...

Decouple TLS extension table order from tlsext_randomize_build_order()

The PSK extension must be the last extension in the client hello. This is
currently implemented by relying on the fact that it

Decouple TLS extension table order from tlsext_randomize_build_order()

The PSK extension must be the last extension in the client hello. This is
currently implemented by relying on the fact that it is the last extension
in the TLS extension table. Remove this dependency so that we can reorder
the table as needed.

ok tb@

show more ...

Too many stupid things whine about these being used uninitialized
(which they aren't), so appease them.

ssl_tlsext.c: Add an accessor for the tls extension type.

Needed for the tlsexttest.c

ok jsing

Fix allocation size

Reported by anton

Free and calloc() the tlsext_build_order and remember its length

Aligns tlsext_randomize_build_order() with tlsext_linearize_build_order()
and will help regression testing.

ok jsing

Use TLSEXT_TYPE_alpn instead of the stupid long one

Randomize the order of TLS extensions

On creation of an SSL using SSL_new(), randomize the order in which the
extensions will be sent. There are several constraints: the PSK extension
must always co

Randomize the order of TLS extensions

On creation of an SSL using SSL_new(), randomize the order in which the
extensions will be sent. There are several constraints: the PSK extension
must always come last. The order cannot be randomized on a per-message
basis as the strict interpretation of the standard chosen in the CH hashing
doesn't allow changing the order between first and second ClientHello.

Another constraint is that the current code calls callbacks directly on
parsing an extension, which means that the order callbacks are called
depends on the order in which the peer sent the extensions. This results
in breaking apache-httpd setups using virtual hosts with full ranomization
because virtual hosts don't work if the SNI is unknown at the time the
ALPN callback is called. So for the time being, we ensure that SNI always
precedes ALPN to avoid issues until this issue is fixed.

This is based on an idea by David Benjamin
https://boringssl-review.googlesource.com/c/boringssl/+/48045

Input & ok jsing

show more ...

Make internal header file names consistent

Libcrypto currently has a mess of *_lcl.h, *_locl.h, and *_local.h names
used for internal headers. Move all these headers we inherited from
OpenSSL to *_l

Make internal header file names consistent

Libcrypto currently has a mess of *_lcl.h, *_locl.h, and *_local.h names
used for internal headers. Move all these headers we inherited from
OpenSSL to *_local.h, reserving the name *_internal.h for our own code.
Similarly, move dtls_locl.h and ssl_locl.h to dtls_local and ssl_local.h.
constant_time_locl.h is moved to constant_time.h since it's special.

Adjust all .c files in libcrypto, libssl and regress.

The diff is mechanical with the exception of tls13_quic.c, where
#include <ssl_locl.h> was fixed manually.

discussed with jsing,
no objection bcook

show more ...

Get rid of SSL_CTX_INTERNAL and SSL_INTERNAL.

These are no longer necessary due to SSL_CTX and SSL now being fully
opaque. Merge SSL_CTX_INTERNAL back into SSL_CTX and SSL_INTERNAL back
into SSL.

P

Get rid of SSL_CTX_INTERNAL and SSL_INTERNAL.

These are no longer necessary due to SSL_CTX and SSL now being fully
opaque. Merge SSL_CTX_INTERNAL back into SSL_CTX and SSL_INTERNAL back
into SSL.

Prompted by tb@

show more ...

Avoid shadowing the cbs function parameter in tlsext_alpn_server_parse()

ok jsing

Make tlsext_*_{build,needs,parse}() functions static

None of these functions are used outside of ssl_tlsext.c. The only reason
they are prototyped in the header is for the use of tlsexttest.c. Rathe

Make tlsext_*_{build,needs,parse}() functions static

None of these functions are used outside of ssl_tlsext.c. The only reason
they are prototyped in the header is for the use of tlsexttest.c. Rather
than having a big pile of useless copy-paste in the header, we can adapt
the test to avoid using these functions directly.

ok jsing

show more ...

Rely on tlsext_parse() to set a decode_error alert

Instead of setting the alert manually in various parse handlers, we can
make use of the fact that tlsext_parse() sets the alert to decode_error
by

Rely on tlsext_parse() to set a decode_error alert

Instead of setting the alert manually in various parse handlers, we can
make use of the fact that tlsext_parse() sets the alert to decode_error
by default. This simplifies the code quite a bit.

ok jsing

show more ...

Remove redundant length checks in parse functions

The main parsing function already checks that the entire extension data
was consumed, so the length checks inside some of the parse handlers are
red

Remove redundant length checks in parse functions

The main parsing function already checks that the entire extension data
was consumed, so the length checks inside some of the parse handlers are
redundant. They were also not done everywhere, so this makes the parse
handlers more consistent.

Similar diff was sent by jsing a long while back

ok jsing

show more ...

Simplify tlsext_supported_groups_server_parse

Add an early return in the s->internal->hit case so that we can unindent
a lot of this code. In the HRR case, we do not need to check that the list
of s

Simplify tlsext_supported_groups_server_parse

Add an early return in the s->internal->hit case so that we can unindent
a lot of this code. In the HRR case, we do not need to check that the list
of supported groups is unmodified from the first CH. The CH extension
hashing already does that for us.

ok jsing

show more ...