Skip to content

Commit

Permalink
Fix status_request and SCT extensions
Browse files Browse the repository at this point in the history
They are valid for use in a CertificateRequest message, but we did not
allow it. If a server sent such a message using either of those two
extensions then the handshake would abort.

This corrects that error, but does not add support for actually processing
the extensions. They are simply ignored, and a TODO is inserted to add
support at a later time.

This was found during interoperability testing with btls:
https://gitlab.com/ilari_l/btls

Prompted by these errors I reviewed the complete list of extensions and
compared them with the latest table in draft-24 to confirm there were no
other errors of a similar type. I did not find any.

Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Ben Kaduk <kaduk@mit.edu>
(Merged from #5490)
  • Loading branch information
mattcaswell committed Mar 5, 2018
1 parent c04c602 commit 5de683d
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 2 deletions.
4 changes: 2 additions & 2 deletions ssl/statem/extensions.c
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ static const EXTENSION_DEFINITION ext_defs[] = {
{
TLSEXT_TYPE_status_request,
SSL_EXT_CLIENT_HELLO | SSL_EXT_TLS1_2_SERVER_HELLO
| SSL_EXT_TLS1_3_CERTIFICATE,
| SSL_EXT_TLS1_3_CERTIFICATE | SSL_EXT_TLS1_3_CERTIFICATE_REQUEST,
init_status_request, tls_parse_ctos_status_request,
tls_parse_stoc_status_request, tls_construct_stoc_status_request,
tls_construct_ctos_status_request, NULL
Expand Down Expand Up @@ -263,7 +263,7 @@ static const EXTENSION_DEFINITION ext_defs[] = {
{
TLSEXT_TYPE_signed_certificate_timestamp,
SSL_EXT_CLIENT_HELLO | SSL_EXT_TLS1_2_SERVER_HELLO
| SSL_EXT_TLS1_3_CERTIFICATE,
| SSL_EXT_TLS1_3_CERTIFICATE | SSL_EXT_TLS1_3_CERTIFICATE_REQUEST,
NULL,
/*
* No server side support for this, but can be provided by a custom
Expand Down
12 changes: 12 additions & 0 deletions ssl/statem/extensions_clnt.c
Original file line number Diff line number Diff line change
Expand Up @@ -1397,6 +1397,12 @@ int tls_parse_stoc_session_ticket(SSL *s, PACKET *pkt, unsigned int context,
int tls_parse_stoc_status_request(SSL *s, PACKET *pkt, unsigned int context,
X509 *x, size_t chainidx)
{
if (context == SSL_EXT_TLS1_3_CERTIFICATE_REQUEST) {
/* We ignore this if the server sends a CertificateRequest */
/* TODO(TLS1.3): Add support for this */
return 1;
}

/*
* MUST only be sent if we've requested a status
* request message. In TLS <= 1.2 it must also be empty.
Expand Down Expand Up @@ -1435,6 +1441,12 @@ int tls_parse_stoc_status_request(SSL *s, PACKET *pkt, unsigned int context,
int tls_parse_stoc_sct(SSL *s, PACKET *pkt, unsigned int context, X509 *x,
size_t chainidx)
{
if (context == SSL_EXT_TLS1_3_CERTIFICATE_REQUEST) {
/* We ignore this if the server sends it in a CertificateRequest */
/* TODO(TLS1.3): Add support for this */
return 1;
}

/*
* Only take it if we asked for it - i.e if there is no CT validation
* callback set, then a custom extension MAY be processing it, so we
Expand Down

0 comments on commit 5de683d

Please sign in to comment.