Test that disabling PSK suites also disables resumption

RESOLVED FIXED in 3.25

Status

NSS
Libraries
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mt, Assigned: mt)

Tracking

Firefox Tracking Flags

(firefox48 affected)

Details

Attachments

(1 obsolete attachment)

(Assignee)

Description

2 years ago
Created attachment 8737601 [details] [diff] [review]
0001-Disable-PSK-and-disable-resumption.patch

I wrote a test for this.  It's probably useful.
Attachment #8737601 - Flags: review?(ekr)

Comment 1

2 years ago
Comment on attachment 8737601 [details] [diff] [review]
0001-Disable-PSK-and-disable-resumption.patch

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

LGTM

::: external_tests/ssl_gtest/ssl_loopback_unittest.cc
@@ +1012,5 @@
> +  Connect();
> +  SendReceive();
> +  CheckKeys(ssl_kea_ecdh, ssl_auth_rsa);
> +  DataBuffer psk1(c1->extension());
> +  EXPECT_EQ(psk1.len(), 0UL);

Is there a reason to make a DataBuffer? Can't you interrogate extension directly
Attachment #8737601 - Flags: review?(ekr) → review+
(Assignee)

Comment 2

2 years ago
Comment on attachment 8737601 [details] [diff] [review]
0001-Disable-PSK-and-disable-resumption.patch

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

> Is there a reason to make a DataBuffer? Can't you interrogate extension directly

That spun off an investigation.  I discovered several things:

1. we leak the DummyPrSocket on every test
2. the resume TLS 1.3 twice test never bothered to capture the PSK extension
3. because I cribbed from that test without double-checking, neither did this test
4. when we disable all the PSK suites, we still send the PSK extension (this is benign, but we shouldn't)

I will work on a patch during this upcoming breakfast meeting.
Attachment #8737601 - Flags: review-
(Assignee)

Comment 4

2 years ago
See rietveld request above.
Flags: needinfo?(ekr)
(Assignee)

Updated

2 years ago
Attachment #8737601 - Attachment is obsolete: true
(Assignee)

Comment 5

2 years ago
See rietveld request: https://codereview.appspot.com/292550043

Comment 6

2 years ago
LGTM in Rietveld.
Flags: needinfo?(ekr)
(Assignee)

Comment 7

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