Closed
Bug 1018033
Opened 10 years ago
Closed 10 years ago
Potential integer overflow in buffer length check in mozilla::pkix::der::Input::EnsureLength
Categories
(Core :: Security: PSM, defect)
Core
Security: PSM
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: briansmith, Assigned: briansmith)
References
Details
(Whiteboard: [qa-])
Attachments
(1 file)
4.52 KB,
patch
|
keeler
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•10 years ago
|
Blocks: mozilla::pkix-beta
Assignee | ||
Comment 1•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
status-firefox31:
--- → affected
tracking-firefox31:
--- → ?
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+
Assignee | ||
Comment 3•10 years ago
|
||
(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
Assignee | ||
Comment 4•10 years ago
|
||
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?
Comment 5•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3637e46a6150
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Updated•10 years ago
|
Updated•10 years ago
|
Attachment #8431404 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 8•10 years ago
|
||
Brian, should this be marked as in-testsuite+ ? Thanks!
Flags: needinfo?(brian)
Assignee | ||
Comment 9•10 years ago
|
||
Yes, there's an automated test included in the patch.
Flags: needinfo?(brian) → in-testsuite+
Updated•10 years ago
|
Whiteboard: [qa-]
You need to log in
before you can comment on or make changes to this bug.
Description
•