Closed Bug 1340260 Opened 3 years ago Closed 3 years ago

Tokenizer can return a string with negative (overflowed) length


(Core :: XPCOM, defect, critical)

48 Branch
Not set



Tracking Status
firefox-esr45 --- unaffected
firefox51 --- wontfix
firefox52 --- wontfix
firefox53 --- wontfix
firefox54 --- fixed


(Reporter: mayhemer, Assigned: mayhemer)




(1 file, 2 obsolete files)

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.
Attached patch v1 (obsolete) — Splinter Review
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: nobody → honzab.moz
Attachment #8838241 - Flags: review?(nfroyd)
Attached patch test, v1 (obsolete) — Splinter Review
Here is a test, not sure we want to land this, since it could help discover the flaw relatively easily.
There are only two uses of ReadUntil in the codebase (m-c, m-a, m-b):

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
Duplicate of this bug: 1340252
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+
Attachment #8838241 - Flags: review?(nfroyd) → review+
Thanks for the quick r.

Is there anyone who has the correct rights to open this bug to the public?
Keywords: checkin-needed
I guess that would be dveditz?  See comment 3.
Flags: needinfo?(dveditz)
Attachment #8838241 - Attachment is obsolete: true
Attachment #8838243 - Attachment is obsolete: true
Attachment #8838587 - Flags: review+
Group: core-security
Flags: needinfo?(dveditz)
Pushed by
Fix potentially wrong string returned from Tokenizer::ReadUntil, keep Record/Claim work after ReadUntil. r=froydnj
Keywords: checkin-needed
Pushed by
Fix gtest Werror build bustage on a CLOSED TREE.
You need to log in before you can comment on or make changes to this bug.