DTLS 1.3 CertificateRequest + Certificate Messages#774
DTLS 1.3 CertificateRequest + Certificate Messages#774theodorsm merged 2 commits intopion:masterfrom
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #774 +/- ##
==========================================
+ Coverage 81.40% 81.61% +0.21%
==========================================
Files 103 105 +2
Lines 5717 5837 +120
==========================================
+ Hits 4654 4764 +110
- Misses 684 689 +5
- Partials 379 384 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ef35536 to
fd492eb
Compare
theodorsm
left a comment
There was a problem hiding this comment.
Wow, thank you a lot for such a fast implementation @adrianosela!! 💯
I have left some comments. I perfer the usage of cryptobyte like you do in the CertificateRequest implementation, it's a lot more readable and easy to catch errors. Additionally, would be cool if you could add some fuzzing tests for unmarshalling. I think it's a powerful way of catching bugs to have as much as possible of our raw bytes parsing fuzzed.
| certificateList := []byte{} | ||
| for _, entry := range m.CertificateList { | ||
| // Add cert_data as a 3-byte length prefix | ||
| certDataLen := len(entry.CertificateData) |
There was a problem hiding this comment.
We should check for boundaries <1..2^24-1>.
if certDataLen == 0 || certDataLen > 0xffffff {
return nil, errInvalidCertificateEntry
}
There was a problem hiding this comment.
For testing the upper bound, should I allocate a 16 MB slice... or is testing the zero case sufficient you think @theodorsm ?
There was a problem hiding this comment.
I think only testing for the zero case is sufficent.
There was a problem hiding this comment.
The actually max size should be 0xffffff - cert13CertLengthFieldSize. I forgot about the length prefix for the vector, sorry!
There was a problem hiding this comment.
Correct me if I'm wrong, but I think 0xFFFFFF was correct. I think from reading TLS/DTLS specs, variable-length vectors <floor..ceiling> constrain the vector contents length, and the length field is encoded separately:
Variable-length vectors are defined by specifying a subrange of legal
lengths, inclusively, using the notation <floor..ceiling>. When
these are encoded, the actual length precedes the vector's contents
in the byte stream. The length will be in the form of a number
consuming as many bytes as required to hold the vector's specified
maximum (ceiling) length. A variable-length vector with an actual
length field of zero is referred to as an empty vector.
from boringssl code: https://boringssl.googlesource.com/boringssl/%2B/master/ssl/tls13_both.cc#541
There was a problem hiding this comment.
Yeah @JoTurk is right here. The n-byte length fields are independent from the vector whose length they describe.
There was a problem hiding this comment.
@JoTurk, @adrianosela Yes, I agree that the length fields are independent, but to me the confusion arises from the parent vector: CertificateEntry certificate_list<0..2^24-1>.
As the vector holds a list of vectors, then the length prefix of any vector in this list is included in the 2^24-1 boundary no? It then seems to be that the max length for any certificate data must be 0xffffff - cert13CertLengthFieldSize? I would say this problem/confusion is avoided when using the cryptobyte library as it does to the upper bound checking for us. For example, this would return an error:
certificateData := make([]byte, 0xffffff)
for i := range certificateData {
certificateData[i] = byte(i)
}
var builder cryptobyte.Builder
builder.AddUint24LengthPrefixed(func(b *cryptobyte.Builder) {
b.AddUint24LengthPrefixed(func(b *cryptobyte.Builder) {
fmt.Printf("Length of certData: %x\n", len(certificateData))
b.AddBytes(certificateData)
fmt.Printf("Length of certData vector with prefix: %x\n", len(b.BytesOrPanic()))
})
fmt.Printf("Length of certificate_list vector with prefix: %x\n", len(b.BytesOrPanic()))
})
_, err := builder.Bytes()
if err != nil {
fmt.Println(err.Error())
}
// Output:
Length of certData: ffffff
Length of certData vector with prefix: 1000002
Length of certificate_list vector with prefix: 1000005
cryptobyte: pending child length 16777218 exceeds 3-byte length prefix
There was a problem hiding this comment.
I agree with @adrianosela points about the extension size.
Also in isolation we shouldn't check for the list bound here. because if we do, we'll return errInvalidCertificateEntry when an entry is only out of bounds because it causes the parent list to exceed its limit, in that case we should return errCertificateListTooLong, which is already checked at the end of the loop. So subtracting -3 or -5 here is really a logic / spec bug.
|
Sounds good! Thanks for the fast review! I'll get to it sometime soon / this week. I'll request a review from you when its ready for another pass. |
b2b0743 to
51e9f64
Compare
|
@theodorsm ready for another pass. Q: I put the fuzz tests in the same test file as the other tests. Or do you want them in fuzz_test.go? No strong opinion from me for either of these items. |
I prefer having them in the test file. Having the fuzz cases as small as possible leads to better results (the fuzzer engine with it's coverage guidance have to dig through less paths) + we can seed them better and adjust fail cases if necessary.
|
theodorsm
left a comment
There was a problem hiding this comment.
Great work! I have two more comments on the PR (one of them being my fault, sorry).
| certificateList := []byte{} | ||
| for _, entry := range m.CertificateList { | ||
| // Add cert_data as a 3-byte length prefix | ||
| certDataLen := len(entry.CertificateData) |
There was a problem hiding this comment.
The actually max size should be 0xffffff - cert13CertLengthFieldSize. I forgot about the length prefix for the vector, sorry!
69cd542 to
87c0158
Compare
|
e: @JoTurk highlighted the bounds are correct, and I concur. |
| certificateList := []byte{} | ||
| for _, entry := range m.CertificateList { | ||
| // Add cert_data as a 3-byte length prefix | ||
| certDataLen := len(entry.CertificateData) |
There was a problem hiding this comment.
@JoTurk, @adrianosela Yes, I agree that the length fields are independent, but to me the confusion arises from the parent vector: CertificateEntry certificate_list<0..2^24-1>.
As the vector holds a list of vectors, then the length prefix of any vector in this list is included in the 2^24-1 boundary no? It then seems to be that the max length for any certificate data must be 0xffffff - cert13CertLengthFieldSize? I would say this problem/confusion is avoided when using the cryptobyte library as it does to the upper bound checking for us. For example, this would return an error:
certificateData := make([]byte, 0xffffff)
for i := range certificateData {
certificateData[i] = byte(i)
}
var builder cryptobyte.Builder
builder.AddUint24LengthPrefixed(func(b *cryptobyte.Builder) {
b.AddUint24LengthPrefixed(func(b *cryptobyte.Builder) {
fmt.Printf("Length of certData: %x\n", len(certificateData))
b.AddBytes(certificateData)
fmt.Printf("Length of certData vector with prefix: %x\n", len(b.BytesOrPanic()))
})
fmt.Printf("Length of certificate_list vector with prefix: %x\n", len(b.BytesOrPanic()))
})
_, err := builder.Bytes()
if err != nil {
fmt.Println(err.Error())
}
// Output:
Length of certData: ffffff
Length of certData vector with prefix: 1000002
Length of certificate_list vector with prefix: 1000005
cryptobyte: pending child length 16777218 exceeds 3-byte length prefix
Implements TLS 1.3 Certificate Request and Certificate messages per RFC 8446 Sections 4.3.2 and 4.4.2 respectively. Adds comprehensive unit tests for both.
|
So if I understand what you are saying: the current bounds check is correct per RFC i.e. each Does that capture your point? I agree with you, but the cases where either the 3-byte-length prefix or the data that comes after it is incorrectly (but valid per RFC) between Also, if we are going to go with more strict bounds checking, Also, we are not accounting for data from So, really we may as well check that it is <= Personally, I think the bounds check per RFC's bounds (and as @JoTurk pointed out, in accordance with BoringSSLs implementation) are the way to go about this. This also keeps the code simpler, and not any less correct. Malformed packets with I've moved that check to inside the loop to fail faster. Would that address your concern? |
Adds unit tests that test the upper bounds of the produced certificate_list's length when marshaling a MessageCertificate13
|
We said earlier that we wouldnt add tests that allocate 16MB slices to test the upper bounds:
But I think the conversation above merits adding such tests! Added as a new commit, let me know if you disagree and want me to drop it. |
theodorsm
left a comment
There was a problem hiding this comment.
I agree with the conclusion about the upper bound checking, and thanks for the discussion about it!
This PR looks really good now, especially with all the good tests.
@adrianosela thanks for working on this and the quick iterations 💯.
DTLS 1.3 CertificateRequest + Certificate Messages
Closes #772
CertificateRequest13per RFC 8446 section 4.3.2Certificate13per RFC 8446 section 4.4.2cc:// @theodorsm