Trying to access same-origin content of a sandboxed iframe <iframe src="..." sandbox> gives a misleading error-message. "Unsafe JavaScript attempt to access frame with URL http://domain.com/url1 from frame with URL http://domain.com/url2app/html/app.html. Domains, protocols and ports must match." Error message should reference the sandboxed state to be useful. "Prevented access to sandboxed iframe." In my example, I used the following jQuery: $('#iframeId').contents();
Definitely agree!
Hey, logging improvements! I've taken a quick stab at this. There's a FIXME comment suggesting that the error message generation might be better pushed out to SecurityOrigin; I started in that direction, but I backed out after reading some other comments. Specifically, moving the generation of this error message into SecurityOrigin means that we won't be able to check the sandbox flags directly on the SecurityContext/Documents in question. We'd just be looking at SecurityOrigin::isUnique. Would that be enough? There's a comment on that method noting subtle differences between a sandboxed frame and a frame with a unique origin. WDYT, Adam? I'll upload a patch in a moment with a trivial approach, but if moving it to SecurityOrigin is the better option, I'm happy to go in that direction.
Created attachment 163004 [details] Patch
Comment on attachment 163004 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=163004&action=review > Source/WebCore/page/DOMWindow.cpp:1759 > // FIXME: This error message should contain more specifics of why the same origin check has failed. > // Perhaps we should involve the security origin object in composing it. I'd remove this FIXME. We used to keep the sandbox bits in the SecurityOrigin, which is what this is referring to. > Source/WebCore/page/DOMWindow.cpp:1765 > + if (document()->isSandboxed(SandboxOrigin)) > + return "Sandbox access violation: " + message + " The former frame is sandboxed.\n"; > + if (activeWindow->document()->isSandboxed(SandboxOrigin)) > + return "Sandbox access violation: " + message + " The latter frame is sandboxed.\n"; What if they're both sandboxed? :)
> Specifically, moving the generation of this error message into SecurityOrigin means that we won't be able to check the sandbox flags directly on the SecurityContext/Documents in question. We'd just be looking at SecurityOrigin::isUnique. Would that be enough? The approach you have in this patch is great. I'm surprised you don't need to rebaseline a bunch of sandbox-related tests. > There's a comment on that method noting subtle differences between a sandboxed frame and a frame with a unique origin. WDYT, Adam? Not any more! I fixenated them so they're the same now. We can probably remove that comment. > I'll upload a patch in a moment with a trivial approach, but if moving it to SecurityOrigin is the better option, I'm happy to go in that direction. I think the version in this patch is great. We might want to elaborate the message more later, but this is a good improvement.
(In reply to comment #4) > (From update of attachment 163004 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=163004&action=review > > > Source/WebCore/page/DOMWindow.cpp:1759 > > // FIXME: This error message should contain more specifics of why the same origin check has failed. > > // Perhaps we should involve the security origin object in composing it. > > I'd remove this FIXME. We used to keep the sandbox bits in the SecurityOrigin, which is what this is referring to. Will do. > > > Source/WebCore/page/DOMWindow.cpp:1765 > > + if (document()->isSandboxed(SandboxOrigin)) > > + return "Sandbox access violation: " + message + " The former frame is sandboxed.\n"; > > + if (activeWindow->document()->isSandboxed(SandboxOrigin)) > > + return "Sandbox access violation: " + message + " The latter frame is sandboxed.\n"; > > What if they're both sandboxed? :) I can add that in. ;)
(In reply to comment #5) > > Specifically, moving the generation of this error message into SecurityOrigin means that we won't be able to check the sandbox flags directly on the SecurityContext/Documents in question. We'd just be looking at SecurityOrigin::isUnique. Would that be enough? > > The approach you have in this patch is great. I'm surprised you don't need to rebaseline a bunch of sandbox-related tests. I might. I only ran the sandbox* tests locally. I was hoping the bot would let me know what else broke. I'll just do a full pass before landing this, I guess. > > There's a comment on that method noting subtle differences between a sandboxed frame and a frame with a unique origin. WDYT, Adam? > > Not any more! I fixenated them so they're the same now. We can probably remove that comment. They are slightly different, though. http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/page/SecurityOrigin.cpp&exact_package=chromium&q=SecurityOrigin&type=cs&l=102 spells out a few cases where SecurityOrigin::isUnique would be true without the frame being sandboxed. > > I'll upload a patch in a moment with a trivial approach, but if moving it to SecurityOrigin is the better option, I'm happy to go in that direction. > > I think the version in this patch is great. We might want to elaborate the message more later, but this is a good improvement. What would you like to see added? I'm happy to make more changes now while I'm already rebasing tests. :)
> What would you like to see added? I'm happy to make more changes now while I'm already rebasing tests. :) One corner case that would be good to cover is when document.domain is set. There are some counter-intuitive cases there. For example, http://www.example.com/foo might not be able to access http://www.example.com/bar if bar sets document.domain to example.com. The even stranger case is when http://example.com/foo can't talk with http://www.example.com/bar even after bar sets document.domain to example.com. The rule is that either the both need to have set document.domain or neither of them can have set it.
Created attachment 163064 [details] Patch
(In reply to comment #8) > > What would you like to see added? I'm happy to make more changes now while I'm already rebasing tests. :) > > One corner case that would be good to cover is when document.domain is set. There are some counter-intuitive cases there. For example, http://www.example.com/foo might not be able to access http://www.example.com/bar if bar sets document.domain to example.com. > > The even stranger case is when http://example.com/foo can't talk with http://www.example.com/bar even after bar sets document.domain to example.com. The rule is that either the both need to have set document.domain or neither of them can have set it. Makes sense. I think I'll take that in a separate patch, if you don't mind, just to keep this one focused on sandboxing: I've filed https://bugs.webkit.org/show_bug.cgi?id=96247 to cover it. I've rebased the patch onto ToT, updated a few more test baselines, and slightly improved the wording of the error. Let's see what the bots think.
Comment on attachment 163064 [details] Patch Bots are happy. CQ?
Comment on attachment 163064 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=163064&action=review > LayoutTests/platform/chromium/http/tests/security/sandboxed-iframe-modify-self-expected.txt:3 > -CONSOLE MESSAGE: Unsafe JavaScript attempt to access frame with URL http://127.0.0.1:8000/security/sandboxed-iframe-modify-self.html from frame with URL http://127.0.0.1:8000/security/resources/sandboxed-iframe-modify-self.html. Domains, protocols and ports must match. > +CONSOLE MESSAGE: Sandbox access violation: Unsafe JavaScript attempt to access frame with URL http://127.0.0.1:8000/security/sandboxed-iframe-modify-self.html from frame with URL http://127.0.0.1:8000/security/resources/sandboxed-iframe-modify-self.html. The frame requesting access is sandboxed into a unique origin. We will probably need to rebaseline the non-chromium versions of these results. I'll watch the tree and take care of it.
Comment on attachment 163064 [details] Patch Clearing flags on attachment: 163064 Committed r128070: <http://trac.webkit.org/changeset/128070>
All reviewed patches have been landed. Closing bug.