Open Bug 1522702 Opened 6 years ago Updated 4 months ago

"allow-scripts" iframe sandbox attribute not well taken into account if set after the iframe is appended to the DOM and after the src is set.

Categories

(Core :: DOM: Security, defect, P3)

defect

Tracking

()

People

(Reporter: ylesaout, Unassigned)

Details

(Keywords: reporter-external, Whiteboard: [reporter-external] [client-bounty-form] [verif?][domsecurity-backlog1])

Attachments

(1 file)

1.19 KB, application/zip
Details
Attached file Archive.zip

I discovered this issue when working on the following PR: https://github.com/Aaronius/penpal/pull/25

I have been able to reproduce it with Firefox 64 on the latest Ubuntu and OSX, and with Firefox 63 on Windows 10.

The issue is that you can get full access to the parent from the iframe if :

  • iframe and parent have the same origin and
  • the sandbox attribute is set with "allow-scripts" after the iframe has been appended to the DOM and the src attribute has been set
    This occurs even if the sandbox attribute is set imediately after.
    Note that, if you set the sandbox attribute to "", JS is not even executed in the iframe, thus indicating that the sandbox attribute is taken into account.

To reproduce the issue, you need to :

  • fisrt, create an iframe element
  • second set the src attribute and append the iframe to the DOM (no matter in which order)
  • finally set the sandbox attribute with "allow-scripts"

You will also find attached a POC with parent.html and child.html files. Just serve them from any webserwer (or even localy in file: protocol) to see that JS is not only executed but that the iframe has access to the parent. In this POC I insert element in the parent document, replace the XMLHttpRequest constructor (and thus can control any request made by the parent) or get access to the parent.document.cookie (read/write).

Finally, please note that this issue doesn't occur on latest Chrome or Safari.

Please let me know if you need more information.

Regards,
Yannick

Flags: sec-bounty?

Baku, in Christoph's absence, do you want to triage this? I haven't really taken a closer look, mostly moving this out of the Firefox component. Let me know if I can help.

Group: firefox-core-security → core-security
Component: Security → DOM: Security
Flags: needinfo?(amarchesini)
Product: Firefox → Core

This doesn't seem a sec-high/sec-critical or anything like that due to it requiring a few harder to replicate attack requirements.

I think we can wait for Christophs return to check this, however Dan could you confirm my assessment? I'll not add a whiteboard tag so we catch it in the next review also.

Flags: needinfo?(dveditz)
Keywords: sec-moderate

Hi,

Please note that this issue leads to the same security risks (e.g. remove the sandbox attribute) that using both allow-scripts and allow-same-origin when the embedded document has the same origin as the embedding page.
Using both these values, even if allowed by the specification, seems to imply an enough high security risk for Mozilla to:

However, here we are using only the allow-scripts value. The developer, who has read the doc, will think that the security is OK as he doesn't use "allow-same-origin". But if he set the sandbox attribute after setting the src one and appending the iframe element to the DOM, the parent page will be subject to the same security issues as if he had used allow-scripts allow-same-origin, but without any warning.

Below, you will find a POC which removes the sandbox attribute from the iframe. Just serve the content of parent.html and child.html from any webserver (in the same directory) and open parent.html in firefox.

parent.html

<!DOCTYPE html>
<html>
<head>
    <script type="text/javascript">
        window.addEventListener("load", function() {
            var iframe = document.createElement('iframe');
            iframe.src = "./child.html";
            document.body.appendChild(iframe);
	        iframe.sandbox = "allow-scripts";
        });
    </script>
</head>
<body></body>
</html>

child.html

<!DOCTYPE html>
<html>
<head>
    <script type="text/javascript">
        Array.prototype.forEach.call(
            parent.document.getElementsByTagName('iframe'),
            function (iframe){
            if (iframe.contentWindow == window) {
                iframe.removeAttribute('sandbox');
            }
        });
    </script>
</head>
<body></body>
</html>

By spec, "these flags only take effect when the nested browsing context of the iframe is navigated. Removing them, or removing the entire sandbox attribute, has no effect on an already-loaded page." https://html.spec.whatwg.org/multipage/iframe-embed-object.html#attr-iframe-sandbox

https://html.spec.whatwg.org/multipage/iframe-embed-object.html#attr-iframe-src also says: "When an iframe element element is inserted into a document that has a browsing context, the user agent must run these steps: [...] Process the iframe attributes for the first time". This triggers: "Navigate the element's nested browsing context to resource.".

