Bug 849791 (CVE-2013-1695)

Frame DocShells do not inherit sandbox flags from their parents.

RESOLVED FIXED in Firefox 22

Status

()

Core
Document Navigation
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: bobowen, Assigned: imelven)

Tracking

({sec-low})

Trunk
mozilla22
sec-low
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox21 wontfix, firefox22 fixed, firefox23 fixed, firefox-esr17 wontfix, b2g18 wontfix)

Details

(Whiteboard: [adv-main22+])

Attachments

(2 attachments)

(Reporter)

Description

4 years ago
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.
(Reporter)

Updated

4 years ago
Blocks: 341604
Component: Security → Document Navigation
(Assignee)

Comment 1

4 years ago
Marking as sec-low based on other iframe sandbox bugs. Open to discussion, as always.
Keywords: sec-low
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>
(Assignee)

Updated

4 years ago
Assignee: nobody → imelven
(Assignee)

Updated

4 years ago
Version: unspecified → Trunk
(Assignee)

Comment 3

4 years ago
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.
Attachment #725030 - Flags: review?(bzbarsky)
Comment on attachment 725030 [details] [diff] [review]
patch

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

Comment 5

4 years ago
Created attachment 725104 [details] [diff] [review]
tests

Tests.
Attachment #725104 - Flags: review?(bzbarsky)
(Assignee)

Comment 6

4 years ago
https://tbpl.mozilla.org/?tree=Try&rev=752dc609077b
Comment on attachment 725104 [details] [diff] [review]
tests

r=me assuming these fail without the patch.
Attachment #725104 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 8

4 years ago
(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
(Assignee)

Comment 9

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/629a55eb6ff8
https://hg.mozilla.org/integration/mozilla-inbound/rev/512c93bfdb66
https://hg.mozilla.org/mozilla-central/rev/629a55eb6ff8
https://hg.mozilla.org/mozilla-central/rev/512c93bfdb66
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
status-b2g18: --- → wontfix
status-firefox21: --- → wontfix
status-firefox22: --- → fixed
status-firefox23: --- → fixed
status-firefox-esr17: --- → wontfix
Whiteboard: [adv-main22+]
Alias: CVE-2013-1695
Blocks: 886262
Group: core-security
You need to log in before you can comment on or make changes to this bug.