1228103 - (CVE-2016-1974) Lack of status return from nsScannerString::AppendUnicodeTo causes out-of-bounds read in AppendErrorPointer

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[q1]

Attachment: poc.7z: unzip and use as described in comment 0 (1.1gb when unzipped)

nsScannerString::AppendUnicodeTo (parser\htmlparser\nsScannerString.cpp) calls nsAString::SetLength (which can fallibly fail under OOM conditions), but AppendUnicodeTo does not return status, so its callers incorrectly assume that it cannot fail. When the caller is AppendErrorPointer (parser\htmlparser\nsExpatDriver.cpp), and a document similar to the attached proof-of-concept is loaded, this omission causes AppendErrorPointer to read memory to which it has no right. AFAIKT this doesn't cause a meaningful information leak, only a broken error string and/or a crash. However, there are many other callers of nsScannerString::AppendUnicodeTo, plus there are callers of nsScannerString::CopyUnicodeTo, which has a similar bug. Probably some of these callers malfunction insecurely when AppendUnicodeTo or CopyUnicodeTo fail.

The bug is still present in the current trunk: http://hg.mozilla.org/mozilla-central/file/tip/parser/htmlparser/nsScannerString.cpp .


Details
-------

The bug is in the void return type:

485: void
486: AppendUnicodeTo( const nsScannerIterator& aSrcStart,
487:                  const nsScannerIterator& aSrcEnd,
488:                  nsAString& aDest )
489:   {
490:     nsAString::iterator writer;
491:     uint32_t oldLength = aDest.Length();
492:     if (!aDest.SetLength(oldLength + Distance(aSrcStart, aSrcEnd), mozilla::fallible))
493:       return; // out of memory
494:     aDest.BeginWriting(writer).advance(oldLength);
495:     nsScannerIterator fromBegin(aSrcStart);
496:     
497:     copy_multifragment_string(fromBegin, aSrcEnd, writer);
498:   }

With the attached POC, this bug causes out-of-bounds reading in AppendErrorPointer. This occurs because nsExpatDriver::ConsumeToken, line 1126, updates its |mExpatParser.mPosition.col| member to account for the number of just-consumed characters (|consumed|) [1], Then it calls AppendUnicodeTo to append those characters to |mLastLine| (line 1139):

1115:     uint32_t consumed;
1116:     ParseBuffer(buffer, length, noMoreBuffers, &consumed);
1117:     if (consumed > 0) {
1118:       nsScannerIterator oldExpatPosition = currentExpatPosition;
1119:       currentExpatPosition.advance(consumed);
1120: 
1121:       // We consumed some data, we want to store the last line of data that
1122:       // was consumed in case we run into an error (to show the line in which
1123:       // the error occurred).
1124: 
1125:       // The length of the last line that Expat has parsed.
1126:       XML_Size lastLineLength = XML_GetCurrentColumnNumber(mExpatParser);
1127: 
1128:       if (lastLineLength <= consumed) {
1129:         // The length of the last line was less than what expat consumed, so
1130:         // there was at least one line break in the consumed data. Store the
1131:         // last line until the point where we stopped parsing.
1132:         nsScannerIterator startLastLine = currentExpatPosition;
1133:         startLastLine.advance(-((ptrdiff_t)lastLineLength));
1134:         CopyUnicodeTo(startLastLine, currentExpatPosition, mLastLine);
1135:       }
1136:       else {
1137:         // There was no line break in the consumed data, append the consumed
1138:         // data.
1139:         AppendUnicodeTo(oldExpatPosition, currentExpatPosition, mLastLine);
1140:       }
1141:     }

But this fails because the resulting string is longer than the maximum allowed by nsString (0x3ffffff9, enforced by nsTSubstring_CharT::MutatePrep). Because AppendUnicodeTo doesn't return status, ConsumeToken doesn't learn about this failure, and so doesn't correct |mPosition| to account for it. Thus, |mPosition| indexes data that's beyond the end of |mLastLine|. ConsumeToken then plows ahead, calling nsExpatDriver::HandleError (line 1184):

1160:     if (NS_FAILED(mInternalState)) {
1161:       if (XML_GetErrorCode(mExpatParser) != XML_ERROR_NONE) {
1162:         NS_ASSERTION(mInternalState == NS_ERROR_HTMLPARSER_STOPPARSING,
1163:                      "Unexpected error");
...
1184:         HandleError();
1185:       }
1186: 
1187:       return mInternalState;
1188:     }

