1170246 - (CVE-2015-4521) Memory-safety bugs in ConvertDialogOptions

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, 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:

ConvertDialogOptions (38.0.1\dom\base\nsGlobalWindow.cpp) routinely references memory that it does not own, referencing iterators that could == end without first checking. See lines 9071, 9088, 9092, 9102, 9108, etc., which use expressions of the form:

    while (nsCRT::IsAsciiSpace(*iter) && iter != end) {...

This function can also enter a long loop. If the aOptions string contains the single character " ", followed in memory by ";", the function will run well beyond the end of the aOptions argument, parsing data it does not own until it access-violates. Lines 9071-73 skip the space and increment iter to == end. Lines from 9074-91 do nothing. Then the if statement on lines 9092-97 dereferences iter, finds that *iter is ";", increments iter so that it is no longer == end, and continues the outer while (iter != end) loop (line 9069), which then runs until the code increments iter to point to an inaccessible area of memory and dereferences it, causing an access violation.

This can occur because aOptions isa nsAString, which is not necessarily zero-terminated according to https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Glue_classes/nsAString/BeginReading .

I think the worst consequence of these bugs is a crash, but I can't rule out a compromise of sensitive data and/or overwriting of unowned portions of Firefox's address space.

Comment 1

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

Code from Bug 194404.

Comment 2

[Andrea Marchesini [:baku]]

Attachment: crash6.patch

Comment 3

[Andrea Marchesini [:baku]]

Attachment: crash6.patch

#undef added.

Comment 4

[(no longer active)]

Comment on attachment 8620988 [details] [diff] [review]
crash6.patch

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

While this patch probably fixes the issue, it still does ad-hoc token recognition, so it's still pretty error prone.

Can you instead try to tokenize the string properly?  Basically, I would like to see this rewritten to use a function like |bool GetNextToken(nsAString& token, nsAString::const_iterator& iter, nsAString::const_iterator end)| or some such, which updates its first argument to the next token, if any and returns false when it runs out of tokens.  You can keep the logic to skip the whitespace around tokens inside that tokenizer function.

::: dom/base/nsGlobalWindow.cpp
@@ +9231,1 @@
>  ConvertDialogOptions(const nsAString& aOptions, nsAString& aResult)

Nit: please mark this function as static.

Comment 5

[Andrea Marchesini [:baku]]

Attachment: crash6.patch

Comment 6

[Andrea Marchesini [:baku]]

Attachment: crash6.patch

Comment 7

[(no longer active)]

Comment on attachment 8621547 [details] [diff] [review]
crash6.patch

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

::: dom/base/nsGlobalWindow.cpp
@@ +9234,5 @@
> +  if (aIter == aEnd) {                                   \
> +    return false;                                        \
> +  }
> +
> +  SKIP_ASCII_SPACES

Hmm, you don't need to define a macro for something that you only need to do once.  ;-)

@@ +9252,5 @@
> +    ++aIter;
> +  }
> +
> +  aToken.Assign(Substring(start, aIter));
> +  return true;

There is a bug here, if the string contains some trailing whitespace.  aIter would get to aEnd, then you'd try to run the second while loop but the body of the loop won't execute, so you would assign aToken to an empty string and return true.

I think a similar situation can happen if the aIter points to ';' for example.  Basically anything that would prevent the body of the second loop from getting executed.

@@ +9287,5 @@
> +      break;
> +    }
> +
> +    // No value found, skip the ';' and keep going.
> +    if (*iter == ';') {

This condition will never match, because of |name.EqualsLiteral(";")| above.

Comment 8

[Andrea Marchesini [:baku]]

Attachment: crash6.patch

Comment 9

[(no longer active)]

Comment on attachment 8622382 [details] [diff] [review]
crash6.patch

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

r=me, but this code is still scary, at least in term of future modifications.  Do you mind adding a gtest for the ConvertDialogOptions function, making sure that it can handle any edge casey input in a sane way?

Thanks!

Comment 10

[Andrea Marchesini [:baku]]

Attachment: crash6.patch

gtest included

Comment 11

[(no longer active)]

Comment on attachment 8622963 [details] [diff] [review]
crash6.patch

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

Thanks, this is great!

Comment 12

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3b73bf339093

Comment 13

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

This should have had a security rating and sec-approval+ before landing (unless it was a sec-moderate or lower).

Can you please set sec-approval? and answer the questions in the form and/or rate this?

Security bugs don't just get checked in without ratings due to https://wiki.mozilla.org/Security/Bug_Approval_Process

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3b73bf339093

Comment 15

[Andrea Marchesini [:baku]]

Comment on attachment 8622963 [details] [diff] [review]
crash6.patch

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

I don't know if this code can be used to exploit ff but it should be relatively easy to reproduce the issue.

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

Yes. the bug is well commented.

Which older supported branches are affected by this flaw?

All the branches. The code is there since 2007.

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

It should be easy enough, maybe excluding the gtests.

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

The patch is fully tested with gtests.

Comment 16

[Daniel Veditz [:dveditz]]

This would almost certainly always be an access violation crash. If the memory it reads into happens to be valid options arguments that can affect the popup, but you could do anything the page doesn't already have permission to do. You could leak some information but only to the extent the bytes you read into happen to be valid arguments that could be tested for (e.g. screen size).

Comment 17

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

Sec-approval doesn't matter now since it has been rated sec-low but I wanted to comment on the template answers:

(In reply to Andrea Marchesini (:baku) from comment #15)

> Do comments in the patch, the check-in comment, or tests included in the
> patch paint a bulls-eye on the security problem?
> 
> Yes. the bug is well commented.

 That isn't the intent of the question. The question is whether the patch itself, the checkin comment for it, or included tests will make the security problem this bug fixes really obvious. :-)