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

RESOLVED FIXED in Firefox 31

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: briansmith, Assigned: briansmith)

Tracking

Trunk
mozilla32
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox31- fixed, firefox32 fixed)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment)

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: 5 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+
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.