Try to be more consistent about the alerts we send #3480

mattcaswell · 2017-05-16T16:36:10Z

We are quite inconsistent about which alerts get sent. Specifically, these
alerts should be used (normally) in the following circumstances:

SSL_AD_DECODE_ERROR = The peer sent a syntactically incorrect message
SSL_AD_ILLEGAL_PARAMETER = The peer sent a message which was syntactically
correct, but a parameter given is invalid for the context
SSL_AD_HANDSHAKE_FAILURE = The peer's messages were syntactically and
semantically correct, but the parameters provided were unacceptable to us
(e.g. because we do not support the requested parameters)
SSL_AD_INTERNAL_ERROR = We messed up (e.g. malloc failure)

The standards themselves aren't always consistent but I think the above
represents the best interpretation.

We are quite inconsistent about which alerts get sent. Specifically, these alerts should be used (normally) in the following circumstances: SSL_AD_DECODE_ERROR = The peer sent a syntactically incorrect message SSL_AD_ILLEGAL_PARAMETER = The peer sent a message which was syntactically correct, but a parameter given is invalid for the context SSL_AD_HANDSHAKE_FAILURE = The peer's messages were syntactically and semantically correct, but the parameters provided were unacceptable to us (e.g. because we do not support the requested parameters) SSL_AD_INTERNAL_ERROR = We messed up (e.g. malloc failure) The standards themselves aren't always consistent but I think the above represents the best interpretation.

richsalz

A note in CHANGES pointing to the commit description for details?

kaduk

Generally it looks good, and thank you for doing it!
A few comments inline.

kaduk · 2017-05-17T03:55:04Z

ssl/statem/extensions.c

