Closed Bug 1334290 Opened 3 years ago Closed 3 years ago

Truncation in nsScanner

Categories

(Core :: XML, defect)

50 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox-esr45 --- wontfix
firefox51 --- wontfix
firefox52 + fixed
firefox-esr52 52+ fixed
firefox53 + fixed
firefox54 + fixed

People

(Reporter: q1, Assigned: hsivonen)

Details

(Keywords: csectype-intoverflow, sec-moderate, Whiteboard: [post-critsmash-triage][adv-main52+])

Attachments

(1 file)

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 [1].

I do not know the security import of this bug.

[1] 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().
Group: core-security → dom-core-security
Flags: needinfo?(hsivonen)
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
Flags: needinfo?(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)
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.
Attachment #8837671 - Flags: sec-approval?
> 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.
Attachment #8837671 - Flags: sec-approval?
https://hg.mozilla.org/mozilla-central/rev/f54a857324e4
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
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.
Attachment #8837671 - Flags: approval-mozilla-beta?
Attachment #8837671 - Flags: approval-mozilla-aurora?
Track 53+/54+ as security issue.
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+
Flags: sec-bounty+
Group: dom-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main52+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.