Harry/9 #12
Harry/9 #12
Conversation
ptlshs/signal.go
Outdated
| // The server signal is made to look like the response. The maximum payload size for supported | ||
| // cipher suites is in the range of 1151 to 1187 bytes. We cap the server signal below this | ||
| // range to avoid complications from split records. | ||
| minSignalLenServer, maxSignalLenServer = 250, 1150 |
There was a problem hiding this comment.
This was the source of one of the bugs. When server signals were > 1150 bytes, they were sometimes split into multiple records. The client code assumes server signals will arrive in a single record. We could implement a length header into the server signal and piece it together from multiple records, but this was a simpler fix and seems acceptable to me.
ptlshs/signal.go
Outdated
| actualMinSignalLenServer = rand.Intn(maxSignalLenServer - minSignalLenServer - serverSignalLenSpread) | ||
| rand.Seed(time.Now().UnixNano()) | ||
| randSpread := maxSignalLenServer - minSignalLenServer - serverSignalLenSpread | ||
| actualMinSignalLenServer = rand.Intn(randSpread) + minSignalLenServer |
There was a problem hiding this comment.
This was an arithmetic mistake and the source of the second bug. We were allowing server signals less than minSignalLenServer which confused clients. This bug was masked in tests because math/rand is deterministic when not seeded - which I didn't realize!
The determinism in math/rand actually defeated the randomization of server signal lengths across runtimes, effectively a third bug. We are now properly seeding. This will also help in testing as we have a few tests in ptlshs which fuzz data using math/rand.
There was a problem hiding this comment.
Ah, good catch! I think it's a general practice to use crypto/rand instead of math/rand for stuff that's even remotely security related, so maybe worth switching to that?
There was a problem hiding this comment.
math/rand is only used here to randomize lengths (and crypto/rand doesn't have API for random integers anyway). In testing, it's also used just to randomize lengths. The only case in this package which actually uses random data is in the nonce, which indeed uses crypto/rand.
There was a problem hiding this comment.
Oh btw, rand.Seed is global, and some other code might change the seed (or maybe even clear it?). To be safe, you should probably make a new local random.
There was a problem hiding this comment.
Right - will change, thanks
There was a problem hiding this comment.
@oxtoacart I realized that creating a new local random for all uses of math/rand would be a bit cumbersome. A shared, package-internal one would need to be protected against concurrent access (the underlying rand.Source would not be directly safe for concurrent access).
I also did some more reading about using math/rand for generating simple integers like this. This thread was particularly insightful. There were quite a few comments which echoed your point that we should just always use crypto/rand, even for simple integer generation (for example, this comment).
So I am convinced, and I will switch over to crypto/rand for everything in this repo. Thanks for the pointer and the great conversation on Slack. Sorry it took me so long to come around!
There was a problem hiding this comment.
As an aside, that thread I linked also has some great discussion on some of the math/rand design decisions which you and I talked about.
This thread is only tangentially related, but also interesting!
| @@ -15,7 +15,7 @@ jobs: | |||
| name: Test | |||
| # We have a lot of concurrency and synchronization in this library. Test with -race and | |||
| # -count=1000 to expose any timing related bugs. | |||
| command: go test -race -count=1000 ./... | |||
| command: go test -race -count=1000 -v ./... | |||
There was a problem hiding this comment.
|
Thanks @oxtoacart! |
Fixes a couple of bugs related to the newly introduced server signals.