Andrew McCreight [:mccr8] Assignee
	Description • 8 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.

	Andrew McCreight [:mccr8] Assignee
	Comment 1 • 8 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

	Boris Zbarsky [:bzbarsky]
	Comment 2 • 8 years ago

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.

	Andrew McCreight [:mccr8] Assignee
	Comment 3 • 8 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

	Andrew McCreight [:mccr8] Assignee
	Comment 4 • 8 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.

	Boris Zbarsky [:bzbarsky]
	Comment 5 • 8 years ago

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

r=me

	Andrew McCreight [:mccr8] Assignee
	Comment 6 • 8 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.

	Al Billings [:abillings - ex-MoCo]
	Comment 7 • 8 years ago

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.

	Daniel Veditz [:dveditz]
	Comment 8 • 8 years ago

I agree we shouldn't land this cycle, but I don't see a need to wait once the tree reopens.

	Carsten Book [:Tomcat]
	Comment 9 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/6e875e1462fc68ee5deef2ce1923d4e351982d03

	Carsten Book [:Tomcat]
	Comment 10 • 8 years ago

https://hg.mozilla.org/mozilla-central/rev/6e875e1462fc

	Andrew McCreight [:mccr8] Assignee
	Comment 11 • 8 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

	Carsten Book [:Tomcat]
	Comment 12 • 8 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/bd7ac9bc5d7b8285929c956025ee6bd30221fea6

	Carsten Book [:Tomcat]
	Comment 13 • 8 years ago

https://hg.mozilla.org/releases/mozilla-beta/rev/cab91bc0d960

	Carsten Book [:Tomcat]
	Comment 14 • 8 years ago

https://hg.mozilla.org/releases/mozilla-esr45/rev/a49fd7eb57ba