@@ -1078,7 +1078,7 @@ static int final_key_share(SSL *s, unsigned int context, int sent, int *al)
            && (!s->hit
                || (s->ext.psk_kex_mode & TLSEXT_KEX_MODE_FLAG_KE) == 0)) {
        /* Nothing left we can do - just fail */
-        *al = SSL_AD_HANDSHAKE_FAILURE;
+        *al = SSL_AD_MISSING_EXTENSION;


Am I remembering correctly that sent is tracking the local state, i.e., the client did not send a key_share here?
If so, then this seems like arguably an internal error. Or are we considering some edge cases like where we send a ClientHello with no key_share but both valid psk_key_exchange_modes, and the server picks psk_dhe_ke but doesn't send a HelloRetryRequest?

No, the "final" functions are called at the end of the parsing stage. So, "sent" here is true if the extension was sent to us by the peer.

kaduk · 2017-05-17T03:58:16Z

ssl/statem/extensions_clnt.c

@@ -946,7 +946,7 @@ int tls_parse_stoc_renegotiate(SSL *s, PACKET *pkt, unsigned int context,
                  s->s3->previous_client_finished_len) != 0) {
        SSLerr(SSL_F_TLS_PARSE_STOC_RENEGOTIATE,
               SSL_R_RENEGOTIATION_MISMATCH);
-        *al = SSL_AD_HANDSHAKE_FAILURE;
+        *al = SSL_AD_ILLEGAL_PARAMETER;


I could see someone make a case that these two should remain as HANDSHAKE_FAILURE to avoid giving information to an attacker on what went wrong, but I am not going to make that case -- this seems fine to me.

kaduk · 2017-05-17T04:01:35Z

ssl/statem/extensions_srvr.c

@@ -154,7 +154,7 @@ int tls_parse_ctos_srp(SSL *s, PACKET *pkt, unsigned int context, X509 *x,
     * upon resumption. Instead, we MUST ignore the login.
     */
    if (!PACKET_strndup(&srp_I, &s->srp_ctx.login)) {
-        *al = TLS1_AD_INTERNAL_ERROR;
+        *al = SSL_AD_INTERNAL_ERROR;


Why the many changes from TLS1_AD_FOO to SSL_AD_FOO?

For consistency. All the alert codes have a protocol specific version indicating the protocol in which they were first defined, e.g. TLS1_AD_INTERNAL_ERROR, TLS1_AD_UNKNOWN_CA, SSL3_AD_BAD_CERTIFICATE etc, and a non-protocol specific version, e.g. SSL_AD_INTERNAL_ERROR, SSL_AD_UNKNOWN_CA, SSL_AD_BAD_CERTIFICATE. There is also a protocol specific translation function (e.g. see ssl3_alert_code()) which translates any given alert code into one specific to that protocol version. Mostly this just returns what you pass in, except in the case where a specific alert does not exist in that protocol version. If the alert code is not defined in that protocol version then it translates it into some other appropriate alert code that is defined.

I believe the intention is that we should always use the non-protocol specific version. At least that's what we use pretty much exclusively at the moment with the exception of the few items that I am changing here. I'm just trying to make it consistent.

Probably we don't really need the protocol specific defines - but because they are in public header files it is difficult to get rid of them.

kaduk · 2017-05-17T04:04:19Z

ssl/statem/statem_clnt.c

@@ -1183,7 +1183,6 @@ int tls_construct_client_hello(SSL *s, WPACKET *pkt)

    /* TLS extensions */
    if (!tls_construct_extensions(s, pkt, SSL_EXT_CLIENT_HELLO, NULL, 0, &al)) {
-        ssl3_send_alert(s, SSL3_AL_FATAL, al);


Why drop sending the alert?

This is in the construction of the initial ClientHello - i.e. before we've started any kind of handshake at all. It would be a bit weird for a client to open a connection to a server, just send an alert and then close down again. The alert really signifies the end of a TLS connection - but we haven't started one at this point. I removed the alert here for consistency with the rest of this function. None of the other construction errors in this function result in an alert being sent - so why do that for the extension construction step?

Okay, thanks for the explanation. (I guess I forgot that tls_construct_extensions() has lots of call sites.)

kaduk · 2017-05-17T04:06:09Z

ssl/statem/statem_dtls.c

@@ -770,7 +770,7 @@ static int dtls_get_reassembled_message(SSL *s, int *errtype, size_t *len)
     * Fragments must not span records.
     */
    if (frag_len > RECORD_LAYER_get_rrec_length(&s->rlayer)) {
-        al = SSL3_AD_ILLEGAL_PARAMETER;
+        al = SSL_AD_ILLEGAL_PARAMETER;


Ditto with the TLS_ case mentioned above.
(I assume to standardize on a single spelling, but I didn't see it mentioned in the commit message.)

kaduk · 2017-05-17T04:09:27Z

ssl/statem/statem_srvr.c

@@ -1318,7 +1318,7 @@ MSG_PROCESS_RETURN tls_process_client_hello(SSL *s, PACKET *pkt)
        }

        if (session_id_len > SSL_MAX_SSL_SESSION_ID_LENGTH) {
-            al = SSL_AD_DECODE_ERROR;
+            al = SSL_AD_ILLEGAL_PARAMETER;


I could see this staying as DECODE_ERROR.

It's probably debatable. The way I saw it we have a syntactically correct ciphersuite_len, session_id_len and challenge_len, i.e. we have successfully parsed the data structure. However on inspection of the data we find a parameter that has a value that should not be allowed, so it's an illegal parameter.

Fair enough.

kaduk · 2017-05-17T04:11:27Z

ssl/statem/statem_srvr.c

@@ -3621,7 +3625,6 @@ static int tls_construct_encrypted_extensions(SSL *s, WPACKET *pkt)
                                  NULL, 0, &al)) {
        ssl3_send_alert(s, SSL3_AL_FATAL, al);
        SSLerr(SSL_F_TLS_CONSTRUCT_ENCRYPTED_EXTENSIONS, ERR_R_INTERNAL_ERROR);
-        ssl3_send_alert(s, SSL3_AL_FATAL, al);


Good catch!

mattcaswell · 2017-05-18T16:26:02Z

A note in CHANGES pointing to the commit description for details?

I'm not really sure a note in CHANGES is justified. Almost all of these will never be encountered by well behaved implementations. It's unlikely anyone is going to notice the difference.

We are quite inconsistent about which alerts get sent. Specifically, these alerts should be used (normally) in the following circumstances: SSL_AD_DECODE_ERROR = The peer sent a syntactically incorrect message SSL_AD_ILLEGAL_PARAMETER = The peer sent a message which was syntactically correct, but a parameter given is invalid for the context SSL_AD_HANDSHAKE_FAILURE = The peer's messages were syntactically and semantically correct, but the parameters provided were unacceptable to us (e.g. because we do not support the requested parameters) SSL_AD_INTERNAL_ERROR = We messed up (e.g. malloc failure) The standards themselves aren't always consistent but I think the above represents the best interpretation. Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #3480)

mattcaswell · 2017-05-19T07:52:46Z

Pushed. Thanks.

mattcaswell added the branch: master Merge to master branch label May 16, 2017

richsalz approved these changes May 16, 2017

View reviewed changes

kaduk reviewed May 17, 2017

View reviewed changes

mattcaswell closed this May 19, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Try to be more consistent about the alerts we send #3480

Try to be more consistent about the alerts we send #3480

mattcaswell commented May 16, 2017

richsalz left a comment

kaduk left a comment

kaduk May 17, 2017

mattcaswell May 17, 2017

kaduk May 17, 2017

kaduk May 17, 2017

mattcaswell May 17, 2017

kaduk May 17, 2017

mattcaswell May 17, 2017

kaduk May 17, 2017

kaduk May 17, 2017

kaduk May 17, 2017

mattcaswell May 17, 2017

kaduk May 17, 2017

kaduk May 17, 2017

mattcaswell commented May 18, 2017

mattcaswell commented May 19, 2017

Try to be more consistent about the alerts we send #3480

Try to be more consistent about the alerts we send #3480

Conversation

mattcaswell commented May 16, 2017

richsalz left a comment

Choose a reason for hiding this comment

kaduk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattcaswell commented May 18, 2017

mattcaswell commented May 19, 2017