Closed Bug 1018033 Opened 7 years ago Closed 7 years ago

Potential integer overflow in buffer length check in mozilla::pkix::der::Input::EnsureLength

Categories

(Core :: Security: PSM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32
Tracking Status
firefox31 - fixed
firefox32 --- fixed

People

(Reporter: briansmith, Assigned: briansmith)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file)

Result EnsureLength(uint16_t len)
   {
-    if (input + len > end) {
+    if (static_cast<size_t>(end - input) < len) {
       return Fail(SEC_ERROR_BAD_DER);
     }
     return Success;
   }

Imagine that this->input = std::numeric_limits<intptr_t>::max() and this->end == this->input and len == 1. Then input + len is zero and the comparison would not detect the overflow.
Note that I added tests for Input::Read even though Input::Read does not use EnsureLength and so neither variant was affected by overflows. Note also that the reason Input::Read(uint16_t) isn't affected by overflows is because it is impossible to construct a valid input to Input::Init that straddles the overflow boundary. Bug 1018061 tries to make this clearer.
Attachment #8431404 - Flags: review?(dkeeler)
Comment on attachment 8431404 [details] [diff] [review]
prevent-overflow-in-EnsureLength.patch

Review of attachment 8431404 [details] [diff] [review]:
-----------------------------------------------------------------

Do we support any platforms where the top n GB of memory isn't kernel-privileged? In any case, I agree this is a good thing to fix.
Attachment #8431404 - Flags: review?(dkeeler) → review+
(In reply to David Keeler (:keeler) [use needinfo?] from comment #2)
> Do we support any platforms where the top n GB of memory isn't
> kernel-privileged?

I don't know.

Thanks for the review!

https://hg.mozilla.org/integration/mozilla-inbound/rev/3637e46a6150
Comment on attachment 8431404 [details] [diff] [review]
prevent-overflow-in-EnsureLength.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 915930 and the other bugs it depends on.
User impact if declined: Potential buffer read overflow.
Testing completed (on m-c, etc.): This code is thoroughly tested with automated tests and new automated tests were added to verify this fix (on at least some platforms).
Risk to taking this patch (and alternatives if risky): Very low. This is a very simple change.
String or IDL/UUID changes made by this patch: I would never do this to you.
Attachment #8431404 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/3637e46a6150
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Not tracking. See quite minor to me.
Attachment #8431404 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Brian, should this be marked as in-testsuite+ ? Thanks!
Flags: needinfo?(brian)
Yes, there's an automated test included in the patch.
Flags: needinfo?(brian) → in-testsuite+
Depends on: 1578626
You need to log in before you can comment on or make changes to this bug.