Memory-safety bug in nsContentUtils::ParseLegacyFontSize

RESOLVED FIXED in Firefox 41

Status

()

Core
DOM
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: q1, Assigned: baku)

Tracking

({csectype-bounds})

38 Branch
mozilla41
csectype-bounds
Points:
---
Bug Flags:
sec-bounty -

Firefox Tracking Flags

(firefox41 fixed)

Details

(Whiteboard: [post-critsmash-triage])

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
User Agent: Mozilla/5.0 (Windows; rv:***) Gecko/20100101 Firefox/**.*
Build ID: 20150305021524

Steps to reproduce:

nsContentUtils::ParseLegacyFontSize (38.0.1\dom\base\nsContentUtils.cpp) can reference one character of memory that it does not own if the argument aValue is malformed. aValue is a font-size attribute from an HTML page being parsed, and is thus under external control. If the attribute begins with a + or -, and contains nothing else, line 1624 references the first character beyond the end of aValue:

  ...
1609:  if (iter == end) {
1610:    return 0;
1611:  }
1612:
1613:  bool relative = false;
1614:  bool negate = false;
1615:  if (*iter == char16_t('-')) {
1616:    relative = true;
1617:    negate = true;
1618:    ++iter;
1619:  } else if (*iter == char16_t('+')) {
1620:    relative = true;
1621:    ++iter;
1622:  }
1623:
1624:  if (*iter < char16_t('0') || *iter > char16_t('9')) {
1625:    return 0;
1626:  }

The only result is that the user's browser might crash; the rest of the function prevents anything worse from happening.

I have tagged this as a security bug because it might suggest to look for other things of similar kind; feel free to change its classification as needed.
Status: UNCONFIRMED → NEW
Component: Untriaged → DOM
Ever confirmed: true
Flags: sec-bounty?
Product: Firefox → Core
(Assignee)

Comment 1

3 years ago
Created attachment 8613423 [details] [diff] [review]
crash.patch
Attachment #8613423 - Flags: review?(dveditz)
(Assignee)

Updated

3 years ago
Assignee: nobody → amarchesini
Comment on attachment 8613423 [details] [diff] [review]
crash.patch

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

r=dveditz
Attachment #8613423 - Flags: review?(dveditz) → review+
Keywords: csectype-bounds, sec-low
https://hg.mozilla.org/mozilla-central/rev/5dd4d3360258
status-firefox41: --- → fixed
Target Milestone: --- → mozilla41
(Assignee)

Comment 5

3 years ago
Should this bug be marked as resolved?
Flags: needinfo?(ryanvm)
Yes.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Flags: needinfo?(ryanvm)
Resolution: --- → FIXED
Given that you can only read 1 byte with this issue, is there anything you can actually do here?
(Reporter)

Comment 8

3 years ago
(In reply to [On PTO until 8/24. Call :dveditz for help] from comment #7)
> Given that you can only read 1 byte with this issue, is there anything you
> can actually do here?

It doesn't appear so. If the unowned character is < '0' || > '9', the function returns 0 (which appears, from e.g., line 1610, to be a valid return value, and presumably causes the specified text's size to be changed accordingly). Otherwise, the remaining code in the function (not shown) checks for iter == end (which it will be in this case), and returns a value in the range [1..7]. I think the worst that can happen is an access-violation crash if the unowned character happens to live in a page that disallows usermode read access, or mis-sized text (but caused by a defect in the page, so not significant).
Whiteboard: [post-critsmash-triage]
Flags: sec-bounty? → sec-bounty-
Group: core-security
Keywords: sec-low
You need to log in before you can comment on or make changes to this bug.