in bug 341604 comment 25 khuey suggested "We should detect the situation called out in the first warning (and maybe the second too?) at http://www.whatwg.org/specs/web-apps/current-work/multipage/the-iframe-element.html#attr-iframe-sandbox and send a warning to the error console." this seems like a potential good first bug for a contributor to me, once bug 341604 lands, setting mentor=me, lang=c++

I would ask for a link to a source file, except that that can't really happen until bug 341604 is finished. Please remember to leave one once that occurs?

(In reply to Josh Matthews [:jdm] (travelling until June 25th) from comment #1) > I would ask for a link to a source file, except that that can't really > happen until bug 341604 is finished. Please remember to leave one once that > occurs? will do - thanks for setting the dependency, i meant to do that and apparently forgot !

Here's a potential spot to add this console warning : nsContentUtils::ParseSandboxAttributeToFlags http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsContentUtils.cpp#935 Another possibility is in nsHTMLIFrameElement::AfterSetAttr after we have parsed the attribute, we could check the new flags http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLIFrameElement.cpp#209 The warning from the spec is "Setting both the allow-scripts and allow-same-origin keywords together when the embedded page has the same origin as the page containing the iframe allows the embedded page to simply remove the sandbox attribute." It seems like we'd check to see if these flags were both unset and then would have to check the frame's principal matches that of its parent.

I am interested in tackling this issue. I have managed to generate a warning when there is a sandbox attribute in an iframe element in the error console. At the moment I am creating the warning like this: nsCOMPtr<nsIScriptError> error(do_CreateInstance(NS_SCRIPTERROR_CONTRACTID)); error->Init(NS_LITERAL_STRING("a logging message"), NS_LITERAL_STRING(""),NS_LITERAL_STRING(""),0, 0, 1,"HTML"); and then sending it to the console service. Should the warning contain the line number of the iframe tag? I think that containing it would be appropriate. The link I presume should be the name of the parent. I am not certain how I can get the name of the parent. I also not certain how to get the line number of the tag. Another issue I am having is checking for the same origin. What would be the best way to approach this, and is that possible in http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLIFrameElement.cpp#209 Thank you for your time to help mentor me through this bug.

With regards to checking the origin, you can try using nsContentUtils::CheckSameOrigin, or maybe looking at hg.mozilla.org/mozilla-central/annotate/f2a500997116/docshell/base/nsDocShell.cpp#l1686 for inspiration. It's good that you've managed to get warnings showing up in the error console. I recommend not focusing on the details like the line number to display right now; I don't think the line number of the iframe is the best data to show, and something like the JS line that triggered this check would make more sense but is probably not straightforward. You'll definitely need to obtain the correct window ID to initialize the script error correctly, though. Look for other examples of creating script errors for the right way to get that.

Vincentiu, thanks for working on this ! If you have a work-in-progress patch, please feel free to attach it to the bug and feedback? it to me if you would like me to take a look. I agree with what Josh said in comment 5, don't worry about reporting the line number for now. Also as he says, if we want this warning to show in the web console and not just the error console, we need an inner window ID. Check out nsConsoleUtils::ReportToConsole which you may want to use, it handles some of this as well as localization for you.

I have been working on this for a while now. I am unfortunately stuck in two points. I approached the problem in the following way: - try to keep the existing code structure as is and only add a check to: nsHTMLIFrameElement::AfterSetAttr right before the call on line 230 http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLIFrameElement.cpp#230 This makes sense to me in the following way: I have the element and the sandbox flags. I can check if the two flags: allow-same-origin and allow-scripts are set via bitwise operations. I should be able to check the origin of the parent and the iframe via (CheckSameOrigin): http://mxr.mozilla.org/mozilla-central/source/content/base/public/nsContentUtils.h#357 If both these checks are true, then display the error in the error console. The issues that I am having with this approach are the following: --------------- The first one is that the code to set the flags in nsHTMLIFrameElement::AfterSetAttr is never reached, because the test on line 217 concludes that mFrameLoader is NULL. I ran a gdb trace to see where ParseSandboxAttributeToFlags is called and I've concluded that it gets called once in: nsFrameLoader::ReallyStartLoadingInternal at line 457 iframe->GetSandBoxFlags(), then the flags are set. --------------- The second issue is with actually calling the function CheckSameOrigin. From what I've managed to pick up from various sources (code and documentation) I need to get two nodes, the parent and the iframe node. I've tried the following approach: http://pastebin.mozilla.org/2029329 Coding style aside, the code ends in a Segmentation Fault because: this->GetContentDocument(&doc); gives a NULL doc. --------------- The page that I am testing my code on is the following: http://pastebin.mozilla.org/2029337 My approach is probably not the way to go about solving this. This is mostly due to the fact that I don't understand how the docshell and node structure works. I've read a general presentation on http://www-archive.mozilla.org/projects/embedding/docshell.html but it is slightly to general for what I feel is necessary here. Any help is appreciated. Thank you again and have a Happy New Year! :)

(In reply to Vicențiu Ciorbaru from comment #7) Vincentiu, I've been thinking more about the right approach here based on what you've found so far. Maybe it would be better to do the check for the sandbox flags when they are set during document load in nsDocument::StartDocumentLoad The flags are stored in two places, on the docshell and the document itself. The docshell stores what's essentially the current value of the iframe's sandbox attribute. Per spec, when you change the value of .sandbox, it doesn't take affect until a navigation occurs - when this happens, we see if the docshell (and also the parent document) has sandbox flags set, and if so, copy the appropriate flags to the document as it is loaded. When an iframe's .sandbox is called, nsHTMLIFrameElement::AfterSetAttr is called (and mFrameLoader will be non-NULL), and the new flags are set on the docshell. For purposes of the warning, I think it's better to do the warning when the document is loaded, rather than every time the value of .sandbox is changed. Checking same origin should be more straightforward as well.. One note on your test case, there's a typo in allow-scripts : "<iframe sandbox="allow-same-origin allow-scrips" src="test.html">" Happy New Year to you as well ! :D

(In reply to Ian Melven :imelven from comment #8) > (In reply to Vicențiu Ciorbaru from comment #7) > > Vincentiu, > > I've been thinking more about the right approach here based on what you've > found so far. > > Maybe it would be better to do the check for the sandbox flags when they are > set during document load in nsDocument::StartDocumentLoad > > The flags are stored in two places, on the docshell and the document itself. > The docshell stores what's essentially the current value of the iframe's > sandbox attribute. Per spec, when you change the value of .sandbox, it > doesn't take affect until a navigation occurs - when this happens, we see if > the docshell (and also the parent document) has sandbox flags set, and if > so, copy the appropriate flags to the document as it is loaded. > > When an iframe's .sandbox is called, nsHTMLIFrameElement::AfterSetAttr is > called sigh, I mean, when an iframe's .sandbox attribute is set...

Hello there, My friend and I are interested in helping out. Is this bug still active? If so, where/how do we begin? Thanks.

I am quite busy at the moment and will probably be unable to work on this until the 10th of February (exams period). I think the bug is still active. You could use my findings from earlier I guess. Vicentiu

Vicentiu, thanks very much for your work on this ! Josh, this bug is still active. I think comment 8 is a good place to start - the previous comments, especially comment 5 and comment 6 also talk about some of the details involved in fixing this. I think after what Vincentiu found, we should try to do the logging when the sandboxing flags are copied from the docshell to the document during nsDocument::StartDocumentLoad : http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#2202 You can also find me on irc.mozilla.org as imelven if you want to chat about working on this :)

After a long while, this is my first work in progress patch. I doubt that checking the origin is done correctly, however I do not know how to get the parent document. I need to get the document that was loaded before the iframe document. At the moment, mParentDocument that is used in the call is a nullptr in my test case. The part that works is the warning in the error console, as well as the localization string that I have added.

Comment on attachment 716141 [details] [diff] [review] Initial patch to introduce the console warning Review of attachment 716141 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, I'll try to take a look at nsDocument some more to see what we can do about getting the parent document. ::: content/base/src/nsDocument.cpp @@ +2289,5 @@ > NS_ENSURE_SUCCESS(rv, rv); > + if (mSandboxFlags & SANDBOXED_NAVIGATION && > + !(mSandboxFlags & SANDBOXED_SCRIPTS) && > + !(mSandboxFlags & SANDBOXED_ORIGIN)) { > + // If the document is part of a sandbox, and the allow-scripts and nit: trailing whitespace What about: // If the document is sandboxed (via the HTML5 iframe sandbox attribute or // CSP sandbox) and the allow-scripts and allow-same-origin flags are set, // the sandboxed document can call into its parent document and remove its // sandboxing entirely - we print a warning to the web console in this case. (again, the reviewer may also have input here :) ) @@ +2292,5 @@ > + !(mSandboxFlags & SANDBOXED_ORIGIN)) { > + // If the document is part of a sandbox, and the allow-scripts and > + // allow-same-origin flags are set, the origin must be checked, and > + // if it is the same, a warning is printed to the console. > + nsresult rv = nsContentUtils::CheckSameOrigin(this, mParentDocument); so we need to look at where mParentDocument is set and see if we can do that earlier. If we can't do that, we could try and get the parent document explicitly here when we need it (ie inside the if clause that looks for the risky sandbox flags). ::: dom/locales/en-US/chrome/dom/dom.properties @@ +132,5 @@ > OldCSPHeaderDeprecated=The X-Content-Security-Policy and X-Content-Security-Report-Only headers will be deprecated in the future. Please use the Content-Security-Policy and Content-Security-Report-Only headers with CSP spec compliant syntax instead. > # LOCALIZATION NOTE: Do not translate "X-Content-Security-Policy/Report-Only" or "Content-Security-Policy/Report-Only" > +BothCSPHeadersPresent=This site specified both an X-Content-Security-Policy/Report-Only header and a Content-Security-Policy/Report-Only header. The X-Content-Security-Policy/Report-Only header(s) will be ignored. > +# LOCALIZATION NOTE: Do not translate "allow-scripts" or "allow-same-origin" or "sandbox" or "iframe" > +BothAllowScriptsAndOriginPresent=Warning: Using both allow-scripts and allow-same-origin invalidates the iframe sandbox attribute. How about "An iframe which has both allow-scripts and allow-same-origin for its sandbox attribute can remove its sandboxing" ? I suspect others including the reviewer will also have opinions here :)

I have removed the trailing white space as well as changed the comments / locale warning string as you suggested. Your suggestion makes things much clearer. The problem that remains now is how to get the parent document. I have tried searching around but I could not find any solution. I will keep trying though, the code is starting to make a bit more sense overall now. Also, let me know if the way I am proposing patches to bugzilla is ok. Vicențiu

(In reply to Vicențiu Ciorbaru from comment #15) > Created attachment 717622 [details] [diff] [review] > Second Patch > > I have removed the trailing white space as well as changed the comments / > locale warning string as you suggested. Your suggestion makes things much > clearer. > > The problem that remains now is how to get the parent document. I have tried > searching around but I could not find any solution. I will keep trying > though, the code is starting to make a bit more sense overall now. > > Also, let me know if the way I am proposing patches to bugzilla is ok. > > Vicențiu hi Vicentiu, please see https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F and/or http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed which have info on the right settings for patch diffs and other useful info. I'm glad the suggestions were helpful :) I'll take a look to see if I can help you get the parent document - i'm thinking maybe you could get the document's docshell, get that as a nsIDocShellTreeItem and go up the docshell tree to get the parent. I'll also look at where mParentDocument gets set and where it comes from. Thanks !

Comment on attachment 717622 [details] [diff] [review] Second Patch Patch looks good !

Comment on attachment 717622 [details] [diff] [review] Second Patch Wrong flag, I'm not a module peer :)

I looked at this for a bit and found SetParentDocument at http://mxr.mozilla.org/mozilla-central/source/content/base/public/nsIDocument.h#588 which looks like it sets mParentDocument SetParentDocument is called by nsDocument::SetSubDocumentFor which is called by nsDocumentViewer::SyncParentSubDocMap This uses nsIDocShellTreeItem to find the parent document as I mentioned in comment 16.

I'm planning to work on this bug. So where could I have this assigned to me?

(In reply to Sean Chen from comment #20) > I'm planning to work on this bug. So where could I have this assigned to me? Saying you're planning on working on it is a good start :) I'd start with Vicentiu's patch in comment 15 (assuming he doesn't mind) and use the information in comment 19 to address the remaining piece of work he also mentions in comment 15.

Thank you Ian, this is for a Software Engineering class. My partner is also interested in this as well so we would definitely like to take on the remaining tasks. Please feel free to comment if you have any questions.

Hi, I would be interested in fixing this bug, as I'm very new

Hi Stephen, I suggest checking out the attached patch which does some significant amount of the work for this bug. Comment 15 from Vicentiu mentions one of the remaining things to fix. Comment 19 has some pointers to code that might help with this. I should be back on IRC within the next couple of weeks, but feel free to email me (as you have already) with questions etc. or just ask right here in the bug :)

Ian : could you please repost that test case? Thanks

And also, in comment 15, Vincentiu says that the remaining thing to fix is getting the parent document. Why is this code not working? nsresult rv = nsContentUtils::CheckSameOrigin(this, mParentDocument);

(In reply to Stephen from comment #25) > Ian : could you please repost that test case? > Thanks The test case looks something like : a.html : <html> <body> <iframe src='b.html' sandbox='allow-scripts allow-same-origin></iframe> </body> </html> b.html : <html> <body> i am sandboxed with allow-scripts and allow-same-origin </body> </html>

(In reply to Stephen from comment #26) > And also, in comment 15, Vincentiu says that the remaining thing to fix is > getting the parent document. > Why is this code not working? > nsresult rv = nsContentUtils::CheckSameOrigin(this, mParentDocument); In comment 13 he says that mParentDocument was null in his test case. In comment 16 and comment 19 I suggest using nsIDocShellTreeItem to find the parent.

I get this error when building with Vincentiu's patch 1:17.10 In the directory /home/zsteve/Code/mozilla-source/obj-i686-pc-linux-gnu/content/base/src 1:17.10 The following command failed to execute properly: 1:17.11 c++ -o nsDocument.o -c -I../../../dist/stl_wrappers -I../../../dist/system_wrappers -include /home/zsteve/Code/mozilla-source/config/gcc_hidden.h -DMOZ_GLUE_IN_PROGRAM -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -DSTATIC_EXPORTABLE_JS_API -DNO_NSPR_10_SUPPORT -DOS_POSIX=1 -DOS_LINUX=1 -I/home/zsteve/Code/mozilla-source/netwerk/sctp/datachannel -I/home/zsteve/Code/mozilla-source/ipc/chromium/src -I/home/zsteve/Code/mozilla-source/ipc/glue -I../../../ipc/ipdl/_ipdlheaders -I/home/zsteve/Code/mozilla-source/caps/include -I/home/zsteve/Code/mozilla-source/content/events/src -I/home/zsteve/Code/mozilla-source/content/html/content/src -I/home/zsteve/Code/mozilla-source/content/html/document/src -I/home/zsteve/Code/mozilla-source/content/xbl/src -I/home/zsteve/Code/mozilla-source/content/xml/content/src -I/home/zsteve/Code/mozilla-source/content/xml/document/src -I/home/zsteve/Code/mozilla-source/content/xslt/src/xpath -I/home/zsteve/Code/mozilla-source/content/xul/content/src -I/home/zsteve/Code/mozilla-source/content/xul/document/src -I/home/zsteve/Code/mozilla-source/dom/base -I/home/zsteve/Code/mozilla-source/dom/ipc -I/home/zsteve/Code/mozilla-source/dom/workers -I/home/zsteve/Code/mozilla-source/js/ipc -I/home/zsteve/Code/mozilla-source/image/src -I/home/zsteve/Code/mozilla-source/js/xpconnect/src -I/home/zsteve/Code/mozilla-source/layout/base -I/home/zsteve/Code/mozilla-source/layout/generic -I/home/zsteve/Code/mozilla-source/layout/style -I/home/zsteve/Code/mozilla-source/layout/svg -I/home/zsteve/Code/mozilla-source/layout/xul/base/src -I/home/zsteve/Code/mozilla-source/netwerk/base/src -I/home/zsteve/Code/mozilla-source/js/xpconnect/wrappers -I/home/zsteve/Code/mozilla-source/content/base/src -I. -I../../../dist/include -I/home/zsteve/Code/mozilla-source/obj-i686-pc-linux-gnu/dist/include/nspr -I/home/zsteve/Code/mozilla-source/obj-i686-pc-linux-gnu/dist/include/nss -fPIC -DMOZILLA_CLIENT -include ../../../mozilla-config.h -MD -MP -MF .deps/nsDocument.o.pp -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Wtype-limits -Wempty-body -Wsign-compare -Wno-invalid-offsetof -Wcast-align -fno-exceptions -fno-strict-aliasing -fno-rtti -fno-exceptions -fno-math-errno -std=gnu++0x -pthread -pipe -DNDEBUG -DTRIMMED -g -Os -freorder-blocks -fomit-frame-pointer /home/zsteve/Code/mozilla-source/content/base/src/nsDocument.cpp 1:17.11 make[5]: *** [nsDocument.o] Error 1 1:17.11 make[4]: *** [content/base/src/compile] Error 2 1:17.11 make[4]: *** Waiting for unfinished jobs.... 1:17.27 make[3]: *** [compile] Error 2 1:17.27 make[2]: *** [default] Error 2 1:17.27 make[1]: *** [realbuild] Error 2 1:17.27 make: *** [build] Error 2 1:17.31 674 compiler warnings present. Help!

I think the actual error message comes before the bit of the log that you pasted.

1:25.29 /home/zsteve/Code/mozilla-source/content/base/src/nsDocument.cpp: In member function ‘virtual nsresult nsDocument::StartDocumentLoad(const char*, nsIChannel*, nsILoadGroup*, nsISupports*, nsIStreamListener**, bool, nsIContentSink*)’: 1:25.29 /home/zsteve/Code/mozilla-source/content/base/src/nsDocument.cpp:2476:75: error: no matching function for call to ‘nsContentUtils::ReportToConsole(nsIScriptError::<anonymous enum>, const char [11], nsDocument* const, nsContentUtils::PropertiesFile, const char [33])’ 1:25.29 /home/zsteve/Code/mozilla-source/content/base/src/nsDocument.cpp:2476:75: note: candidate is: 1:25.29 ../../../dist/include/nsContentUtils.h:809:19: note: static nsresult nsContentUtils::ReportToConsole(uint32_t, const nsACString_internal&, nsIDocument*, nsContentUtils::PropertiesFile, const char*, const PRUnichar**, uint32_t, nsIURI*, const nsAFlatString&, uint32_t, uint32_t) 1:25.29 ../../../dist/include/nsContentUtils.h:809:19: note: no known conversion for argument 2 from ‘const char [11]’ to ‘const nsACString_internal&’ looks like the code has changed since the patch was made...

scratch that. fixed up

Ok. So currently my code looks like : if (docShell) { nsresult rv = docShell->GetSandboxFlags(&mSandboxFlags); NS_ENSURE_SUCCESS(rv, rv); // If the document is sandboxed (via the HTML5 iframe sandbox attribute // or CSP sandbox) and the allow-scripts and allow-same-origin flags are // set, the sandboxed document can call into its parent document and // remove its sandboxing entirely - we print a warning to the web console // in this case. if (mSandboxFlags & SANDBOXED_NAVIGATION && !(mSandboxFlags & SANDBOXED_SCRIPTS) && !(mSandboxFlags & SANDBOXED_ORIGIN)) { nsresult rv = nsContentUtils::CheckSameOrigin(this, mParentDocument); if (/*NS_SUCCEEDED(rv)*/true) { nsCString str; str.AssignASCII("DOM Events"); nsContentUtils::ReportToConsole((int)nsIScriptError::warningFlag, str, this, nsContentUtils::eDOM_PROPERTIES, "BothAllowScriptsAndOriginPresent"); } } } but I'm not recieving any error message for this when loading a.html

so could someone help me with this? :)

(In reply to Stephen from comment #34) > so could someone help me with this? :) is ReportToConsole being called ?

Ok Well the console problem is now gone, because I have made an embarassing mistake :P With both if conditions commented out and replaced with true, the error fires for every page loaded, which means that, at least, is working, like if(/* old condition*/ true) ... However, if I uncomment the condition for the first if block if (mSandboxFlags & SANDBOXED_NAVIGATION && !(mSandboxFlags & SANDBOXED_SCRIPTS) && !(mSandboxFlags & SANDBOXED_ORIGIN)) { No error message loads for Ian's test case, and according to gdb, the condition is failing. Sorry for all this trouble. Maybe it's going to be another dumb mistake... but oh well

(In reply to Stephen from comment #36) You could debug in nsHTMLIFrameElement::AfterSetAttr or nsContentUtils::ParseSandboxAttributeToFlags and make sure the sandbox flags are being set on the iframe as you expect. Actually now I notice I missed a ' in the test case : <iframe src='b.html' sandbox='allow-scripts allow-same-origin></iframe> the sandbox attribute needs a closing ' after allow-same-origin

Hi, Sorry, but I won't be able to contribute to Mozilla for the time being, due to too many commitments! :P Could I please be unassigned from this bug. Thanks

Here's a updated version of Vicențiu's patch which can be applied to tip and compiled. I've got two questions: 1. Is nsIScriptError the most appropriate kind of error? I was thinking that it would be better if that warning showed up in the Security section rather than Script. 2. What category should I use for that warning? I couldn't find where the console message categories are defined.

Francois - I've cc'd Garrett and Ivan. Ivan recently worked on some security error reporting and might know offhand the answer to your questions. I'll dig in when I have a little more time soon if they don't get back to you.

Here's a patch which addresses the "window ID" problem highlighted by Josh in comment 5, using the approach suggested by Ian in comment 16. The warning is now correctly reported in both the Browser Console and the Web Console. However, I'm not sure I'm going from an nsIDocShellTreeItem to an nsIChannel and nsIDocument in the best way because that code looks pretty convoluted to my untrained eyes. Also, I still have to move from a Script error to a Security error as mentioned in comment 39. So not done yet, but making progress :)

This patch changes the warning category to be "security" instead of "script", based on what Ivan did on bug 846918, in both consoles (browser and devtools). It also includes a test for this new console warning: mach mochitest-browser browser/devtools/webconsole/test/browser_webconsole_bug_752559_ineffective_iframe_sandbox_warning.js There's nothing left on my TODO list, so I would greatly appreciate any feedback on this patch, including nits.

Thanks Francois - especially for digging up bug 846918 which sounds like it's a good model to follow, nice work. I'll get you some feedback sometime this week, sorry that I've been so busy recently.

This patch is the same as the previous one, but I've rebased it on top of the latest trunk and removed the patch fuzz. For some reason, I'm no longer able to run the devtools mochitests (not sure why, I can run "./mach mochitest-browser browser/base/content/test/" just fine). I have however manually tested this new patch and it still works.

The two attached HTML files are what I'm using for testing this manually.

It turns out the devtools tests have moved. Here's the new way of running them: ./mach mochitest-devtools browser/devtools/webconsole/test/browser_webconsole_bug_752559_ineffective_iframe_sandbox_warning.js

Hi Ian, are you still able to provide feedback on this patch or would you prefer if I found another reviewer?

Hey Francois - sorry i've been so negligent. Bob Owen is another good person to ask for feedback (hope that's alright Bob) since he knows the iframe sandbox code very well. I'll try to look at this soon as well, sorry for the delays.

(In reply to Ian Melven :imelven from comment #49) > Hey Francois - sorry i've been so negligent. Bob Owen is another good person > to ask for feedback (hope that's alright Bob) since he knows the iframe > sandbox code very well. I'll try to look at this soon as well, sorry for the > delays. Sure, I'll try and have a look at this tomorrow. It'll be good to get this warning in the console.

Comment on attachment 8407332 [details] [diff] [review] rebased patch Review of attachment 8407332 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for working on this. Took me a while to try out some test cases. A few things to iron out, particularly whether we should do any (or more) same origin checking. I can't comment much on the actually reporting to console - I've not used it yet. I'll look at the tests once these other things are sorted out. ::: content/base/src/nsDocument.cpp @@ +2585,5 @@ > if (docShell) { > nsresult rv = docShell->GetSandboxFlags(&mSandboxFlags); > NS_ENSURE_SUCCESS(rv, rv); > + // If the document is sandboxed (via the HTML5 iframe sandbox attribute > + // or CSP sandbox) and the allow-scripts and allow-same-origin flags are The CSP sandbox would get applied just below here I think. Might need to move these checks. @@ +2594,5 @@ > + !(mSandboxFlags & SANDBOXED_SCRIPTS) && > + !(mSandboxFlags & SANDBOXED_ORIGIN)) { > + nsCOMPtr<nsIDocShellTreeItem> parentAsItem; > + nsresult rv = docShell->GetSameTypeParent(getter_AddRefs(parentAsItem)); > + NS_ENSURE_SUCCESS(rv, rv); I think the preferred style is now: if (NS_WARN_IF(NS_FAILED(rv))) { return rv; } Same again a couple of times below. But I wonder if we'd want to return, given this is just for a warning message - I'm not sure it can ever fail anyway. @@ +2597,5 @@ > + nsresult rv = docShell->GetSameTypeParent(getter_AddRefs(parentAsItem)); > + NS_ENSURE_SUCCESS(rv, rv); > + nsCOMPtr<nsIDocShell> parentDocShell = do_QueryInterface(parentAsItem); > + nsCOMPtr<nsIChannel> parentChannel; > + rv = parentDocShell->GetCurrentDocumentChannel(getter_AddRefs(parentChannel)); Couldn't |parentDocShell| be null? @@ +2599,5 @@ > + nsCOMPtr<nsIDocShell> parentDocShell = do_QueryInterface(parentAsItem); > + nsCOMPtr<nsIChannel> parentChannel; > + rv = parentDocShell->GetCurrentDocumentChannel(getter_AddRefs(parentChannel)); > + NS_ENSURE_SUCCESS(rv, rv); > + rv = nsContentUtils::CheckSameOrigin(this->GetChannel(), parentChannel); I'm not so sure about the same origin checking. What if we had: Origin1 top containing Origin2 iframe (with allow-scripts allow-same-origin) that contained Origin1 iframe. The final iframe would be able to get its parent's iframe element (from top) modify the sandbox and set its src. It's a bit of a contrived scenario, but we wouldn't warn though. We could check all ancestors, but do we really want to do that? If we removed the same origin check, we might need to change the message a bit. Not sure who's the best person to make this sort of decision. Looks like IE warns whether same origin or not. I thought Chrome warned as well, but I couldn't seem to get it to just now. @@ +2606,5 @@ > + nsresult rv = parentDocShell->GetContentViewer(getter_AddRefs(parentContentViewer)); > + NS_ENSURE_SUCCESS(rv, rv); > + nsContentUtils::ReportToConsole(nsIScriptError::warningFlag, > + NS_LITERAL_CSTRING("Iframe Sandbox"), > + parentContentViewer->GetDocument(), GetContentViewer can return a null, (as I discovered recently to my cost). Not sure it would be in this case, but perhaps we should check. ::: dom/locales/en-US/chrome/security/security.properties @@ +20,5 @@ > InsecurePasswordsPresentOnIframe=Password fields present on an insecure (http://) iframe. This is a security risk that allows user login credentials to be stolen. > LoadingMixedActiveContent=Loading mixed (insecure) active content on a secure page "%1$S" > LoadingMixedDisplayContent=Loading mixed (insecure) display content on a secure page "%1$S" > +# LOCALIZATION NOTE: Do not translate "allow-scripts" or "allow-same-origin" or "sandbox" or "iframe" > +BothAllowScriptsAndOriginPresent=An iframe which has both allow-scripts and allow-same-origin for its sandbox attribute can remove its sandboxing. Should this be BothAllowScriptsAndSameOriginPresent?

Thanks for your feedback Bob, much appreciated! I think this revised patch should address most of the issues you raised: 1. moves the checks after InitCSP is run 2. replaces NS_ENSURE_SUCCESS with the new style 3. checks that parentDocShell and parentContentViewer are not null 4. changes the l10n key to BothAllowScriptsAndSameOriginPresent The one thing I chose not to address straight away is the same origin check. From what I understand, right now, we don't always warn, but when we do, it's always relevant. If we took that check out, then we would always warn, but it would not always be relevant. I'm not sure what's best either. Regarding change #1 above, I don't think it can be tested because according to bug 671389, CSP sandboxing has not landed yet. Regarding change #2, I couldn't find examples on mxr of code that checks the negation of the pattern you showed, so I'm not sure I'm doing it correctly. I think however that given that this is just a warning (as you pointed out), we should probably not return when these checks fail.

(In reply to François Marier [:francois] from comment #52) > Created attachment 8420794 [details] [diff] [review] > patch addressing comment 51 > > Thanks for your feedback Bob, much appreciated! No problem. > 1. moves the checks after InitCSP is run Ah, sorry I thought I'd changed this comment. I meant this to be a question about whether we should just be checking the sandbox attribute or if we should include any CSP sandboxing as well. So it should either move or the comment should change. The warning doesn't seem as relevant to sandbox restrictions enforced through CSP, since I don't think the control anyone might have over CSP sandboxing would be affected by this. Garrett - what's your take on this? > The one thing I chose not to address straight away is the same origin check. > From what I understand, right now, we don't always warn, but when we do, > it's always relevant. If we took that check out, then we would always warn, > but it would not always be relevant. I'm not sure what's best either. Ah, that's a good point, and having re-read the spec at [1] for this, there is a legitimate use of both keywords when the embedded content is not same-origin. I think this check will catch the vast majority of same-origin cases, so trying to check all the ancestors is probably overkill. > Regarding change #1 above, I don't think it can be tested because according > to bug 671389, CSP sandboxing has not landed yet. If we do decide to take account of the CSP sandbox, then we should probably have a new bug raised that blocks this one and is dependent on bug 671389, to add tests for this. > Regarding change #2, I couldn't find examples on mxr of code that checks the > negation of the pattern you showed, so I'm not sure I'm doing it correctly. > I think however that given that this is just a warning (as you pointed out), > we should probably not return when these checks fail. This is getting pretty horribly nested now and is going way over 80 chars. I think a static (file scope) void function like WarnIfSandboxIneffective would be more readable, with the if statements inverted with early returns. I believe that file scope functions are acceptable from reading the style guide and I think you'd only have to pass the flags and docshell. I still need to look at the tests, so I'll leave the feedback flag to remind me. [1] http://www.whatwg.org/specs/web-apps/current-work/multipage/origin-0.html#attr-iframe-sandbox-allow-same-origin

> I meant this to be a question about whether we should just be checking the sandbox attribute or if we should include any CSP sandboxing as well. So it should either move or the comment should change. The warning doesn't seem as relevant to sandbox restrictions enforced through CSP, since I don't think the control anyone might have over CSP sandboxing would be affected by this. I'm not familiar with the type of warnings that are being reported here, but generally I think we should consider CSP sandboxing if possible. The use cases have significant overlap, and there certainly could be cases in which the person who sees the warning also has control over the CSP header being sent (if I'm understanding your "control" comment correctly). Since, as I understand it, CSP sandbox uses the same flags on the docshell as iframe sandbox, it seems like t should be pretty easy to reuse this code for both features (as long as, as you pointed out, the check happens after InitCSP is run).

Comment on attachment 8420794 [details] [diff] [review] patch addressing comment 51 Review of attachment 8420794 [details] [diff] [review]: ----------------------------------------------------------------- Sorry about the delay in continuing with this feedback. I had a brief chat with Garrett on IRC over including CSP, but I'm not sure we came to a complete conclusion, so I'll try and set out my concerns here in more detail. Is having allow-same-origin and allow-scripts with a document that is the same origin as the parent inherently bad? If it is then we should include the CSP sandbox and we should change the message to be more general. In fact maybe we should check them both separately and warn separately (this would need to be a follow-up to bug 671389). Otherwise, a weakness in one sandbox "policy" could be masked by the other, although this would only become a problem if the stronger policy was removed for some reason. If the problem with allow-same-origin, allow-scripts is specifically due to the fact that the sandboxed document could change or remove the sandbox attribute from its iframe (as suggested by the spec and our message), then I don't think it is relevant to the CSP sandbox. It doesn't in itself allow someone to change the CSP sandbox, although they may well have control over it by other means. If we do include it, it could: a) mask a problem with the settings on the iframe sandbox attribute or b) cause a warning when there is no sandbox attribute at all Hopefully that makes sense :) There was some other feedback at the end of comment 53, just in case you missed it with the split feedback. More specific feedback for tests below. ::: browser/devtools/webconsole/test/browser_webconsole_bug_752559_ineffective_iframe_sandbox_warning.js @@ +14,5 @@ > + waitForMessages({ > + webconsole: hud, > + messages: [ > + { > + name: "Ineffective iframe sandboxing warning displayed successfully", I'm not familiar with devtools tests, so I can't give general feedback. However, I think we need some negative testing, i.e. when we do not expect a message ... not sure how easy this is. It would be good if we could at least test the following: * allow-scripts, allow-same-origin, but not same-origin document * allow-scripts, no allow-same-origin, same-origin document * no allow-scripts, allow-same-origin, same-origin document That way we exercise most of the logic in the code. ::: browser/devtools/webconsole/test/test-bug-752559-ineffective-iframe-sandbox-warning-inner.html @@ +3,5 @@ > + <head> > + <meta charset="utf8"> > + <title>Bug 752559 - print warning to error console when iframe sandbox > + is being used ineffectively</title> > + <!-- Any copyright is dedicated to the Public Domain. tiny nit: indentation doesn't quite match, same in other html file.

Sorry to drag you back into this Garrett. Could you take a look at the first part of comment 55, over including CSP sandbox. Hopefully I've explained things more clearly.

This revised patch includes the static function suggested in comment 53 as well as the extra tests mentioned in comment 55.

Comment on attachment 8428613 [details] [diff] [review] patch addressing comments 53 and 55 Review of attachment 8428613 [details] [diff] [review]: ----------------------------------------------------------------- Thanks François, I definitely think this is more readable (hopefully the reviewer will agree :-) ) You could possibly reduce the nesting further and there are still a couple of lines over 80. Might be worth waiting until you've had some feedback from the actual reviewer before making any more changes. I've also just recently discovered (on one of my patches) that there is still a debate going on over the NS_ENSURE_* macros, so apologies if you're asked to change that back again at some point. I think those tests give pretty good coverage now, but as I said before I'm not terribly familiar with these types of tests, so other people may have more comments to make. Possibly over reducing duplication for example. The main thing outstanding is the question from comment 55 over including CSP. Once that's resolved and you've made any necessary changes, I think this is ready for some feedback from a reviewer. ::: browser/devtools/webconsole/test/test-bug-752559-ineffective-iframe-sandbox-warning-inner.html @@ +7,5 @@ > + <!-- Any copyright is dedicated to the Public Domain. > + http://creativecommons.org/publicdomain/zero/1.0/ --> > + </head> > + <body> > + <p>I am sandboxed with allow-scripts and allow-same-origin.</p> nit: this isn't always the case any more.

(In reply to Bob Owen (:bobowen) from comment #55) > Is having allow-same-origin and allow-scripts with a document that is the > same origin as the parent inherently bad? > If it is then we should include the CSP sandbox and we should change the > message to be more general. > In fact maybe we should check them both separately and warn separately (this > would need to be a follow-up to bug 671389). > Otherwise, a weakness in one sandbox "policy" could be masked by the other, > although this would only become a problem if the stronger policy was removed > for some reason. > > If the problem with allow-same-origin, allow-scripts is specifically due to > the fact that the sandboxed document could change or remove the sandbox > attribute from its iframe (as suggested by the spec and our message), then I > don't think it is relevant to the CSP sandbox. > It doesn't in itself allow someone to change the CSP sandbox, although they > may well have control over it by other means. > If we do include it, it could: > a) mask a problem with the settings on the iframe sandbox attribute or > b) cause a warning when there is no sandbox attribute at all > > Hopefully that makes sense :) You laid out the issue nicely. We followed up on IRC today. The conclusion was that we should check them separately. "allow-same-origin allow-scripts" is a bad policy for a same-origin load for both iframe sandbox and CSP sandbox, although or different reasons: * for iframe sandbox, it allows the framed content to modify or remove the sandbox * for CSP sandbox, the framed content cannot modify the sandbox - but this policy is so loose as to almost be useless, it allows full XSS in the parent doc. There are still some protections it provides (e.g. no plugins, popups, pointer lock, etc.) So this bug should focus on iframe sandbox, and, as mentioned previously, we should file a follow-up to consider reporting a similar error for CSP sandbox. Finally, dveditz pointed out that this warning should include the URL of the iframe in question. Otherwise, it would be difficult to distinguish between nested iframes.

This revised patch removes the CSP-related stuff, fixes the nesting and nit suggested in comment 58 as well as sets the warning URL to the inner iframe, as requested in comment 59.

Comment on attachment 8440568 [details] [diff] [review] patch addressing comments 58 and 59 Review of attachment 8440568 [details] [diff] [review]: ----------------------------------------------------------------- Sorry about the delay, I got snarled up with my own patches. Thanks for your perseverance with this. ::: content/base/src/nsDocument.cpp @@ +2519,5 @@ > + // and remove its sandboxing entirely - we print a warning to the > + // web console in this case. > + if (aSandboxFlags & SANDBOXED_NAVIGATION && > + !(aSandboxFlags & SANDBOXED_SCRIPTS) && > + !(aSandboxFlags & SANDBOXED_ORIGIN)) { nit: this could be inverted as well. @@ +2546,5 @@ > + if (NS_WARN_IF(NS_FAILED(rv)) || !parentContentViewer) { > + return; > + } > + nsCOMPtr<nsIURI> iframeUri; > + aChannel->GetURI(getter_AddRefs(iframeUri)); I suspect this should be the parentChannel, as it is that document that contains the offending sandboxing attribute. In the IRC exchange, dveditz did mention that the line number would be nice as well, but I think that might be quite difficult.

Thanks for your thoughtful feedback Bob. Regarding the check that could be inverted, I did try it, but it reads better to me when the check is not negated. It's easier to verify that it matches the comment and that it does what it's meant to do. As for the line numbers, I did look into adding that, but got stuck and so I'm not sure exactly where I should pull that from. Without the line numbers, it seems like it might be easier to find the offending iframe if we print the URL of the inner iframe as opposed to the outer document. For example, if you have the following: outer-page.html +----> iframe1.html | +----> iframe2.html | +----> iframe3.html and you get "outer-page.html is ineffective" then you don't know which iframe triggered that warning. On the other hand, if you get "iframe2.html is ineffective" then you can open up outer-page.html and search for "iframe2.html" and find it. Was there another use case you had in mind?

This patch is the same as attachment 8440568 [details] [diff] [review] except for the fact that I refactored the devtools tests to remove duplicated code and improve their output.

(In reply to François Marier [:francois] from comment #62) > Thanks for your thoughtful feedback Bob. > > Regarding the check that could be inverted, I did try it, but it reads > better to me when the check is not negated. It's easier to verify that it > matches the comment and that it does what it's meant to do. Fair enough, it's largely a matter of taste when it comes to things like this. :-) Although, it did make me read the comment more closely ... "// If the document is sandboxed (via the HTML5 iframe sandbox // attribute) and both the allow-scripts and allow-same-origin flags // are set, the sandboxed document can call into its parent document // and remove its sandboxing entirely - we print a warning to the // web console in this case." if (aSandboxFlags & SANDBOXED_NAVIGATION && !(aSandboxFlags & SANDBOXED_SCRIPTS) && !(aSandboxFlags & SANDBOXED_ORIGIN)) {" You are talking about flags being set here and then checking that flags are not set. Probably better to refer to the allow-* parts as keywords, which is what the spec calls them. The spec is a little confusing in the way it has been written, where keywords in the attribute cause flags to be turned off. We may not be totally consistent in the way we've use these terms in the code. Sorry, that I didn't notice this before. > As for the line numbers, I did look into adding that, but got stuck and so > I'm not sure exactly where I should pull that from. Yeah, I thought this would be tricky and I did quickly search the code-base before and didn't spot any similar examples. I'll try and have a look today or tomorrow, to see if I can figure anything out. It might be easier if we did the check earlier on, but then it might be more difficult to check the documents are same origin. I only came in fairly late on in this bug, so I've not looked at other potential places we could check this. > Without the line numbers, it seems like it might be easier to find the > offending iframe if we print the URL of the inner iframe as opposed to the > outer document. For example, if you have the following: > > outer-page.html +----> iframe1.html > | > +----> iframe2.html > | > +----> iframe3.html > > and you get "outer-page.html is ineffective" then you don't know which > iframe triggered that warning. On the other hand, if you get "iframe2.html > is ineffective" then you can open up outer-page.html and search for > "iframe2.html" and find it. > > Was there another use case you had in mind? Hmm, without the line number maybe you're right in this situation. The only thing is, when you click on the link, it opens a document source that doesn't have the iframe in question in it, but the developer should know what's going on. If I remember correctly dveditz was thinking of nested iframes when he mentioned this on IRC, e.g. outer-page.html +-(allow-same-origin allow-scripts)-> iframe1.html +-(allow-scripts)-> iframe2.html Here, with this patch, we would link to iframe1.html, which contains an iframe that is OK. I'll pass this one to dveditz ... Dan, if we can't get the line number, do you think it is better to give the URL of the document that contains the iframe with the faulty sandbox attribute or the URL of the document within that iframe? Also François, on a slightly different note, because we're doing the checks in the document after all the flags have been merged, if we had the opposite of this example: outer-page.html +-(allow-scripts)-> iframe1.html +-(allow-same-origin allow-scripts)-> iframe2.html This wouldn't warn because you can't relax the sandboxing from the parent iframe. However, if someone got you to load iframe1.html directly, iframe2.html would be able to escape its sandbox. Anyway, maybe I'm over analysing this again, this patch will catch most cases I think.

(In reply to Bob Owen (:bobowen) from comment #64) > Dan, if we can't get the line number, do you think it is better to give the > URL of the document that contains the iframe with the faulty sandbox > attribute or the URL of the document within that iframe? Not a lot of point giving the outer URL -- presumably the dev looking at the console knows which page they're debugging. The nested case could be confusing, though. If we decide to warn about /potential/ issues (should an attacker load iframe1.html directly) then we could add the containing URL to the message. But only in the nested case, and I'm not sure we need to. Why warn only if the outer and inner frames are same-origin? That combination of flags is just plain dangerous. What if the inner content is cross-domain, but there was a way to get a user to navigate the frame back to same origin? Then you'd warn, but the developer isn't looking during an attack. So, now I'm leaning back toward giving the outer URL and warning about the bad flag combination regardless. Then the nested case is simple because you'd give the middle URL in that case and the developer can go investigate that page. The line number would be nice, but the developer should be able to inspect the code of the page and see which one has the problem.

Here's a patch that changes the comment as pointed out in comment 64 and makes the patch apply cleanly on mozilla-central.

(In reply to Daniel Veditz [:dveditz] from comment #65) > Why warn only if the outer and inner frames are same-origin? That > combination of flags is just plain dangerous. What if the inner content is > cross-domain, but there was a way to get a user to navigate the frame back > to same origin? Then you'd warn, but the developer isn't looking during an > attack. > > So, now I'm leaning back toward giving the outer URL and warning about the > bad flag combination regardless. Then the nested case is simple because > you'd give the middle URL in that case and the developer can go investigate > that page. The line number would be nice, but the developer should be able > to inspect the code of the page and see which one has the problem. If we change this, we might need different wording for the warning since the problem is no longer that this keyword combination is ineffective, but rather that it's dangerous. Dan, do you have any suggestions for what that warning should say?

(In reply to Daniel Veditz [:dveditz] from comment #65) > The nested case could be confusing, though. If we decide to warn about > /potential/ issues (should an attacker load iframe1.html directly) then we > could add the containing URL to the message. But only in the nested case, > and I'm not sure we need to. So to be clear, the three examples we have discussed (assuming all same-origin documents): outer-page.html +--(whatever)--> iframe1.html | +--(allow-same-origin allow-scripts)--> iframe2.html With the current patch we would warn and give the URL of iframe2.html. If this changed to outer-page.html then they would have to search for the issue, but if the source was big enough to warrant this, then they have to search for the iframe2 URL anyway. At least it would open the source that has the problem in it. First nested example: outer-page.html +-(allow-same-origin allow-scripts)-> iframe1.html +-(allow-scripts)-> iframe2.html Here we would warn and give the URL for iframe1.html, this definitely seems confusing to me as the sandbox in that document is fine. URL for outer-page.html makes more sense in my opinion. Second nested example: outer-page.html +-(allow-scripts)-> iframe1.html +-(allow-same-origin allow-scripts)-> iframe2.html I could be wrong, but I don't think the current patch would warn at all here. This is because we are checking the flags after all the inheritance has been applied. So because the "allow-same-origin allow-scripts" sandbox is nested within one with only the allow-scripts keyword, the 'sandboxed origin browsing context flag' (SANDBOXED_ORIGIN in our code) will still be set, so we won't warn. If you were to navigate to iframe1.html directly, then we would warn. If I'm right and we thought this was a common enough scenario that we'd like to fix this, we'd need to get hold of the actual sandbox attribute and check that. nsDocument.cpp might not be the best place to do that, but unfortunately, I imagine that in the parts of the code where it is easier to check this, it would be harder to do the same-origin checking. > Why warn only if the outer and inner frames are same-origin? That > combination of flags is just plain dangerous. What if the inner content is > cross-domain, but there was a way to get a user to navigate the frame back > to same origin? Then you'd warn, but the developer isn't looking during an > attack. I thought this at first (back in comment 51), but the spec suggests that there is a legitimate use of "allow-same-origin allow-scripts", when you are framing something that is not same-origin with you. In examples here: http://www.whatwg.org/specs/web-apps/current-work/multipage/the-iframe-element.html#attr-iframe-sandbox and in the note here: http://www.whatwg.org/specs/web-apps/current-work/multipage/origin-0.html#attr-iframe-sandbox-allow-same-origin You could argue that this is a weakness in the spec and for this use case we should have a different keyword. Particularly when you consider that the cross-origin document could just have it's own iframe holding a page from the original origin, which could I believe remove the original sandbox and then reload the cross-origin page. Of course that script would already have to exist on the original origin to begin with. I'm sure I tested this before posting comment 51, but I can't find it at the moment. > So, now I'm leaning back toward giving the outer URL and warning about the > bad flag combination regardless. Then the nested case is simple because > you'd give the middle URL in that case and the developer can go investigate > that page. The line number would be nice, but the developer should be able > to inspect the code of the page and see which one has the problem. I think the URL of the page that contains the iframe with the dodgy sandbox attribute makes most sense. As you say the line number would be nice, but it should be fairly easy to find the problem without it.

Here's a version of the patch that links to the outer page (i.e. parentChannel) and that fixes the wording the comment as pointed out in comment 64. Based on what Bob found in the spec regarding valid use cases for allow-same-origin and allow-scripts, I would be tempted to say that this is ready for a reviewer, no?

Comment on attachment 8451448 [details] [diff] [review] link to the outer page and actually fix the comment Review of attachment 8451448 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to François Marier [:francois] from comment #69) > Based on what Bob found in the spec regarding valid use cases for > allow-same-origin and allow-scripts, I would be tempted to say that this is > ready for a reviewer, no? I think so, thanks. While we still have some potential issues around not warning for nested scenarios, it seems reasonable to think that this will cover most cases.

Comment on attachment 8451448 [details] [diff] [review] link to the outer page and actually fix the comment Olli: it looks like you're regularly reviewing patches to this file, but please feel free to reassign it to someone else if you don't have time or if there's a more appropriate reviewer. This is my first C++ patch on mozilla-central, so I would be more than happy to get any feedback, including nits, you have.

Comment on attachment 8451448 [details] [diff] [review] link to the outer page and actually fix the comment >+WarnIfSandboxIneffective(nsIDocShell* aDocShell, >+ uint32_t aSandboxFlags, >+ nsIChannel* aChannel) >+{ >+ // If the document is sandboxed (via the HTML5 iframe sandbox >+ // attribute) and both the allow-scripts and allow-same-origin >+ // keywords are supplied, the sandboxed document can call into its >+ // parent document and remove its sandboxing entirely - we print a >+ // warning to the web console in this case. >+ if (aSandboxFlags & SANDBOXED_NAVIGATION && >+ !(aSandboxFlags & SANDBOXED_SCRIPTS) && >+ !(aSandboxFlags & SANDBOXED_ORIGIN)) { >+ nsCOMPtr<nsIDocShellTreeItem> parentAsItem; >+ nsresult rv = aDocShell->GetSameTypeParent(getter_AddRefs(parentAsItem)); >+ if (NS_WARN_IF(NS_FAILED(rv))) { >+ return; >+ } I would just drop nsresult rv = and the if+warning >+ nsCOMPtr<nsIDocShell> parentDocShell = do_QueryInterface(parentAsItem); >+ if (!parentDocShell) { >+ return; >+ } ...since do_QueryInterface is null safe >+ nsCOMPtr<nsIChannel> parentChannel; >+ rv = parentDocShell->GetCurrentDocumentChannel(getter_AddRefs(parentChannel)); >+ if (NS_WARN_IF(NS_FAILED(rv))) { >+ return; >+ } This is not useful if+warning either, since apparently GetCurrentDocumentChannel returns always NS_OK, even if parentChannel is null. So you need to null check here, otherwise nsContentUtils::CheckSameOrigin will assert >+ nsCOMPtr<nsIContentViewer> parentContentViewer; >+ rv = parentDocShell->GetContentViewer(getter_AddRefs(parentContentViewer)); >+ if (NS_WARN_IF(NS_FAILED(rv)) || !parentContentViewer) { >+ return; >+ } >+ nsCOMPtr<nsIURI> iframeUri; >+ parentChannel->GetURI(getter_AddRefs(iframeUri)); >+ nsContentUtils::ReportToConsole(nsIScriptError::warningFlag, >+ NS_LITERAL_CSTRING("Iframe Sandbox"), >+ parentContentViewer->GetDocument(), >+ nsContentUtils::eSECURITY_PROPERTIES, >+ "BothAllowScriptsAndSameOriginPresent", >+ nullptr, 0, iframeUri); Unusual way to get document. You could just nsCOMPtr<nsIDocument> doc = do_GetInterface(parentDocShell);

Here's a new revision of the patch, addressing the points raised in comment 72.

Comment on attachment 8459408 [details] [diff] [review] review revision 2 > if (docShell) { > nsresult rv = docShell->GetSandboxFlags(&mSandboxFlags); > NS_ENSURE_SUCCESS(rv, rv); >+ WarnIfSandboxIneffective(docShell, mSandboxFlags, this->GetChannel()); Drop this-> We need some tests for nested iframes. But I think we shouldn't warn in case nested iframes are from different domains. They might not be in control of the web dev (they might be ads or some FB or G+ stuff etc). So, if we have toplevel-page-in-domain-A >>>> iframe-in-domainB >>>> subiframe-in-domainB-with-ineffective-sandbox-flags. I wouldn't expect to see a warning.

Here's a revised patch which removes the unnecessary "this->" and adds the nested tests requested in comment 74. Olli: can you confirm that neither of the following two tests should produce a warning? domainA/warning4.html iframe sandbox="allow-scripts allow-same-origin" src="domainB/nested1.html" iframe src="domainB/inner.html" domainA/warning5.html iframe src="domainB/nested2.html" iframe sandbox="allow-scripts allow-same-origin" src="domainB/inner.html"

The first one shouldn't cause warning because the content of the sandboxed iframe is in different domain, and the second one shouldn't cause warning because the document in which the sandboxed iframe element is in is not the top most document. So looks right to me.

Sorry for the delay in resolving this issue Olli, I was away for all of August. This third revision adds a check to ensure that the parent document is the top-level one. I'm not entirely sure that's the best way to check this, but it does seem to work and all tests pass now.

Comment on attachment 8485607 [details] [diff] [review] review revision 3 >+{ >+ addTab(TEST_URI_WARNING); >+ browser.addEventListener("load", function onLoad(aEvent) { >+ browser.removeEventListener(aEvent.type, onLoad, true); >+ openConsole(null, function testIneffectiveIframeSandboxingLogged (hud) { >+ content.console.log(SENTINEL_MSG) >+ waitForMessages({ Assuming this setup isn't racy (I'm not too familiar with webconsole), r+ I mean, I would expect one should call waitForMessage right after the load has started, but if this other setup works too, fine.

Thanks for picking this up and driving it to the end, Francois !