1170326 - Memory-safety bug in nsHTMLCSSUtils::ParseLength

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[q1]

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:36.0) Gecko/20100101 Firefox/36.0
Build ID: 20150305021524

Steps to reproduce:

nsHTMLCSSUtils::ParseLength (38.0.1\editor\libeditor\nsHTMLCSSUtils.cpp) reads memory it doesn't own by dereferencing an iterator without checking it first.

The problem occurs only if the aString.Length()==0, and is caused by line 660:

660:    c = *iter;

As far as I can see, the worst problem is bug can cause is a crash. Feel free to remove its security classification if you agree.

Comment 1

[(no longer active)]

Andrea, since you love fixing these bugs(!), would you be interested in this one too?  :-)

Comment 2

[Andrea Marchesini [:baku]]

Attachment: crash5.patch

Comment 3

[(no longer active)]

Comment on attachment 8620921 [details] [diff] [review]
crash5.patch

Review of attachment 8620921 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/libeditor/nsHTMLCSSUtils.cpp
@@ +622,2 @@
>  {
> +  if (aString.Length() == 0) {

Nit: IsEmpty()

Comment 4

[(no longer active)]

(In reply to q1 from comment #0)
> As far as I can see, the worst problem is bug can cause is a crash. Feel
> free to remove its security classification if you agree.

Yes, I think you're right, and this is not security sensitive.

Comment 5

[Andrea Marchesini [:baku]]

Attachment: crash5.patch

How easily could an exploit be constructed based on the patch?

Relatively easy I guess. But q1 maybe has a different opinion.

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

Yes.

Which older supported branches are affected by this flaw?

I guess all the branches.

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

Easy to backport.

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

none test, no regressions.

Comment 6

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

Comment 4 from Ehsan says he doesn't think this is a security bug. Comment 5 implies that not only is it a security bug but that it is a sec-high or sec-critical needing security approval to land. I'm confused here about whether this is a security bug and what its rating should be.

Comment 7

[q1]

(In reply to Andrea Marchesini (:baku) from comment #5)
> Created attachment 8621042 [details] [diff] [review]
> crash5.patch
> 
> How easily could an exploit be constructed based on the patch?
> 
> Relatively easy I guess. But q1 maybe has a different opinion.

Hmm, I think you might be correct. If the iterator points to something other than nonaccessible memory, the dereference and assignment on line 660 won't crash. At that point, i == 0 and j == 0. If c turns out to contain a '-' or '+', however, i gets incremented to 1 by lines 661 or 662, respectively. Control skips the long loop, *aValue is computed as 0, but, and here's the bad thing, line 687 executes:

687:  *aUnit = NS_NewAtom(StringTail(aString, j-i)).take();

j-i is 0-1, so we get a call to StringTail (aString, -1). That seems like it could do something bad, perhaps beyond merely crashing. I'll try to trace it down today.

Comment 8

[(no longer active)]

(Putting the bug back into the security group until everyone agrees that it's not a security bug. ;-)

(In reply to q1 from comment #7)
> (In reply to Andrea Marchesini (:baku) from comment #5)
> > Created attachment 8621042 [details] [diff] [review]
> > crash5.patch
> > 
> > How easily could an exploit be constructed based on the patch?
> > 
> > Relatively easy I guess. But q1 maybe has a different opinion.
> 
> Hmm, I think you might be correct. If the iterator points to something other
> than nonaccessible memory, the dereference and assignment on line 660 won't
> crash. At that point, i == 0 and j == 0. If c turns out to contain a '-' or
> '+', however, i gets incremented to 1 by lines 661 or 662, respectively.
> Control skips the long loop, *aValue is computed as 0, but, and here's the
> bad thing, line 687 executes:
> 
> 687:  *aUnit = NS_NewAtom(StringTail(aString, j-i)).take();
> 
> j-i is 0-1, so we get a call to StringTail (aString, -1). That seems like it
> could do something bad, perhaps beyond merely crashing. I'll try to trace it
> down today.

Right, but that will only cause you to read 1 character past the end of the buffer: <https://dxr.allizom.org/mozilla-central/source/xpcom/string/nsTDependentSubstring.h#121>

I still don't think this is a security bug since we don't make branching decisions and the like based on the possibly garbage memory that we read.

Comment 9

[q1]

Does anyone know how I can easily trigger a call to nsHTMLCSSUtils::ParseLength ? I'd like to doctor the arguments in the debugger and see what happens.

Comment 10

[(no longer active)]

(In reply to q1 from comment #9)
> Does anyone know how I can easily trigger a call to
> nsHTMLCSSUtils::ParseLength ? I'd like to doctor the arguments in the
> debugger and see what happens.

1. Open |data:text/html,<div contenteditable><img width=100 height=100 src="https://bugzilla.mozilla.org/extensions/BMO/web/images/favicon.ico"></div>|.
2. Click on the image, and try to resize using the resize handles.

That should ParseLength around here, I would expect <https://dxr.allizom.org/mozilla-central/source/editor/libeditor/nsHTMLObjectResizer.cpp#257>.

Comment 11

[q1]

Doctoring aString in the debugger by changing its first character to a '+' and its mLength to 0 yields a call to

nsTDependentSubstring_CharT(const substring_type& aStr, uint32_t aStartPos,
                              uint32_t aLength = size_type(-1))
with arguments (aStr, 1, 0xffffffff (-1))

(note that nsTDependentSubstring_CharT coincidentally appears to be prepared to receive aLength == -1).

That devolves to a call to

void
nsTDependentSubstring_CharT::Rebind(const substring_type& str,
                                    uint32_t startPos, uint32_t length)

with arguments (aStr, 1, 0xffffffff)

The call to Finalize on line 12 appears to do nothing. The remainder of the function then sets mData == str .mData and calculates mLength == 0, does SetDataFlags(F_NONE), and returns to ParseLength line 686.

This looks OK, from my outsider perspective, though it also seems like something of a coincidence that it is.

Comment 12

[q1]

> 1. Open |data:text/html,<div contenteditable><img width=100 height=100
> src="https://bugzilla.mozilla.org/extensions/BMO/web/images/favicon.ico"></
> div>|.

BTW, I had not known about data: . It's nifty.

Comment 13

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

Comment on attachment 8621042 [details] [diff] [review]
crash5.patch

Clearing sec-approval until we know if it is actually a security bug that needs sec-approval. Since only high or critical rated security issues do, I suspect that it won't.

Comment 14

[Andrea Marchesini [:baku]]

Also if this is not a security bug, I think would be nice to land my 3-lines patch. It makes the code more easy to understand and it avoid to do several non-needed steps.

Comment 15

[(no longer active)]

Thanks to q1 for looking at this under the debugger.  Based on comment 11, and my own code inspection, I can fairly confidently say that this is not a security bug.  Opening the bug up.

Andrea, please feel free to land your patch!

Comment 16

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f1617a11f15b

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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