New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Tolerate a zero length ticket nonce #3957
Conversation
TLSv1.3 draft-21 requires the ticket nonce to be at least 1 byte in length. However NSS sends a zero length nonce. This is actually ok because the next draft will allow zero length nonces anyway, so we should tolerate this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
approved once the tests build.
TLSv1.3 draft-21 requires the ticket nonce to be at least 1 byte in length. However NSS sends a zero length nonce. This is actually ok because the next draft will allow zero length nonces anyway, so we should tolerate this. Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #3957)
@@ -2422,7 +2422,6 @@ MSG_PROCESS_RETURN tls_process_new_session_ticket(SSL *s, PACKET *pkt) | |||
|| (SSL_IS_TLS13(s) | |||
&& (!PACKET_get_net_4(pkt, &age_add) | |||
|| !PACKET_get_length_prefixed_1(pkt, &nonce) | |||
|| PACKET_remaining(&nonce) == 0 | |||
|| !PACKET_memdup(&nonce, &s->session->ext.tick_nonce, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(PACKET_memdup() does not write to the data or length output parameters when PACKET_remaining(pkt) is zero. This is vaguely tolerable here because the session's extensions block is initialized to zero, but is vaguely disquieting.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. Maybe it should?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, I was reading too fast and only looked at the body of the "length == 1" conditional -- *data
and *len
are initialized unconditionally in PACKET_memdup(), so there's nothing to do.
I already pushed this before @kaduk's comment, so closing. If there are any follow on actions it will have to be a different PR. |
TLSv1.3 draft-21 requires the ticket nonce to be at least 1 byte in length.
However NSS sends a zero length nonce. This is actually ok because the next
draft will allow zero length nonces anyway, so we should tolerate this.