1169800 - Memory-safety bug in nsContentUtils::ParseLegacyFontSize

Status: RESOLVED FIXED

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

Description

[q1]

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.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: crash.patch

Comment 2

[Daniel Veditz [:dveditz]]

Comment on attachment 8613423 [details] [diff] [review]
crash.patch

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

r=dveditz

Comment 3

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5dd4d3360258

Comment 4

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

https://hg.mozilla.org/mozilla-central/rev/5dd4d3360258

Comment 5

[Andrea Marchesini [:baku]]

Should this bug be marked as resolved?

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Yes.

Comment 7

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

Given that you can only read 1 byte with this issue, is there anything you can actually do here?

Comment 8

[q1]

(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).