Closed Bug 1170246 (CVE-2015-4521) Opened 5 years ago Closed 5 years ago

Memory-safety bugs in ConvertDialogOptions

Categories

(Core :: DOM: Core & HTML, defect)

38 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: q1, Assigned: baku)

Details

(Keywords: csectype-bounds, sec-low, Whiteboard: [post-critsmash-triage][adv-main41+])

Attachments

(1 file, 5 obsolete files)

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.
Version: 36 Branch → 38 Branch
Component: Untriaged → DOM
Product: Firefox → Core
Code from Bug 194404.
Attached patch crash6.patch (obsolete) — Splinter Review
Attachment #8620981 - Flags: review?(ehsan)
Assignee: nobody → amarchesini
Attached patch crash6.patch (obsolete) — Splinter Review
#undef added.
Attachment #8620981 - Attachment is obsolete: true
Attachment #8620981 - Flags: review?(ehsan)
Attachment #8620988 - Flags: review?(ehsan)
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.
Attachment #8620988 - Flags: review?(ehsan) → review-
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch crash6.patch (obsolete) — Splinter Review
Attachment #8620988 - Attachment is obsolete: true
Attachment #8621546 - Flags: review?(ehsan)
Attached patch crash6.patch (obsolete) — Splinter Review
Attachment #8621546 - Attachment is obsolete: true
Attachment #8621546 - Flags: review?(ehsan)
Attachment #8621547 - Flags: review?(ehsan)
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.
Attachment #8621547 - Flags: review?(ehsan) → review-
Attached patch crash6.patch (obsolete) — Splinter Review
Attachment #8621547 - Attachment is obsolete: true
Attachment #8622382 - Flags: review?(ehsan)
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!
Attachment #8622382 - Flags: review?(ehsan) → review+
Attached patch crash6.patchSplinter Review
gtest included
Attachment #8622382 - Attachment is obsolete: true
Attachment #8622963 - Flags: review?(ehsan)
Comment on attachment 8622963 [details] [diff] [review]
crash6.patch

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

Thanks, this is great!
Attachment #8622963 - Flags: review?(ehsan) → review+
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
Flags: needinfo?(amarchesini)
https://hg.mozilla.org/mozilla-central/rev/3b73bf339093
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
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.
Flags: needinfo?(amarchesini)
Attachment #8622963 - Flags: sec-approval?
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).
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. :-)
Attachment #8622963 - Flags: sec-approval?
Whiteboard: [post-critsmash-triage]
Group: core-security → core-security-release
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main41+]
Alias: CVE-2015-4521
Group: core-security-release
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.