Closed Bug 1227905 Opened 5 years ago Closed 5 years ago

Support ChaCha20+Poly1305 cipher suites

Categories

(NSS :: Libraries, defect)

defect
Not set
normal

Tracking

(firefox45 affected)

RESOLVED FIXED
Tracking Status
firefox45 --- affected

People

(Reporter: ttaubert, Assigned: ttaubert)

References

()

Details

Bug 917571 will add the cipher, this bug is about bringing it to TLS.
Patch at: https://codereview.appspot.com/277090043

Based on the cipher added in bug 917571, and Adam's initial implementation. Fixed/Changed how the IV is constructed according to the current draft. ssl_gtests.sh runs successfully with the new cipher.

Any tips for TLS loopback tests I could write? I see that the cipher is selected and the tests succeed, but should we exchange actual application data and assert the chosen cipher suite?
Flags: needinfo?(martin.thomson)
Flags: needinfo?(ekr)
(In reply to Tim Taubert [:ttaubert] from comment #1)
> Patch at: https://codereview.appspot.com/277090043
> 
> Based on the cipher added in bug 917571, and Adam's initial implementation.
> Fixed/Changed how the IV is constructed according to the current draft.
> ssl_gtests.sh runs successfully with the new cipher.
>

OK. Will try to take a look today or over the weekend.


> Any tips for TLS loopback tests I could write? I see that the cipher is
> selected and the tests succeed, but should we exchange actual application
> data and assert the chosen cipher suite?

Yes. And there are existing tests for that, I believe.
Flags: needinfo?(ekr)
(In reply to Eric Rescorla (:ekr) from comment #2)
> OK. Will try to take a look today or over the weekend.

Thanks!

> > Any tips for TLS loopback tests I could write? I see that the cipher is
> > selected and the tests succeed, but should we exchange actual application
> > data and assert the chosen cipher suite?
> 
> Yes. And there are existing tests for that, I believe.

Just found TlsConnectTestBase::SendReceive(), will work on a test.
Added a test and updated the patch.
New version of the patch at: https://codereview.appspot.com/277090043

Updated cipher suite numbers according to the latest draft at https://tools.ietf.org/html/draft-ietf-tls-chacha20-poly1305-03
Flags: needinfo?(ekr)
r+ and nits on rietveld.
Flags: needinfo?(martin.thomson)
(In reply to Martin Thomson [:mt:] from comment #6)
> r+ and nits on rietveld.

I don't see new comment on rietveld, did you forget to submit them?
Flags: needinfo?(martin.thomson)
Must have lost it.  I looked again.
Flags: needinfo?(martin.thomson)
(In reply to Martin Thomson [:mt:] from comment #8)
> Must have lost it.  I looked again.

Thanks! Updated patch to address your feedback.
Patch updated to reflect changes from draft-ietf-tls-chacha20-poly1305-04. Rebased due to Bob's patch too.
Adding Wan-Teh because I added him as a reviewer on Rietveld.
Flags: needinfo?(wtc)
I reviewed the patch at https://codereview.appspot.com/277090043/.
Flags: needinfo?(wtc)
Posted the hopefully final version of the patch on Rietveld.
Flags: needinfo?(ekr) → needinfo?(wtc)
I reviewed patch set 12 at https://codereview.appspot.com/277090043/.
Flags: needinfo?(wtc)
https://hg.mozilla.org/projects/nss/rev/83e27ac21329
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/projects/nss/rev/d60719dd22fd
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Depends on: 1246939
Had to back out again, there was some more breakage and I didn't want to block the tree for too long:

https://hg.mozilla.org/projects/nss/rev/11c4a1c6f906
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Will merge the patch [1] addressing the PKCS#11 bypass assertion into this one. It has r=ekr.

[1] https://codereview.appspot.com/286200043
https://hg.mozilla.org/projects/nss/rev/b6c9ec057991
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Depends on: 1247579
FTR, I just built a custom Nightly with latest NSS and was able to communicate successfully with OpenSSL 1.1.0-pre2 via TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256.
Blocks: 1247860
Target Milestone: --- → 3.23
Blocks: 1255443
You need to log in before you can comment on or make changes to this bug.