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
Verify that only NULL compression is sent in TLSv1.3 ClientHello #3410
Conversation
It is illegal in a TLSv1.3 ClientHello to send anything other than the NULL compression method. We should send an alert if we find anything else there. Previously we were ignoring this error.
Test that sending a non NULL compression method fails in TLSv1.3 as well as other similar tests.
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.
Code looks good, just a couple extraneous things in the tests that probably don't really harm anything.
test/recipes/70-test_comp.t
Outdated
if disabled("tls1_3"); | ||
|
||
$ENV{OPENSSL_ia32cap} = '~0x200000200000000'; | ||
$ENV{CTLOG_FILE} = srctop_file("test", "ct", "log_list.conf"); |
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.
Do we need the CTLOG_FILE for this test? (I didn't decode the ia32cap vector to see if we need it or not.)
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.
Probably not, although it is in all of the tests. The ia32cap vector turns off AESNI which caused me some headaches trying to figure out why it was failing in one of the other tests at one point. It only causes an issue under certain circumstances (which I don't now remember), but I just turn it off by default now.
} | ||
|
||
SKIP: { | ||
skip "TLSv1.3 disabled", 2 if disabled("tls1_3"); |
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.
You skip_all if tls1_3 is disabled, so this seems redundant?
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.
Oh, that's a copy&paste error. We shouldn't be doing a skip_all. Will fix.
test/recipes/70-test_comp.t
Outdated
@comp = ( | ||
0x00, #Null compression method | ||
0xff); #Unknown compression | ||
} else { |
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.
It might be more future-proof to explicitly check against NON_NULL_COMPRESSION
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.
Ok.
reconfirm |
Pushed. Thanks. |
It is illegal in a TLSv1.3 ClientHello to send anything other than the NULL compression method. We should send an alert if we find anything else there. Previously we were ignoring this error. Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #3410)
Test that sending a non NULL compression method fails in TLSv1.3 as well as other similar tests. Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #3410)
Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #3410)
It is illegal in a TLSv1.3 ClientHello to send anything other than the
NULL compression method. We should send an alert if we find anything else
there. Previously we were ignoring this error.
Checklist