Closed
Bug 1340260
Opened 7 years ago
Closed 7 years ago
Tokenizer can return a string with negative (overflowed) length
Categories
(Core :: XPCOM, defect)
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox-esr45 | --- | unaffected |
firefox51 | --- | wontfix |
firefox52 | --- | wontfix |
firefox53 | --- | wontfix |
firefox54 | --- | fixed |
People
(Reporter: mayhemer, Assigned: mayhemer)
References
Details
Attachments
(1 file, 2 obsolete files)
2.39 KB,
patch
|
mayhemer
:
review+
|
Details | Diff | Splinter Review |
When Tokenizer::ReadUntil is called after EOF has been read, the pointers for Claim() may be screwed. mRecord will point at the '\0', if the string is zero-ended. mRollback (which is the cursor pointing at the end of the string we want to Claim()) may point back to the string, e.g when successful ReadUntil has been called just before. The code is based on presumption that Next() will shift the mRollback cursor to an advanced position, larger than mRecord. But when Next() fails because there is no input, mRollback remains in the previous (< mRecord) position. Claim is constructing the result as substring from mRecord to mRollback, where in this case mRecord > mRollback. The string api silently swallows this! And the length of the returned result then overflows. The Tokenizer method with this flaw has been added in bug 1261382, on 48.
Assignee | ||
Comment 1•7 years ago
|
||
core fix: set mRollback to be at mCursor before we attempt to call Next(), this ensures that mRecord <= mRollback. This also fixes bug 1340252 (discovered sooner than this bug, I will duplicate after this is landed) by saving and reverting value of mRecord to where it has been before this method's been called. I can separate to that bug, tho, if you want. Also, ReadUntil with this patch doesn't eat the EOF token, which most algorithms tend to use as a loop condition (while (!tokenizer.CheckEOF())) or some like that. When we eat it here, CheckEOF will return (correctly) false, since it has to tell you that you are right at the EOF, not after it. Can potentially be moved to bug 1340252 as well.
Assignee | ||
Comment 2•7 years ago
|
||
Here is a test, not sure we want to land this, since it could help discover the flaw relatively easily.
Assignee | ||
Comment 3•7 years ago
|
||
There are only two uses of ReadUntil in the codebase (m-c, m-a, m-b): https://dxr.mozilla.org/mozilla-central/rev/0eef1d5a39366059677c6d7944cfe8a97265a011/netwerk/cache2/CacheFileUtils.cpp#554 https://dxr.mozilla.org/mozilla-central/rev/0eef1d5a39366059677c6d7944cfe8a97265a011/netwerk/streamconv/converters/nsMultiMixedConv.cpp#507 Both are unaffected by this bug (no vulnerability) or by the patch (functionality won't change with it.) I think the bug should be opened to the public (I don't have the rights.)
Blocks: 1328791
Comment 5•7 years ago
|
||
Comment on attachment 8838243 [details] [diff] [review] test, v1 Review of attachment 8838243 [details] [diff] [review]: ----------------------------------------------------------------- If the bug is not actually security-sensitive, this patch should land with the actual fix.
Attachment #8838243 -
Flags: review+
Updated•7 years ago
|
Attachment #8838241 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 6•7 years ago
|
||
Thanks for the quick r. Is there anyone who has the correct rights to open this bug to the public?
Keywords: checkin-needed
Assignee | ||
Comment 8•7 years ago
|
||
Attachment #8838241 -
Attachment is obsolete: true
Attachment #8838243 -
Attachment is obsolete: true
Attachment #8838587 -
Flags: review+
Updated•7 years ago
|
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b9d589a8d513 Fix potentially wrong string returned from Tokenizer::ReadUntil, keep Record/Claim work after ReadUntil. r=froydnj
Keywords: checkin-needed
Comment 10•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a4fb1ed3d4f1 Fix gtest Werror build bustage on a CLOSED TREE.
Comment 11•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b9d589a8d513 https://hg.mozilla.org/mozilla-central/rev/a4fb1ed3d4f1
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Updated•7 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•