1134131 - Type confusion with <picture> and <source> elements

Status: RESOLVED FIXED

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

Description

[Nils]

Attachment: testcase

The following code in HTMLImageElement::TryCreateResponsiveSelector is vulnerable to a type confusion issue:

  bool isSourceTag = aSourceNode->Tag() == nsGkAtoms::source;
  if (isSourceTag) {
    DebugOnly<nsINode *> parent(nsINode::GetParentNode());
    MOZ_ASSERT(parent && parent->Tag() == nsGkAtoms::picture);
    MOZ_ASSERT(IsPreviousSibling(aSourceNode, this));
    MOZ_ASSERT(pictureEnabled);

    // Check media and type
    HTMLSourceElement *src = static_cast<HTMLSourceElement*>(aSourceNode);
    if (!src->MatchesCurrentMedia()) {
      return false;
    }

The code only checks whether aSourceNode has the tag name "source" without validating the namespace. The attached testcase demonstrates the issues.

Comment 1

[Nils]

Attachment: ASAN output on crash

Comment 2

[Robert Longson [:longsonr]]

Attachment: Possible fix?

Comment 3

[Robert Longson [:longsonr]]

Comment on attachment 8565890 [details] [diff] [review]
Possible fix?

Are you able to test whether this patch fixes the vulnerability?

Comment 4

[Nils]

I can confirm that the testcase doesn't crash in an ASAN build with the patch applied.

Comment 5

[Boris Zbarsky [:bzbarsky]]

[Tracking Requested - why for this release]: Security bug.

This affects 33 and later; we may want to backport this to aurora/beta...

Comment 6

[Johnny Stenback (:jst)]

Comment on attachment 8565890 [details] [diff] [review]
Possible fix?

Looks good, r=jst

Comment 7

[Robert Longson [:longsonr]]

Comment on attachment 8565890 [details] [diff] [review]
Possible fix?

[Security approval request comment]
How easily could an exploit be constructed based on the patch? You'd know you needed to create something using a picture tag in a non-html namespace but not exactly how to do that.
Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? no.

Which older supported branches are affected by this flaw? Firefox 33 onwards preffed off, only preffed on in Firefox 38

If not all supported branches, which bug introduced the flaw? bug 870022, bug 1017875 turned the pref on.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? I think the patch will apply to all branches. 

How likely is this patch to cause regressions; how much testing does it need? The ASAN test has been rerun on it and no longer fails. This whole area could do with more tests but that seems to be bug 1017878 as far as I can tell.

Comment 8

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

This needs a security rating before it can be approved. It isn't clear to me what the effects of this are on the end use and the risks. Is the crash exploitable? ASAN calls it a heap-buffer-overflow but it does that a lot.

Comment 9

[Nils]

I would suspect this is exploitable. The type confusion results in a heap buffer overflow. By confusing an Element (smaller) type object with a HTMLSourceElement (larger) object we end up reading past the original object, reading a pointer which might be controlled by an attacker. Looking at the crash in windbg suggests an exploitable condition as well:

5758e15c 8b4314          mov     eax,dword ptr [ebx+14h] ds:002b:5a5a5a6e=????????

An object pointer is read from freed memory, the code then continues working with this bogus object pointer.

Comment 10

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

Comment on attachment 8565890 [details] [diff] [review]
Possible fix?

sec-approval+ for trunk checkin.

Comment 11

[Robert Longson [:longsonr]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=08efd42d1664

Comment 12

[Robert Longson [:longsonr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/67ea8244ccf3

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/67ea8244ccf3

Comment 14

[Nils]

Comment on attachment 8565890 [details] [diff] [review]
Possible fix?

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

Feedback already given in previous comment.