Closed Bug 1271878 Opened 4 years ago Closed 4 years ago

ServerHello parsing for extensions is broken in tls_filter.cc

Categories

(NSS :: Test, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mt, Assigned: mt)

References

Details

Attachments

(1 file)

The format of the TLS 1.3 ServerHello has changed and so the parsing is wrong.
This is one of the "gratuitous changes" you asked me to split out from the DHE patch.  I was depending on it though as it turns out.
Attachment #8751100 - Flags: review?(ekr)
Assignee: nobody → martin.thomson
As I didn't review this file yet, I wouldn't say that I asked you to split it out.

However, it seems like good practice in any case.
:) I did what I could to break out what I could.  There were only a few things.
Comment on attachment 8751100 [details] [diff] [review]
bug1271878-1.patch

Review of attachment 8751100 [details] [diff] [review]:
-----------------------------------------------------------------

::: external_tests/ssl_gtest/libssl_internals.c
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*- */

What happened here?

@@ +37,5 @@
>  
> +    if (authAlgorithm != ssl_auth_rsa_decrypt &&
> +        authAlgorithm != ssl_auth_rsa_sign) {
> +        return 0;
> +    }

And here.

::: external_tests/ssl_gtest/test_io.cc
@@ +26,5 @@
>              << __FUNCTION__ << std::endl;        \
>    PR_ASSERT(PR_FALSE);                           \
>    PR_SetError(PR_NOT_IMPLEMENTED_ERROR, 0)
>  
> +#define LOG(a) std::cerr << name_ << ": " << a << std::endl

What happened here
Attachment #8751100 - Flags: review?(ekr)
Comment on attachment 8751100 [details] [diff] [review]
bug1271878-1.patch

Review of attachment 8751100 [details] [diff] [review]:
-----------------------------------------------------------------

r+ except for the changes to libssl_internals, which belong in a different patch.
Attachment #8751100 - Flags: review+
Comment on attachment 8751100 [details] [diff] [review]
bug1271878-1.patch

Review of attachment 8751100 [details] [diff] [review]:
-----------------------------------------------------------------

::: external_tests/ssl_gtest/libssl_internals.c
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*- */

This file has 4 character indents, but a modeline that has 2.  That's annoying.

@@ +37,5 @@
>  
> +    if (authAlgorithm != ssl_auth_rsa_decrypt &&
> +        authAlgorithm != ssl_auth_rsa_sign) {
> +        return 0;
> +    }

Too often if you have a test fail, this causes the test to crash.
Landed without the libssl_internals.c changes:

https://hg.mozilla.org/projects/nss/rev/aa86e41ad475
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.25
You need to log in before you can comment on or make changes to this bug.