1334290 - Truncation in nsScanner

Status: RESOLVED FIXED

(Core :: XML, defect)

Description

[q1]

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 .

Comment 1

[q1]

There is also a potential overflow of similar type on the assignment:

   mCountRemaining += length;

in nsScanner::UngetReadable().

Comment 2

[Henri Sivonen (:hsivonen)]

I can take a look on Feb 14.

Comment 3

[Henri Sivonen (:hsivonen)]

I'm running out of time for today, but it seems that both variables that are problematic are unnecessary to have.

Comment 4

[Henri Sivonen (:hsivonen)]

Attachment: Remove useless fields from nsScanner

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.

Comment 5

[Henri Sivonen (:hsivonen)]

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.

Comment 6

[Andrew McCreight [:mccr8]]

> 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.

Comment 7

[Henri Sivonen (:hsivonen)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f54a857324e4661bfdd67996551fea7201d3127a
Bug 1334290 - Remove useless fields from nsScanner. r=mrbkap.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f54a857324e4

Comment 9

[Henri Sivonen (:hsivonen)]

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 10

[Gerry Chang [:gchang]]

Track 53+/54+ as security issue.

Comment 11

[Gerry Chang [:gchang]]

Comment on attachment 8837671 [details] [diff] [review]
Remove useless fields from nsScanner

Fix a security issue. Aurora53+.

Comment 12

[Julien Cristau [:jcristau]]

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.

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/17c1ac3fe6cc

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c17f3d7837f0

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/c17f3d7837f0