nsNodeUtils::CloneAndAdopt() incorrectly handles ReparentWrapper failure

RESOLVED FIXED in Firefox 49

Status

()

defect
RESOLVED FIXED
3 years ago
a month ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

(Blocks 1 bug, {sec-high})

Trunk
mozilla51
Points:
---

Firefox Tracking Flags

(firefox48 wontfix, firefox49+ fixed, firefox-esr4549+ fixed, firefox50+ fixed, firefox51 fixed)

Details

(Whiteboard: [adv-main49+][adv-esr45.4+])

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
Boiled down a bit, the code looks like this:

  NodeInfo *nodeInfo = aNode->mNodeInfo;
  RefPtr<NodeInfo> newNodeInfo;
  if (nodeInfoManager) {
    ...
    newNodeInfo = // create a new node info
    nodeInfo = newNodeInfo;
  }
  ...
  if (...) {
    ...
  } else if (nodeInfoManager) {
    ...
    aNode->mNodeInfo.swap(newNodeInfo);
    ...
    if (aReparentScope) {
      ...
      if (...) {
        ...
        if (NS_FAILED(rv)) {
          aNode->mNodeInfo.swap(nodeInfo);
          return rv;
        }
      }
    }
  }

After the first swap, aNode->mNodeInfo and nodeInfo both contain the new node (I'm assuming no method we call changed aNode->mNodeInfo), while newNodeInfo contains the original node.

The second swap is bogus: it is swapping a refcounted value and a raw pointer (it seems unfortunate that this is legal...). If these were not equal, you'd have a bad refcount problem (aNode->mNodeInfo would underflow its refcount when it was destroyed, and nodeInfo would leak), but it looks like they are the same, so we don't leak any objects. 

Presumably the second swap is supposed to be newNodeInfo instead of nodeInfo.

As far as I can tell there's no security issue here, but given that wrappers and refcounting are involved I wanted to get a second opinion before making this bug public.
(Assignee)

Comment 1

3 years ago
For what it is worth, this bug appears to have been introduced in bug 347524:
https://github.com/mozilla/gecko-dev/commit/64ec124ee5210d7673618c26900c8abd56bdd28b
Ugh.  I can't tell whether there's a security issue, but we end up with a node which is in one document but has a wrapper (and hence principal, via the wrapper!) from another document.  I can't guarantee that this won't cause issues, especially when document.domain is involved.
(Assignee)

Updated

3 years ago
Assignee: nobody → continuation
(Assignee)

Updated

3 years ago
Blocks: 1287222
Group: core-security → dom-core-security
(Assignee)

Comment 3

3 years ago
I'll just be conservative per comment 2 and mark this sec-high. It might be difficult to exploit in practice (I think it requires an OOM at a precise point) but it seems like if somebody is very clever it could go very wrong. Honestly we might be better off just crashing here instead of trying to recover but whatever.

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b856020e804e
Keywords: sec-auditsec-high
(Assignee)

Comment 4

3 years ago
I'm not sure if we should land this now or just wait until the next beta, but I'll see what Al thinks.
Attachment #8773353 - Flags: review?(bzbarsky)
Comment on attachment 8773353 [details] [diff] [review]
Fix typo in nsNodeUtils::CloneAndAdopt().

r=me
Attachment #8773353 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 6

3 years ago
Comment on attachment 8773353 [details] [diff] [review]
Fix typo in nsNodeUtils::CloneAndAdopt().

[Security approval request comment]
How easily could an exploit be constructed based on the patch? It would be hard to trigger reliably, and I'm not sure how easily this would lead to an exploit, but the problem is fairly clear from the patch.

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? this bug is around 10 years old

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? should be the same

How likely is this patch to cause regressions; how much testing does it need? It shouldn't be too risky. If we get in this state, bad things probably happen right now, so this can only be better.
Attachment #8773353 - Flags: sec-approval?
This is too late for the current release cycle so it will have to land two weeks into the next. I'm giving it sec-approval+ for checkin on August 16 (not before). We'll want patches for affected branches made and nominated at the same time as well.
Whiteboard: [checkin on 8/16]
Attachment #8773353 - Flags: sec-approval? → sec-approval+
I agree we shouldn't land this cycle, but I don't see a need to wait once the tree reopens.
Whiteboard: [checkin on 8/16] → [checkin after 8/2]
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
Whiteboard: [checkin after 8/2] → [checkin on 8/2]
https://hg.mozilla.org/mozilla-central/rev/6e875e1462fc
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
(Assignee)

Comment 11

3 years ago
Comment on attachment 8773353 [details] [diff] [review]
Fix typo in nsNodeUtils::CloneAndAdopt().

Approval Request Comment
[Feature/regressing bug #]: very old
[User impact if declined]: sec-high
[Describe test coverage new/current, TreeHerder]: this is an error case, so it probably is not tested
[Risks and why]: low, this just fixes one rare error case
[String/UUID change made/needed]: none
Attachment #8773353 - Flags: approval-mozilla-esr45?
Attachment #8773353 - Flags: approval-mozilla-beta?
Attachment #8773353 - Flags: approval-mozilla-aurora?
Attachment #8773353 - Flags: approval-mozilla-esr45?
Attachment #8773353 - Flags: approval-mozilla-esr45+
Attachment #8773353 - Flags: approval-mozilla-beta?
Attachment #8773353 - Flags: approval-mozilla-beta+
Attachment #8773353 - Flags: approval-mozilla-aurora?
Attachment #8773353 - Flags: approval-mozilla-aurora+
Group: dom-core-security → core-security-release
Whiteboard: [adv-main49+][adv-esr45.4+]
Group: core-security-release
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.