Last Comment Bug 849791 - (CVE-2013-1695) Frame DocShells do not inherit sandbox flags from their parents.
(CVE-2013-1695)
: Frame DocShells do not inherit sandbox flags from their parents.
Status: RESOLVED FIXED
[adv-main22+]
: sec-low
Product: Core
Classification: Components
Component: Document Navigation (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla22
Assigned To: Ian Melven :imelven
:
:
Mentors:
Depends on:
Blocks: framesandbox CVE-2013-5614
  Show dependency treegraph
 
Reported: 2013-03-11 06:07 PDT by Bob Owen (:bobowen)
Modified: 2014-11-19 19:48 PST (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
fixed
fixed
wontfix
wontfix


Attachments
patch (2.52 KB, patch)
2013-03-14 11:27 PDT, Ian Melven :imelven
bzbarsky: review+
Details | Diff | Splinter Review
tests (6.31 KB, patch)
2013-03-14 14:20 PDT, Ian Melven :imelven
bzbarsky: review+
Details | Diff | Splinter Review

Description Bob Owen (:bobowen) 2013-03-11 06:07:04 PDT
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 2013-03-11 11:17:41 PDT
Marking as sec-low based on other iframe sandbox bugs. Open to discussion, as always.
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2013-03-11 20:46:50 PDT
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 2013-03-14 11:27:23 PDT
Created attachment 725030 [details] [diff] [review]
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 [:bz] (still a bit busy) 2013-03-14 11:32:05 PDT
Comment on attachment 725030 [details] [diff] [review]
patch

r=me
Comment 5 Ian Melven :imelven 2013-03-14 14:20:54 PDT
Created attachment 725104 [details] [diff] [review]
tests

Tests.
Comment 6 Ian Melven :imelven 2013-03-14 14:21:20 PDT
https://tbpl.mozilla.org/?tree=Try&rev=752dc609077b
Comment 7 Boris Zbarsky [:bz] (still a bit busy) 2013-03-14 18:29:45 PDT
Comment on attachment 725104 [details] [diff] [review]
tests

r=me assuming these fail without the patch.
Comment 8 Ian Melven :imelven 2013-03-14 22:35:55 PDT
(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

Note You need to log in before you can comment on or make changes to this bug.