which calls XML_GetCurrentColumnNumber (line 946):

884: nsresult
885: nsExpatDriver::HandleError()
886: {
887:   int32_t code = XML_GetErrorCode(mExpatParser);
...
945:   // Adjust the column number so that it is one based rather than zero based.
946:   uint32_t colNumber = XML_GetCurrentColumnNumber(mExpatParser) + 1;
947:   uint32_t lineNumber = XML_GetCurrentLineNumber(mExpatParser);
948: 
949:   nsAutoString errorText;
950:   CreateErrorText(description.get(), XML_GetBase(mExpatParser), lineNumber,
951:                   colNumber, errorText);
952: 
953:   NS_ASSERTION(mSink, "no sink?");

which reads the updated |mPosition| member. Then HandleError passes the resulting (out-of-bounds) value to AppendErrorPointer:

954: 
955:   nsAutoString sourceText(mLastLine);
956:   AppendErrorPointer(colNumber, mLastLine.get(), sourceText);

which then reads from the out-of-bounds locations in |mLastLine| (line 868):

856: static nsresult
857: AppendErrorPointer(const int32_t aColNumber,
858:                    const char16_t *aSourceLine,
859:                    nsString& aSourceString)
860: {
861:   aSourceString.Append(char16_t('\n'));
862: 
863:   // Last character will be '^'.
864:   int32_t last = aColNumber - 1;
865:   int32_t i;
866:   uint32_t minuses = 0;
867:   for (i = 0; i < last; ++i) {
868:     if (aSourceLine[i] == '\t') {
869:       // Since this uses |white-space: pre;| a tab stop equals 8 spaces.
870:       uint32_t add = 8 - (minuses % 8);
871:       aSourceString.AppendASCII("--------", add);
872:       minuses += add;
873:     }
874:     else {
875:       aSourceString.Append(char16_t('-'));
876:       ++minuses;
877:     }
878:   }
879:   aSourceString.Append(char16_t('^'));
880: 
881:   return NS_OK;
882: }

Since the read is used only to decide what to put into the error string |aSourceString|, there is no meaningful data leak.


To use the POC, run FF 42.0 Win64 and attach a debugger to it. Set a breakpoint on nsExpatDriver line 1118 with the condition |consumed==0x5f|, then load the POC. When the BP fires, notice how line 1126 updates |mExpatParser.mPosition.col| to 0x4000000d. Then step to line 1139 and notice that |oldLength| is 0x3fffffae, and that the call to SetLength fails, leaving |aDest| with length 0x3fffffae. Step out, and see that |mLastLine| is also thus 0x3fffffae characters long. Step past lines 1160-1182 (which append one more character to mLastLine, which append succeeds because the resulting length doesn't exceeed nsAString's limit), then step into HandleError. Notice |colNumber| at line 946 is 0x4000000e. Step into AppendErrorPointer, noticing that line 864 assigns 0x4000000d to |last|. Examine |aSourceLine [last-1]| and notice it's inaccessible memory (or sometimes uninitialized memory). Let execution proceed and watch the probable read-access violation crash. It might take a minute or two to crash because AppendErrorPointer appends one character at a time to the error message string.

(Note that the POC has the invalid character 0x1a following "</text>", which is necessary to cause the parse error that causes HandleError to be called).

[1] Yes, |XML_GetCurrentColumnNumber(mExpatParser)| updates |mExpatParser.mPosition.col|.

Comment 1

[q1]

(In reply to q1 from comment #0)
> However, there are many other callers of nsScannerString::AppendUnicodeTo, plus there are callers of 
> nsScannerString::CopyUnicodeTo, which has a similar bug. Probably some of these callers malfunction 
> insecurely when AppendUnicodeTo or CopyUnicodeTo fail.

Further investigation shows that an intermittent OOM could cause other callers of CopyUnicodeTo and AppendUnicodeTo incorrectly to use truncated (AppendUnicodeTo) or zero-length (CopyUnicodeTo) strings. Some of the callers (e.g., those in nsScanner) are used to process XML (both from incoming streams and from chrome) and also SVG inside XML documents. [1] So, this bug could cause incorrect interpretation of those streams, including insecure operation, as by, for example, causing the use of truncated URLs that then cause access to unintended sites, the use of truncated or zero-length encryption keys, etc.

[1] Set breakpoints on CopyUnicodeTo and AppendUnicodeTo. Then select help/about and examine aSrcStart.mPosition to see CopyUnicodeTo's involvement in the loading of a chrome XML file. Then visit nytimes.com to see these functions' involvement in the loading of incoming XML and SVG within XML files.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

hsivonen, could we just use infallible SetLength here?

Comment 3

[Henri Sivonen (:hsivonen)]

(In reply to Olli Pettay [:smaug] from comment #2)
> hsivonen, could we just use infallible SetLength here?

As a quick fix, yes. As a proper fix, no. Since Web content can control the size of the allocation making the allocation infallible would still leave a denial-of-service bug. We should propagate the OOM here.

(And hopefully some day, we'll rewrite just about everything XML parsing-related to do buffering more like the HTML parser does it.)

Comment 4

[Henri Sivonen (:hsivonen)]

Attachment: Untested patch

Comment 5

[Henri Sivonen (:hsivonen)]

I'm unable to reproduce the described debugging conditions on 64-bit Linux without the patch. Could someone who can reproduce the debugging conditions from comment 0, please, test with the patch to confirm that the patch behaves reasonably?

Comment 6

[q1]

(In reply to Henri Sivonen (:hsivonen) from comment #5)
> I'm unable to reproduce the described debugging conditions on 64-bit Linux
> without the patch. Could someone who can reproduce the debugging conditions
> from comment 0, please, test with the patch to confirm that the patch
> behaves reasonably?

I can do so on or after 12/31, but not sooner.

Comment 7

[Henri Sivonen (:hsivonen)]

(In reply to q1 from comment #6)
> (In reply to Henri Sivonen (:hsivonen) from comment #5)
> > I'm unable to reproduce the described debugging conditions on 64-bit Linux
> > without the patch. Could someone who can reproduce the debugging conditions
> > from comment 0, please, test with the patch to confirm that the patch
> > behaves reasonably?
> 
> I can do so on or after 12/31, but not sooner.

OK. Thanks.

Comment 8

[Andrew Overholt [:overholt]]

I'm just setting needinfo on q1 so they remember to check Henri's patch when they have a chance. Thanks in advance :)

Comment 9

[q1]

(In reply to Andrew Overholt [:overholt] from comment #8)
> I'm just setting needinfo on q1 so they remember to check Henri's patch when
> they have a chance. Thanks in advance :)

Unfortunately I won't be able to do this for the foreseeable future due to family issues. I'm sorry that I can't finish out this bug, and I hope that you find someone who can.

The POC created a 100% reproducible crash on FF 42.0 Win64 debug (running under Win7 SP1 x64), so it should be relatively simple to repro it and validate the patch.

Comment 10

[Andrew Overholt [:overholt]]

(In reply to q1 from comment #9)
> (In reply to Andrew Overholt [:overholt] from comment #8)
> > I'm just setting needinfo on q1 so they remember to check Henri's patch when
> > they have a chance. Thanks in advance :)
> 
> Unfortunately I won't be able to do this for the foreseeable future due to
> family issues.

Thank you for reporting it and I wish you the best.

Comment 11

[q1]

(In reply to Andrew Overholt [:overholt] from comment #10)
> (In reply to q1 from comment #9)
> > (In reply to Andrew Overholt [:overholt] from comment #8)
> > > I'm just setting needinfo on q1 so they remember to check Henri's patch when
> > > they have a chance. Thanks in advance :)
> > 
> > Unfortunately I won't be able to do this for the foreseeable future due to
> > family issues.
> 
> Thank you for reporting it and I wish you the best.

Thanks!

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

NI myself to test the proposed patch on windows.

Comment 13

[q1]

(In reply to q1 from comment #9)
> The POC created a 100% reproducible crash on FF 42.0 Win64 debug (running
> under Win7 SP1 x64), so it should be relatively simple to repro it and
> validate the patch.

That was a debug build that I built, not a prebuilt one, but it was from unmodified sources, built with a recent Mozillabuild (I don't remember the version) and using VS 2013 (I think).

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

(assigning to hsivonen, since I think all sec-crit/highs should be assigned to someone asap. If you don't have time to look at this more, feel free to assign to me.)

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Trying to load the PoC in a debug nightly build seems to just spin forever.  Lets try an opt build with symbols...

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

I see the browser climb into 5GB to 6GB memory usage and hang, but I don't see a crash.  I also don't see the conditional breakpoint trigger as described in comment 0.

Because I can't reproduce the original problem I have not tried applying the patch yet.

How would you like to proceed?

This was all on 64-bit builds of nightly on windows 8 and visual studio 2013.

Comment 17

[Henri Sivonen (:hsivonen)]

(In reply to Ben Kelly [:bkelly] from comment #16)
> How would you like to proceed?

I suppose I could just ask for review. Not sure what I should do about a try run for testing that this doesn't accidentally break something. If this really exposes a security bug, it seems bad to post the patch to try.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8701030 [details] [diff] [review]
Untested patch

>+++ b/parser/htmlparser/nsParser.cpp
>@@ -1507,17 +1507,19 @@ nsParser::ResumeParse(bool allowIteratio
>                 DidBuildModel(mStreamStatus);
>                 return NS_OK;
>               }
>             } else {
>               CParserContext* theContext = PopContext();
>               if (theContext) {
>                 theIterationIsOk = allowIteration && theContextIsStringBased;
>                 if (theContext->mCopyUnused) {
>-                  theContext->mScanner->CopyUnusedData(mUnusedInput);
>+                  if (!theContext->mScanner->CopyUnusedData(mUnusedInput)) {
>+                    mInternalState = NS_ERROR_OUT_OF_MEMORY;
>+                  }
>                 }
> 
>                 delete theContext;
>               }
> 
>               result = mInternalState;
Ok, this all is fine since we return from the while loop when result isn't NS_OK


>@@ -546,17 +548,19 @@ nsresult nsScanner::ReadTagIdentifier(ns
Looks like ReadTagIdentifier is unused method

>@@ -601,26 +605,30 @@ nsresult nsScanner::ReadEntityIdentifier
Can't find callers for this method either


>@@ -650,26 +658,30 @@ nsresult nsScanner::ReadNumber(nsString&
Hmm, is also ReadNumber unsused

>@@ -716,37 +728,43 @@ nsresult nsScanner::ReadWhitespace(nsSca
and this too
>@@ -850,34 +868,38 @@ nsresult nsScanner::ReadUntil(nsAString&
and this

Comment 19

[q1]

(In reply to Ben Kelly [:bkelly] from comment #16)
> I see the browser climb into 5GB to 6GB memory usage and hang, but I don't
> see a crash.  I also don't see the conditional breakpoint trigger as
> described in comment 0.
> 
> Because I can't reproduce the original problem I have not tried applying the
> patch yet.
> 
> How would you like to proceed?
> 
> This was all on 64-bit builds of nightly on windows 8 and visual studio 2013.

You've got to use FF 42.0 Win64 debug; I don't know what nightly will do. Also, setting conditional breakpoints in release builds almost always doesn't work, because the debugger doesn't know where the variable really lives. Please see the repro instructions in comment 0.

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

I'm confused.  Are you saying this problem does not affect nightly or releases after 42?  We're not going to apply patches to 42 since release channel is already 43.

Comment 21

[q1]

(In reply to Ben Kelly [:bkelly] from comment #20)
> I'm confused.  Are you saying this problem does not affect nightly or
> releases after 42?  We're not going to apply patches to 42 since release
> channel is already 43.

I don't know whether this bug affects FF nightly or releases later than FF 42.0. I found it in FF 42.0 by directed fuzzing, reviewed the code to find the root cause, then wrote and tested the POC (also under FF 42.0, which was the latest release at the time) to demonstrate it. If the sources still contain the root cause (missing status returns as described in comment 0), then that's still a bug. It could be that some change(s) since FF 42.0 causes the bug to be manifested in a different way than it was in 42.0.

Comment 22

[Henri Sivonen (:hsivonen)]

Attachment: Remove dead code

(In reply to Olli Pettay [:smaug] (expect slower than usual review time for couple of days) from comment #18)
> >@@ -546,17 +548,19 @@ nsresult nsScanner::ReadTagIdentifier(ns
> Looks like ReadTagIdentifier is unused method

Good point.

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8710591 [details] [diff] [review]
Remove dead code

Feel free to move this patch to separate bug if it makes easier to track the changes (like, this kind of clean up could be trunk only, but the other fix might land to branches). But whatever is easiest.

Comment 24

[Henri Sivonen (:hsivonen)]

Comment on attachment 8701030 [details] [diff] [review]
Untested patch

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?

The patch makes it very clear what the problem is. It's not clear to me how easy the exploit development would be, but probably as easy as for other writes past the end of the allocated buffer.

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

No, there are none of these.

> Which older supported branches are affected by this flaw?

All.

> If not all supported branches, which bug introduced the flaw?

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

The same patch applies.

> How likely is this patch to cause regressions; how much testing does it need?

Unlikely. Hopefully the new code paths never run except when under attack. Hopefully both smaug and I took good enough look not to change the non-attack flow.

- -

Additionally, should I land attachment 8710591 [details] [diff] [review] under this bug number or land it separately?

Comment 25

[Al Billings [:abillings - ex-MoCo]]

This has sec-approval+ to check into trunk on February 9. We'll want patches nominated for Aurora, Beta, and ESR38 so they can be checked in after this is on trunk.

Comment 26

[Al Billings [:abillings - ex-MoCo]]

> Additionally, should I land attachment 8710591 [details] [diff] [review] [diff] [review] under this bug number or land it separately?

You can land these together.

Comment 27

[Henri Sivonen (:hsivonen)]

Landed with a trivial rebase of the second patch:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5ef9e8af842
https://hg.mozilla.org/integration/mozilla-inbound/rev/0fa3eddf8063

Comment 28

[Henri Sivonen (:hsivonen)]

Follow-up, because gcc and clang differed on which warnings are treated as fatal:
https://hg.mozilla.org/integration/mozilla-inbound/rev/202de45bc0d4

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b5ef9e8af842
https://hg.mozilla.org/mozilla-central/rev/0fa3eddf8063
https://hg.mozilla.org/mozilla-central/rev/202de45bc0d4

Comment 30

[Henri Sivonen (:hsivonen)]

Comment on attachment 8701030 [details] [diff] [review]
Untested patch

> [Approval Request Comment]
> If this is not a sec:{high,crit} bug, please state case for ESR consideration:

sec-high

> User impact if declined: 

Security risk.

> Fix Landed on Version:

47

> Risk to taking this patch (and alternatives if risky): 

Very low. When inputs aren't large enough to cause an out-of-memory condition, nothing should change.

> String or UUID changes made by this patch: 

None.

> See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

> Approval Request Comment
> [Feature/regressing bug #]:

Very old bug.

> [User impact if declined]:

Security risk.

> [Describe test coverage new/current, TreeHerder]:

No test landed, because the problem isn't reliably reproducible and requires very large input.

> [Risks and why]: 

Very low. The only risk is typoing something in the patch.

> [String/UUID change made/needed]:

None.

Comment 31

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8701030 [details] [diff] [review]
Untested patch

Sec-high, taking it.
Should be in 45 beta 5.

Comment 32

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0ecd4deb260c
https://hg.mozilla.org/releases/mozilla-aurora/rev/597e2d10c56f
https://hg.mozilla.org/releases/mozilla-aurora/rev/a6f485514b7c

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This isn't applying cleanly to beta. Can we get rebased patches for it (and likely esr38)?

Comment 34

[Henri Sivonen (:hsivonen)]

(In reply to Wes Kocher (:KWierso) from comment #33)
> This isn't applying cleanly to beta. Can we get rebased patches for it (and
> likely esr38)?

Only attachment 8701030 [details] [diff] [review] needs to be uplifted. Does that one not apply to beta? Back when I tested for comment 24, it did.

Comment 35

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

So it doesn't need the addendum or the bustage followup commits?

That first commit applies cleanly on its own.

Comment 36

[Henri Sivonen (:hsivonen)]

(In reply to Wes Kocher (:KWierso) from comment #35)
> So it doesn't need the addendum or the bustage followup commits?

Correct. The first patch fixes the bug. The second patch cleans up the code and the third patch fixes a warning-as-error condition with the second patch. Therefore, the first patch is sufficient for uplift.

Comment 37

[Carsten Book [:Tomcat]]

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

Comment 38

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Henri, Wes, Carsten: Even though the relevant patch (was it the right rebased one?) was approved for uplift to esr38, I don't think it landed. Could you please confirm?

Comment 39

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-esr38/rev/271e3a5a53d9