Refactor 1/n-1 record splitting code
ClosedPublic
Actions

Authored by mt on Sep 4 2017, 6:04 AM.

Details

Reviewers

Bugzilla Bug ID

Summary

It turns out that something changed a while back and we started splitting far
more than is needed. The original design split into 1/n-1/n/n/n, but now we
split 1/n-1/1/n-1/1/n-1 for large writes. That's inefficient and the code is
unnecessarily complex in order to support it.

This splits just once for each write, but it splits 1/n/n/n/n/remainder, unlike
the original design, which you can see here:
https://src.chromium.org/viewvc/chrome/trunk/src/net/third_party/nss/ssl/ssl3con.c?r1=97269&r2=97268&pathrev=97269

Diff Detail

Repository

rNSS nss

Branch

NSS_TLS13_DRAFT19_BRANCH

Lint

No Lint Coverage

Unit

No Test Coverage

Build Status

Buildable 50
Build 79: arc lint + arc unit

Event Timeline

mt created this revision.Sep 4 2017, 6:04 AM

phab-bot changed the visibility from "Administrators" to "Public (No Login Required)".Sep 4 2017, 6:04 AM

Harbormaster completed remote builds in B35: Diff 42.Sep 4 2017, 6:04 AM

mt added a child revision: D23: Bug 1396487 - Record size limiting extension.Sep 4 2017, 6:19 AM

This needs a test to demonstrate that it works.

lib/ssl/ssl3con.c
2753	Nit: I would call this splitNeeded or something, because I couldn't initially tell if this was part tense (i.e., we did split).

This revision now requires changes to proceed.Sep 4 2017, 12:06 PM

Test added. I learned something in the process: ssl3_SendApplicationData
splits up its inputs before passing them to ssl_SendRecord. Rather than fight
that to get the test to work, I moved the splitting one layer out. That's nice
because I managed to trim the content type check.

ekr accepted this revision.Sep 21 2017, 9:08 PM

ekr added inline comments.

gtests/ssl_gtest/ssl_loopback_unittest.cc
481	I think it's probably worth having some abstraction here. size_t ExpectedCbcRecordSize(size_t in) { return ((in + 1) / 16) * 16 }

This revision is now accepted and ready to land.Sep 21 2017, 9:08 PM

Landed with suggested changes, which were trivial. https://hg.mozilla.org/projects/nss/rev/27bfdd0ee644c33d3445a844d17d9042e0210035

gtests/ssl_gtest/ssl_loopback_unittest.cc
481	Yeah, I needed one of those for the other code I wrote, and it makes this nicer. BTW, it's ((in + 20 + 15) / 16) * 16. I always sort of knew that, but I was still sort of surprised at how wasteful HMAC-CBC is, especially with the "stronger" suites. No wonder people want a truncated MAC.