Bug 1134561 (CVE-2015-0803)

Type confusion in HTMLSourceElement::AfterSetAttr

RESOLVED FIXED in Firefox 37

Status

()

defect
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: nils, Assigned: longsonr)

Tracking

({sec-critical})

unspecified
mozilla39
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
sec-bounty +
in-testsuite ?

Firefox Tracking Flags

(firefox36 wontfix, firefox37+ fixed, firefox38+ fixed, firefox39+ fixed, firefox-esr31 unaffected, b2g-v1.4 unaffected, b2g-v2.0 unaffected, b2g-v2.0M unaffected, b2g-v2.1 disabled, b2g-v2.1S disabled, b2g-v2.2 disabled, b2g-master fixed)

Details

(Whiteboard: [adv-main37+])

Attachments

(3 attachments, 2 obsolete attachments)

Posted file testcase
The issue can be triggered while setting a specific attribute (srcset,sizes,media,type) of a <source> element. When iterating over the sibling elements in order to find an HTMLImageElement this function does only check the tag name and not the namespace:

nsresult
HTMLSourceElement::AfterSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
                                const nsAttrValue* aValue, bool aNotify)
{
  // If we are associated with a <picture> with a valid <img>, notify it of
  // responsive parameter changes
  nsINode *parent = nsINode::GetParentNode();
  if (aNameSpaceID == kNameSpaceID_None &&
      (aName == nsGkAtoms::srcset ||
       aName == nsGkAtoms::sizes ||
       aName == nsGkAtoms::media ||
       aName == nsGkAtoms::type) &&
      parent && parent->Tag() == nsGkAtoms::picture) {
    nsString strVal = aValue ? aValue->GetStringValue() : EmptyString();
    // Find all img siblings after this <source> and notify them of the change
    nsCOMPtr<nsINode> sibling = AsContent();
    while ( (sibling = sibling->GetNextSibling()) ) {
      if (sibling->Tag() == nsGkAtoms::img) {
        HTMLImageElement *img = static_cast<HTMLImageElement*>(sibling.get());
        if (aName == nsGkAtoms::srcset) {
          img->PictureSourceSrcsetChanged(AsContent(), strVal, aNotify);
        } else if (aName == nsGkAtoms::sizes) {
          img->PictureSourceSizesChanged(AsContent(), strVal, aNotify);
        } else if (aName == nsGkAtoms::media ||
                   aName == nsGkAtoms::type) {
          img->PictureSourceMediaOrTypeChanged(AsContent(), aNotify);
        }
      }
    }


This results in a cast of an object which isn't an HTMLImageElement to this type. The attached testcase demonstrates this behaviour. ASan output attached as well. ASan detects this as a use-after-free issue.
Posted file asan.txt
Blocks: 1134280
Assignee: nobody → longsonr
Posted patch ishtml.txt (obsolete) — Splinter Review
Attachment #8566465 - Flags: review?(jst)
This is going to be sec-critical for the same reason as bug 1134131
Posted patch fix1.patch (obsolete) — Splinter Review
I agree with smaug about removing Tag() completely, but writing that patch will take a while. In the meantime, this patch fixes this issue.
Attachment #8566477 - Flags: review?(bugs)
Comment on attachment 8566477 [details] [diff] [review]
fix1.patch

Sorry, I didn't see the previous patch. I guess we submitted the patch more or less at the same time.
Attachment #8566477 - Attachment is obsolete: true
Attachment #8566477 - Flags: review?(bugs)
Comment on attachment 8566465 [details] [diff] [review]
ishtml.txt

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

Just comparing my patch with yours :)
I let jst the real review of the patch.

::: dom/html/HTMLPictureElement.cpp
@@ +43,5 @@
>  void
>  HTMLPictureElement::RemoveChildAt(uint32_t aIndex, bool aNotify)
>  {
>    // Find all img siblings after this <source> to notify them of its demise
>    nsCOMPtr<nsINode> child = GetChildAt(aIndex);

nsIContent

@@ +46,5 @@
>    // Find all img siblings after this <source> to notify them of its demise
>    nsCOMPtr<nsINode> child = GetChildAt(aIndex);
>    nsCOMPtr<nsIContent> nextSibling;
> +  if (child && child->IsElement() &&
> +      child->AsElement()->IsHTML(nsGkAtoms::source)) {

and remove AsElement/IsElement

::: dom/html/HTMLSourceElement.cpp
@@ +90,3 @@
>      nsString strVal = aValue ? aValue->GetStringValue() : EmptyString();
>      // Find all img siblings after this <source> and notify them of the change
>      nsCOMPtr<nsINode> sibling = AsContent();

nsCOMPtr<nsIContent> sibling = AsContent();

@@ +91,5 @@
>      // Find all img siblings after this <source> and notify them of the change
>      nsCOMPtr<nsINode> sibling = AsContent();
>      while ( (sibling = sibling->GetNextSibling()) ) {
> +      if (sibling->IsElement() &&
> +          sibling->AsElement()->IsHTML(nsGkAtoms::img)) {

and here you can remove IsElement() and AsElement()

@@ +152,4 @@
>      // Find any img siblings after this <source> and notify them
>      nsCOMPtr<nsINode> sibling = AsContent();
>      while ( (sibling = sibling->GetNextSibling()) ) {
> +      if (sibling->IsElement() &&

same here.
Posted patch ishtml.txtSplinter Review
Address review comments
Attachment #8566465 - Attachment is obsolete: true
Attachment #8566465 - Flags: review?(jst)
Attachment #8566491 - Flags: review?(jst)
Attachment #8566491 - Flags: review?(jst) → review+
Comment on attachment 8566491 [details] [diff] [review]
ishtml.txt

[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? This whole area could do with more tests but that seems to be bug 1017878 as far as I can tell.
Attachment #8566491 - Flags: sec-approval?
Sec-approval for checkin on March 10 or later. We're about to ship so we can't take this for 36.
Keywords: sec-critical
Whiteboard: [checkin on 3/10]
Attachment #8566491 - Flags: sec-approval? → sec-approval+
Flags: sec-bounty?
https://hg.mozilla.org/integration/mozilla-inbound/rev/9264aec18c62

Andrea asked me to mention that for the most part, this patch was rendered obsolete by bug 1134280 anyway.
Flags: in-testsuite?
Whiteboard: [checkin on 3/10]
We'll of course need to use the attached patch for the uplifts at least. Robert, can you please request Aurora/Beta approval on this when you get a chance?
Flags: needinfo?(longsonr)
Comment on attachment 8566491 [details] [diff] [review]
ishtml.txt

Approval Request Comment
[Feature/regressing bug #]: bug 870022, bug 1017875 turned the pref on.
[User impact if declined]: exploitable security hole.
[Describe test coverage new/current, TreeHerder]: This whole area could do with more tests but that seems to be bug 1017878 as far as I can tell.
[Risks and why]: Pretty low risk we're just adding extra checks.
[String/UUID change made/needed]: none
Flags: needinfo?(longsonr)
Attachment #8566491 - Flags: approval-mozilla-beta?
Attachment #8566491 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/9264aec18c62
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Attachment #8566491 - Flags: approval-mozilla-beta?
Attachment #8566491 - Flags: approval-mozilla-beta+
Attachment #8566491 - Flags: approval-mozilla-aurora?
Attachment #8566491 - Flags: approval-mozilla-aurora+
Flags: sec-bounty? → sec-bounty+
Whiteboard: [adv-main37+]
Alias: CVE-2015-0803
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.