Memory-safety bug in nsGenericHTMLElement::GetURIListAttr

RESOLVED FIXED in Firefox 41

Status

()

Core
DOM
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: q1, Assigned: baku)

Tracking

({sec-other})

38 Branch
mozilla41
sec-other
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox41 fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main41-])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

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

Steps to reproduce:

nsGenericHTMLElement::GetURIListAttr (38.0.1\dom\html\nsGenericHTMLElement.cpp) can reference memory that it does not own, dereferencing an iterator that could == end without first checking.

The bug is on line 1739:

1739:      while (*start == ' ' && start < iter)
1740         ++start;

If the string "value" is empty, start == iter == end, and *start examines memory that the function does not own.

As far as I can tell from examining the rest of the function, the worst consequence of this bug is a crash, but I am tagging this as a security bug because I could have missed something that would cause a problem.
Component: Untriaged → DOM
Product: Firefox → Core

Comment 1

3 years ago
start should be a pointer to gNullChar and gNullChar is 0.
(Assignee)

Comment 2

3 years ago
Created attachment 8621000 [details] [diff] [review]
crash7.patch
Assignee: nobody → amarchesini
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #8621000 - Flags: review?(ehsan)

Comment 3

3 years ago
Comment on attachment 8621000 [details] [diff] [review]
crash7.patch

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

Here I would also like to see a proper tokenizer (perhaps using NS_strtok -- I think that should be enough here...)
Attachment #8621000 - Flags: review?(ehsan) → review-
(Assignee)

Comment 4

3 years ago
Created attachment 8622425 [details] [diff] [review]
crash7.patch
Attachment #8621000 - Attachment is obsolete: true
Attachment #8622425 - Flags: review?(ehsan)

Comment 5

3 years ago
Comment on attachment 8622425 [details] [diff] [review]
crash7.patch

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

::: dom/html/nsGenericHTMLElement.cpp
@@ +1736,5 @@
> +  char* buffer = str.BeginWriting();
> +
> +  while (char* token = NS_strtok(" ", &buffer)) {
> +    if (!aResult.IsEmpty()) {
> +      aResult.Append(NS_LITERAL_STRING(" "));

Nit: couldn't you leave this as |aResult.Append(char16_t(' '));|?
Attachment #8622425 - Flags: review?(ehsan) → review+
(Assignee)

Comment 6

3 years ago
Created attachment 8622532 [details] [diff] [review]
crash7.patch
Attachment #8622425 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
This needs a security rating before it can land. Andrea, do you agree with comment 0 that says this does not seem like a security issue and this is just a crash? In that case we can rate this sec-other and it can land.
Flags: needinfo?(amarchesini)
Keywords: checkin-needed
(Assignee)

Comment 8

3 years ago
(In reply to Andrew McCreight [:mccr8] from comment #7)
> This needs a security rating before it can land. Andrea, do you agree with
> comment 0 that says this does not seem like a security issue and this is
> just a crash? In that case we can rate this sec-other and it can land.

Yes, I agree. We can rate it sec-other.
Flags: needinfo?(amarchesini)
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/e44f689c3756

Is this worth backporting?
Flags: needinfo?(amarchesini)
Flags: in-testsuite?
Keywords: checkin-needed → sec-other
(Assignee)

Comment 10

3 years ago
It's not sec-critical. I think we can avoid the backporting for this patch. ehsan?
Flags: needinfo?(amarchesini) → needinfo?(ehsan)
(Assignee)

Comment 12

3 years ago
ehsan, we cannot use NS_strtok. Can we go back to my previous patch?
Flags: needinfo?(ehsan)

Comment 13

3 years ago
(In reply to Andrea Marchesini (:baku) from comment #12)
> ehsan, we cannot use NS_strtok. Can we go back to my previous patch?

Why not?
(Assignee)

Comment 14

3 years ago
Comment on attachment 8621000 [details] [diff] [review]
crash7.patch

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

::: dom/html/nsGenericHTMLElement.cpp
@@ +1753,5 @@
> +      ++iter;
> +    }
> +
> +    if (!aResult.IsEmpty()) {
> +      aResult.Append(NS_LITERAL_STRING(" "));

aResult.Append(char16_t(' '));
(Assignee)

Comment 15

3 years ago
Comment on attachment 8621000 [details] [diff] [review]
crash7.patch

\0 is a valid char in html5, not a EOS.
Attachment #8621000 - Flags: review- → review?(ehsan)
(Assignee)

Updated

3 years ago
Attachment #8622532 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8621000 - Attachment is obsolete: false

Updated

3 years ago
Attachment #8621000 - Flags: review?(ehsan) → review+
https://hg.mozilla.org/mozilla-central/rev/1da97e961c3d
https://hg.mozilla.org/mozilla-central/rev/b55daa813afd
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Whiteboard: [post-critsmash-triage]

Updated

3 years ago
Group: core-security → core-security-release
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main41-]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.