So, setting the "sandbox" attribute after should not make the iframe sandboxed, if already navigated.

Of course, would be nice to add a warning message to inform the user that the sandbox attribute will be used only during the following navigation.

This is not a sec-bug. Dan, do you agree?

Flags: needinfo?(amarchesini)
Priority: -- → P3

I just mid-aired with you making a comment linking to that exact part of the spec :-)

I agree this is not a security bug, and by unhiding the bug we're publicizing that authors should add attributes in a safe order. However it is curious that blink/webkit don't behave the same. Is the spec ambiguous in some way you and I aren't seeing, or is it just a bug on their part?

Group: core-security
Flags: needinfo?(dveditz)
Keywords: sec-moderate

IMO the spec is not that clear.
Indeed, the 2nd sentence of the warning pointed by Andrea Marchesini only speaks about removing flags or the attribute itself. Why this sentence does not begins with "Setting them, removing them or ..." ?
The second part specifies that it has no effect on an already-loaded page. But when I set the sandbox attribute just after I appended the iframe into the DOM, should the iframe be considered as already loaded ?

In fact, it looks like that adding the sandbox attribute after setting the src and appending the iframe into the DOM is well taken into account by firefox. If you just set the attribute without the "allow-scripts" flag, then the iframe scripts is not executed. Thus the sandbox attribute has been taken into account. This is the reason I was thinking it may be a security issue.

Looking more closely, the issue may not be due to the sandbox attribute itself but with the origin of the iframe document. In a sandboxed iframe the origin must be an opaque origin and window.origin equal to the string "null". However, by setting the sandbox attribute after setting the src and appending the iframe into the DOM, the origin of the iframe is not updated to an opaque ones. Thus, by just allowing the script to be executed they can access the parent as they have the same origin.
Below are two examples. One (parent1.html) with the sandox attribute set before the iframe is appended and another (parent2.html) with the sandox attribute set after the iframe is appended. The first one triggers an error when trying to access the contentWindow.origin property of the iframe, due to cross-origin violation. The second one can access this property. Note that the script in the child is never executed.

parent1.html

<!DOCTYPE html>
<html>
<head>
    <script type="text/javascript">
    var iframe;
        function createChild() {
             iframe = document.createElement('iframe');
            iframe.src = "./child.html";
            iframe.sandbox = "";
            document.body.appendChild(iframe);
        }
        window.addEventListener("load", createChild);
        setTimeout(function(){
            console.log("Trying to access iframe.contentWindow.origin should have raised an error, but if not, the iframe origin is", iframe.contentWindow.origin);
            }, 2000);
    </script>
</head>
<body></body>
</html>

parent2.html

<!DOCTYPE html>
<html>
<head>
    <script type="text/javascript">
    var iframe;
        function createChild() {
             iframe = document.createElement('iframe');
            iframe.src = "./child.html";
            iframe.sandbox = "";
            document.body.appendChild(iframe);
        }
        window.addEventListener("load", createChild);
        setTimeout(function(){
            console.log("Trying to access iframe.contentWindow.origin should have raised an error, but if not, the iframe origin is", iframe.contentWindow.origin);
            }, 2000);
    </script>
</head>
<body></body>
</html>

child.html

<!DOCTYPE html>
<html>
<head>
  <script type="text/javascript">
  console.log("I should not have been executed");
  </script>
</head>
<body></body>
</html>

Anne: please weigh in on an interpretation of the spec language around sandboxing here.

Flags: needinfo?(annevk)

(You might want to fix the examples as they appear identical to me.)

A particularly relevant requirement here is:

When an iframe element with a sandbox attribute has its nested browsing context created (before the initial about:blank Document is created), and when an iframe element's sandbox attribute is set or changed while it has a nested browsing context, the user agent must parse the sandboxing directive using the attribute's value as the input and the iframe element's nested browsing context's iframe sandboxing flag set as the output.

If you couple that with navigation not being able to load and create the target document (child.html) before the attribute is set, I think we have a bug here. In particular, I would expect the implementation to consult "sandboxed origin browsing context flag" when the new document is being created, which has to be at after the current task. (This last bit isn't specified as clearly as it should be, in part because document creation is a bit of a mess in the HTML Standard.)

Flags: needinfo?(annevk)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [reporter-external] [client-bounty-form] [verif?] → [reporter-external] [client-bounty-form] [verif?][domsecurity-backlog1]
Flags: sec-bounty? → sec-bounty-
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: