Closed Bug 982774 Opened 10 years ago Closed 10 years ago

Test failure for mozilla::pkixder ExpectTagAndGetLengthWithWrongLength

Categories

(Core :: Security: PSM, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: st3fan, Assigned: keeler)

References

Details

Attachments

(1 file)

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.
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.
Summary: Test failure for insanity::pkixder ExpectTagAndGetLengthWithWrongLength → Test failure for mozilla::pkixder ExpectTagAndGetLengthWithWrongLength
Attached patch patchSplinter Review
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.
Assignee: sarentz → dkeeler
Status: NEW → ASSIGNED
Attachment #8407662 - Flags: review?(brian)
Attachment #8407662 - Flags: review?(brian) → review+
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?
https://hg.mozilla.org/mozilla-central/rev/95e6b5e31ce1
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
(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)
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 → ---
Sorry, the failure is with ExpectTagAndLength, not ExpectTagAndGetLength. ExpectTagAndLength is never supposed to fail even if the input is the wrong length, right?
Argh, I can't read. The changes to the test are as expected according to this bug.
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.