Closed Bug 1334290 Opened 6 years ago Closed 5 years ago
Truncation in ns
On x64 builds, nsScanner (parser\htmlparser\nsScanner.cpp) truncates the 64-bit |size_t| quantity Distance() into the 32-bit member variables |mCountRemaining| and |mFirstNonWhitespacePosition|. This matters if the nsScanner object represents a string > 4 GB . I do not know the security import of this bug.  This can occur; see, e.g., https://bugzilla.mozilla.org/show_bug.cgi?id=1334246 .
There is also a potential overflow of similar type on the assignment: mCountRemaining += length; in nsScanner::UngetReadable().
I can take a look on Feb 14.
I'm running out of time for today, but it seems that both variables that are problematic are unnecessary to have.
Assignee: nobody → hsivonen
We can remove mFirstNonWhitespacePosition by removing the optimization that avoids passing whitespace-only buffers to expat. We can remove mCountRemaining by realizing that we do not need to be able to rewrite the replacement character after the fact. Because about:blank is empty and never has bogus bytes, we can hard-code the XML case to begin with.
Attachment #8837671 - Flags: review?(mrbkap)
5 years ago
Component: HTML: Parser → XML
Attachment #8837671 - Flags: review?(mrbkap) → review+
Comment on attachment 8837671 [details] [diff] [review] Remove useless fields from nsScanner I have a hard time believing that this would get rated higher than bug 1334246, but requesting approval to follow the process. [Security approval request comment] > How easily could an exploit be constructed based on the patch? It's not clear whether there is an actual exploit to be constructed. > Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No. > Which older supported branches are affected by this flaw? All. > If not all supported branches, which bug introduced the flaw? Ancient flaw. > Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Trivial mechanical rebase needed. > How likely is this patch to cause regressions; how much testing does it need? Unlikely. A short time on nightly should be enough.
> I have a hard time believing that this would get rated higher than bug 1334246, Sounds reasonable to me. I'll mark it moderate, and then you don't need sec-approval.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f54a857324e4661bfdd67996551fea7201d3127a Bug 1334290 - Remove useless fields from nsScanner. r=mrbkap.
Comment on attachment 8837671 [details] [diff] [review] Remove useless fields from nsScanner Approval Request Comment > [Feature/Bug causing the regression]: Ancient. > [User impact if declined]: Unclear for the truncation. However, this bug also fixes denial of service when Firefox is served an XML file with a very long run of consecutive whitespace. > [Is this code covered by automated tests?]: No, because the test would be too large. > [Has the fix been verified in Nightly?]: Landed uneventfully on nightly. Verified the fix for the denial of service aspect. Didn't verify anything about truncation, because the offending code was completely deleted and there weren't known symptoms to look for. > [Needs manual test from QE? If yes, steps to reproduce]: No. > [List of other uplifts needed for the feature/fix]: Requires bug 1334246. > [Is the change risky?]: No. > [Why is the change risky/not risky?]: Deletion of useless code whose removal is believed to be understood. > [String changes made/needed]: None.
Comment on attachment 8837671 [details] [diff] [review] Remove useless fields from nsScanner Fix a security issue. Aurora53+.
Attachment #8837671 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8837671 [details] [diff] [review] Remove useless fields from nsScanner companion change for bug 1334246, let's get this one in 52.0b9 too.
Attachment #8837671 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main52+]
You need to log in before you can comment on or make changes to this bug.