849791 - (CVE-2013-1695) Frame DocShells do not inherit sandbox flags from their parents.

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Bob Owen (:bobowen)]

At the moment the sandbox flags aren't copied to a new frame's DocShell.
So, if someone has a site that other people are hosting in a sandboxed iframe, they can just add a frame to their site and they're suddenly not sandboxed at all.

I believe the problem is in content/base/src/nsFrameLoader.cpp

  // Is this an <iframe> with a sandbox attribute or a parent which is
  // sandboxed ?
  HTMLIFrameElement* iframe =
    HTMLIFrameElement::FromContent(mOwnerContent);

  uint32_t sandboxFlags = 0;

  if (iframe) {
    sandboxFlags = iframe->GetSandboxFlags();

    uint32_t parentSandboxFlags = iframe->OwnerDoc()->GetSandboxFlags();

    if (sandboxFlags || parentSandboxFlags) {
      // The child can only add restrictions, not remove them.
      sandboxFlags |= parentSandboxFlags;

      mDocShell->SetSandboxFlags(sandboxFlags);
    }
  }


It only checks to see if the parent is sandboxed, if it is an iframe element.

I haven't got time to add a test case at the moment, I'll try and add one later.

Comment 1

[Ian Melven :imelven]

Marking as sec-low based on other iframe sandbox bugs. Open to discussion, as always.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Testcase:

test.html:

   <iframe sandbox="allow-same-origin" src="foo.html"></iframe>

foo.html:

   <frameset> <frame src="baz.html"> </frameset>

baz.html:

   <script> alert('FAIL'); </script>

Comment 3

[Ian Melven :imelven]

Attachment: patch

This fixes bz's test case. I've been working on tests but they're not quite finished yet. I'm trying to avoid making them work like the testcase since then we're checking a script didn't run, which is maybe a bit racey.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 725030 [details] [diff] [review]
patch

r=me

Comment 5

[Ian Melven :imelven]

Attachment: tests

Tests.

Comment 6

[Ian Melven :imelven]

https://tbpl.mozilla.org/?tree=Try&rev=752dc609077b

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 725104 [details] [diff] [review]
tests

r=me assuming these fail without the patch.

Comment 8

[Ian Melven :imelven]

(In reply to Boris Zbarsky (:bz) from comment #7)
> Comment on attachment 725104 [details] [diff] [review]
> tests
> 
> r=me assuming these fail without the patch.

Just verified to make sure they do fail (for me locally at least) without the patch. Thanks for the quick reviews, Boris ! :D

Comment 9

[Ian Melven :imelven]

https://hg.mozilla.org/integration/mozilla-inbound/rev/629a55eb6ff8
https://hg.mozilla.org/integration/mozilla-inbound/rev/512c93bfdb66

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/629a55eb6ff8
https://hg.mozilla.org/mozilla-central/rev/512c93bfdb66