Closed
Bug 982774
Opened 10 years ago
Closed 10 years ago
Test failure for mozilla::pkixder ExpectTagAndGetLengthWithWrongLength
Categories
(Core :: Security: PSM, defect)
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: st3fan, Assigned: keeler)
References
Details
Attachments
(1 file)
2.78 KB,
patch
|
briansmith
:
review+
|
Details | Diff | Splinter Review |
The ExpectTagAndGetLengthWithWrongLength is failing on truncated data. I think it should check how much input is actually available and fail immediately. Although other functions properly check the length I would have a better feeling if this function would fail immediately. This will prevent the possibility of reading past the end of the buffer.
Comment 1•10 years ago
|
||
Maybe it is better for ExpectTagAndGetLength to check that there are at least |length| bytes of input and fail if there are not. However, ExpectTagAndGetLength is working as initially intended. The thinking is that the caller will eventually try to parse |length| bytes of input after calling ExpectTagAndGetLength and *that* will fail due to EOF. One of the goals in the design of this DER parser is to make it as efficient as possible. That is why it tries to be zero-copy and why it avoids some unnecessary checks, except for array out-of-bounds checks in the Input class. However, I agree with Stefan that it seems like a good idea to have ExpectTagAndGetLength fail fast. If the extra length check ends up being a performance problem (unlikely), we can undo it.
Assignee | ||
Updated•10 years ago
|
Summary: Test failure for insanity::pkixder ExpectTagAndGetLengthWithWrongLength → Test failure for mozilla::pkixder ExpectTagAndGetLengthWithWrongLength
Assignee | ||
Comment 2•10 years ago
|
||
My understanding is that Stefan isn't available to work on these any more, so I'll drive these forward unless I can find someone else to do it.
Updated•10 years ago
|
Attachment #8407662 -
Flags: review?(brian) → review+
Assignee | ||
Comment 3•10 years ago
|
||
Thanks, Brian. https://hg.mozilla.org/integration/mozilla-inbound/rev/95e6b5e31ce1
Comment on attachment 8407662 [details] [diff] [review] patch > Result Expect(const uint8_t* expected, size_t expectedLen) > { >- if (input + expectedLen > end) { >+ if (EnsureLength(expectedLen) != Success) { > return Fail(SEC_ERROR_BAD_DER); > } Is it problematic that this case is now converting a size_t to uint16_t before doing the check?
Flags: needinfo?(dkeeler)
Comment 5•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/95e6b5e31ce1
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Assignee | ||
Comment 6•10 years ago
|
||
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #4) > Comment on attachment 8407662 [details] [diff] [review] > patch > > > Result Expect(const uint8_t* expected, size_t expectedLen) > > { > >- if (input + expectedLen > end) { > >+ if (EnsureLength(expectedLen) != Success) { > > return Fail(SEC_ERROR_BAD_DER); > > } > > Is it problematic that this case is now converting a size_t to uint16_t > before doing the check? Good catch. In this case, I think it's not a problem because we don't let an Input have a length greater than what can be expressed in a uint16_t: 77 Result Init(const uint8_t* data, size_t len) 78 { 79 if (input) { 80 // already initialized 81 return Fail(SEC_ERROR_INVALID_ARGS); 82 } 83 if (!data || len > 0xffffu) { 84 // input too large 85 return Fail(SEC_ERROR_BAD_DER); 86 } Since that's the case, I think Expect should take a uint16_t instead of a size_t (it's only used by OID anyway, which is never going to be very long).
Flags: needinfo?(dkeeler)
Comment 7•10 years ago
|
||
Still failing. Is this a bug in the test or ExpectTagAndGetLength? This test was passing last week, but maybe I had an old client. https://tbpl.mozilla.org/?tree=Try&rev=7ce1f77b5392
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 8•10 years ago
|
||
Sorry, the failure is with ExpectTagAndLength, not ExpectTagAndGetLength. ExpectTagAndLength is never supposed to fail even if the input is the wrong length, right?
Comment 9•10 years ago
|
||
Argh, I can't read. The changes to the test are as expected according to this bug.
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•