Closed Bug 1287204 Opened 8 years ago Closed 8 years ago

nsNodeUtils::CloneAndAdopt() incorrectly handles ReparentWrapper failure


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

Not set



Tracking Status
firefox48 --- wontfix
firefox49 + fixed
firefox-esr45 49+ fixed
firefox50 + fixed
firefox51 --- fixed


(Reporter: mccr8, Assigned: mccr8)


(Keywords: sec-high, Whiteboard: [adv-main49+][adv-esr45.4+])


(1 file)

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) {
    if (aReparentScope) {
      if (...) {
        if (NS_FAILED(rv)) {
          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.
For what it is worth, this bug appears to have been introduced in bug 347524:
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: nobody → continuation
Group: core-security → dom-core-security
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:
Keywords: sec-auditsec-high
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().

Attachment #8773353 - Flags: review?(bzbarsky) → review+
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]
Keywords: checkin-needed
Whiteboard: [checkin after 8/2] → [checkin on 8/2]
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
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
You need to log in before you can comment on or make changes to this bug.