Closed Bug 1340260 Opened 3 years ago Closed 3 years ago
Tokenizer can return a string with negative (overflowed) length
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.
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
Status: NEW → ASSIGNED
Attachment #8838241 - Flags: review?(nfroyd)
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): 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.)
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?
I guess that would be dveditz? See comment 3.
Pushed by firstname.lastname@example.org: 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
Pushed by email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a4fb1ed3d4f1 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.