1170274 - Memory-safety bug in nsGenericHTMLElement::GetURIListAttr

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:

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.

Comment 1

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

start should be a pointer to gNullChar and gNullChar is 0.

Comment 2

[Andrea Marchesini [:baku]]

Attachment: crash7.patch

Comment 3

[(no longer active)]

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

Comment 4

[Andrea Marchesini [:baku]]

Attachment: crash7.patch

Comment 5

[(no longer active)]

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(' '));|?

Comment 6

[Andrea Marchesini [:baku]]

Attachment: crash7.patch

Comment 7

[Andrew McCreight [:mccr8]]

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.

Comment 8

[Andrea Marchesini [:baku]]

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

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e44f689c3756

Is this worth backporting?

Comment 10

[Andrea Marchesini [:baku]]

It's not sec-critical. I think we can avoid the backporting for this patch. ehsan?

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Backed out for reflection-embedded.html w-p-t failures.
https://treeherder.mozilla.org/logviewer.html#?job_id=10826031&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/85f92f328b52

Comment 12

[Andrea Marchesini [:baku]]

ehsan, we cannot use NS_strtok. Can we go back to my previous patch?

Comment 13

[(no longer active)]

(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?

Comment 14

[Andrea Marchesini [:baku]]

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(' '));

Comment 15

[Andrea Marchesini [:baku]]

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

\0 is a valid char in html5, not a EOS.

Comment 16

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/689ce5819b24

Comment 17

[Andrea Marchesini [:baku]]

I submitted the wrong patch.
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8df8c6f33ef
https://hg.mozilla.org/integration/mozilla-inbound/rev/1da97e961c3d

Comment 18

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b55daa813afd

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1da97e961c3d
https://hg.mozilla.org/mozilla-central/rev/b55daa813afd