Bug 71745 – Unique SecurityOrigins shouldn't remember their old schemes and hosts

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Adam Barth]

We should create unique security origins with SecurityOrigin::createUnique rather than just setting a flag on SecurityOrigin.

[Adam Barth]

Created attachment 113950 [details]
work in progress

[Adam Barth]

Created attachment 114372 [details]
more work in progress

[Adam Barth]

Created attachment 114431 [details]
Patch

[Adam Barth]

Created attachment 114438 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 114431 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=114431&action=review

I think I need to read this again in the morning.  I also think this might need another round.

> Source/WebCore/ChangeLog:65
> +            - Whenever we set a SecurityOrigin explicitly, we have not longer

not longer -> no longer?

> Source/WebCore/dom/Document.cpp:4393
>  void Document::initSecurityContext()

You noted in our discussion that this should move to SecurityContext.  Seems you should note that in a FIXME here.  It seems this also deserves a FIXME about breaking this up into smaller pieces. :)  "init" is also not a very descriptive name.  I'm not sure I have a better one though.  "compute", "inherit", not sure.  Anyway, that can all be a separate patch too.

> Source/WebCore/dom/Document.cpp:4457
> +        didFailToInheritSecurityOrigin();

Maybe this is not quite the right name.  It's less about recording what you have done, and more about giving yourself a special permission to try again... no?

> Source/WebCore/dom/Document.cpp:4462
> +    // We alias the SecurityOrigins to match Firefox, see Bug 15313

period, not sure you should capitalize Bug either, or simply write "see:" then you don't need a period or the bug number :)

> Source/WebCore/dom/Document.cpp:4465
> +    // FIXME: Consider moving m_contentSecurityPolicy into SecurityOrigin.

Seems like you should be more definite.  You're the one doing the considering. :)

> Source/WebCore/dom/SecurityContext.cpp:59
> +    if (m_failedToInheritSecurityOrigin)

Yeah, I really think this "I have special powers" bool needs a better name.

> Source/WebCore/dom/SecurityContext.cpp:63
> +    RefPtr<SecurityOrigin> other = SecurityOrigin::create(url);
> +    return securityOrigin()->canAccess(other.get());

Really?  A malloc and ref just to check canAccess?

> Source/WebCore/page/SecurityOrigin.cpp:53
> +    // FIXME: Should we use the inner HTML even when BLOB and FILE_SYSTEM are disabled?

You mean inner URL?  Yes.  Seems we should. :)

> Source/WebCore/page/SecurityOrigin.cpp:92
> +        && (!effectiveURL.hasPath() || effectiveURL.path().endsWith("/"))) {

Why the endsWith check instead of something more semantic?  Like "isDirectory" or whatever that's trying to mean?

> Source/WebCore/page/SecurityOrigin.cpp:106
> +    , m_isUnique(!url.isValid())

I don't like that we use this in-band data.  I would prefer to have a separate constructor for unique origins.  That obviously makes it possible to forget to initialize some variable in one or the other (since c++ is stupid), but I think it cleaner.  You'd use an enum type, with a single value like "IsUnique" or similar?  This could be a separate patch of course.

> Source/WebCore/page/SecurityOrigin.cpp:118
> +    if (isDefaultPortForProtocol(m_port, m_protocol))
> +        m_port = 0;

Is 0 always an invalid port?  (May seem like a dumb quesiton, but I think of X display ports, which I think start with 0.)  Seems a constant might be useful here.  InvalidPort?

[Adam Barth]

> > Source/WebCore/page/SecurityOrigin.cpp:118
> > +    if (isDefaultPortForProtocol(m_port, m_protocol))
> > +        m_port = 0;
> 
> Is 0 always an invalid port?

Yes.  Specifically, it's the value KURL uses to represent a missing port.

> InvalidPort?

Done.

[Adam Barth]

> > Source/WebCore/ChangeLog:65
> > +            - Whenever we set a SecurityOrigin explicitly, we have not longer
> 
> not longer -> no longer?

Fixed.

> > Source/WebCore/dom/Document.cpp:4393
> >  void Document::initSecurityContext()
> 
> You noted in our discussion that this should move to SecurityContext.  Seems you should note that in a FIXME here.  It seems this also deserves a FIXME about breaking this up into smaller pieces. :)  "init" is also not a very descriptive name.  I'm not sure I have a better one though.  "compute", "inherit", not sure.  Anyway, that can all be a separate patch too.

I added a FIXME.  Hopefully when we move it to SecurityContext and break it into pieces, we'll think of better names.

> > Source/WebCore/dom/Document.cpp:4462
> > +    // We alias the SecurityOrigins to match Firefox, see Bug 15313
> 
> period, not sure you should capitalize Bug either, or simply write "see:" then you don't need a period or the bug number :)

Fixed.

> > Source/WebCore/dom/Document.cpp:4465
> > +    // FIXME: Consider moving m_contentSecurityPolicy into SecurityOrigin.
> 
> Seems like you should be more definite.  You're the one doing the considering. :)

I've removed the FIXME.  This is on SecurityContext now, which is correct.

> > Source/WebCore/dom/SecurityContext.cpp:63
> > +    RefPtr<SecurityOrigin> other = SecurityOrigin::create(url);
> > +    return securityOrigin()->canAccess(other.get());
> 
> Really?  A malloc and ref just to check canAccess?

This happens very rarely, actually, because it only happens when the FrameLoader is in the DisplayingInitialDocument state.

> > Source/WebCore/page/SecurityOrigin.cpp:53
> > +    // FIXME: Should we use the inner HTML even when BLOB and FILE_SYSTEM are disabled?
> 
> You mean inner URL?  Yes.  Seems we should. :)

Fixed.

> > Source/WebCore/page/SecurityOrigin.cpp:92
> > +        && (!effectiveURL.hasPath() || effectiveURL.path().endsWith("/"))) {
> 
> Why the endsWith check instead of something more semantic?  Like "isDirectory" or whatever that's trying to mean?

Fixed.

> > Source/WebCore/page/SecurityOrigin.cpp:106
> > +    , m_isUnique(!url.isValid())
> 
> I don't like that we use this in-band data.  I would prefer to have a separate constructor for unique origins.  That obviously makes it possible to forget to initialize some variable in one or the other (since c++ is stupid), but I think it cleaner.  You'd use an enum type, with a single value like "IsUnique" or similar?  This could be a separate patch of course.

Fixed.

[WebKit Review Bot]

Comment on attachment 114438 [details]
Patch

Attachment 114438 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10395252

New failing tests:
http/tests/inspector/compiler-source-mapping-provider.html
media/video-controls-no-scripting.html
editing/pasteboard/drag-drop-dead-frame.html

[WebKit Review Bot]

Comment on attachment 114431 [details]
Patch

Attachment 114431 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10399260

New failing tests:
fast/frames/sandboxed-iframe-storage.html
fast/frames/sandboxed-iframe-navigation-top-by-constant-name.html
fast/frames/sandboxed-iframe-navigation-top-by-constant-name2.html
http/tests/inspector/compiler-source-mapping-provider.html
fast/frames/sandboxed-iframe-navigation-top.html
fast/frames/sandboxed-iframe-navigation-top-by-name.html
media/video-controls-no-scripting.html
editing/pasteboard/drag-drop-dead-frame.html

[Adam Barth]

> > Source/WebCore/dom/SecurityContext.cpp:59
> > +    if (m_failedToInheritSecurityOrigin)
> 
> Yeah, I really think this "I have special powers" bool needs a better name.

Ok, I've tried a new name.

[Adam Barth]

Created attachment 114445 [details]
Patch

[WebKit Review Bot]

Comment on attachment 114445 [details]
Patch

Attachment 114445 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10394271

New failing tests:
http/tests/inspector/compiler-source-mapping-provider.html
editing/pasteboard/drag-drop-dead-frame.html

[Adam Barth]

Created attachment 114523 [details]
Patch

[Adam Barth]

There's still an issue with compiler-source-mapping-provider.html that I need to investigate.

[Eric Seidel (no email)]

Comment on attachment 114523 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=114523&action=review

> Source/WebCore/page/SecurityOrigin.cpp:98
> +    // We use unique origins for directory listings to make it harder to crawl a local filesystem.
> +    if (SchemeRegistry::shouldTreatURLSchemeAsLocal(protocol)
> +        && !shouldUseInnerURL(url) // Note the use of url rather than effectiveURL.
> +        && (!effectiveURL.hasPath() || isDirectory(effectiveURL.path()))) {

I don't understand this strange heuristic?  Do we have tests for this?

> LayoutTests/ChangeLog:23
> +            - Previously sandboxed local iframes still got universal access
> +              when we're running with universal access for file URLs!  Now that
> +              they correctly get unique origins, we need to update these tests
> +              to allow-same-origin access in order for them to function
> +              properly.

So did you update the tests, or do we still need to?

[Darin Adler]

Comment on attachment 114523 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=114523&action=review

I think you could do some preparation steps and make the patch a bit smaller, especially the changes to Document.cpp. Not sure it’s worthwhile.

> Source/WebCore/dom/Document.cpp:1732
> -    ScriptExecutionContext::setSecurityOrigin(SecurityOrigin::create(url(), true));
> +    ScriptExecutionContext::setSecurityOrigin(SecurityOrigin::createUnique());

This change could be done in a separate initial step where we introduce the new create functions but otherwise make no changes.

> Source/WebCore/dom/Document.cpp:4414
> -    ScriptExecutionContext::setSecurityOrigin(SecurityOrigin::create(m_url, isSandboxed(SandboxOrigin)));
> +    ScriptExecutionContext::setSecurityOrigin(isSandboxed(SandboxOrigin) ? SecurityOrigin::createUnique() : SecurityOrigin::create(m_url));

This change could be done in a separate initial step where we introduce the new create functions but otherwise make no changes.

> Source/WebCore/dom/Document.cpp:4448
> -    if (!securityOrigin()->isEmpty())
> +    // FIXME: We should not inherit our security origin if we're sandboxed.
> +    if (m_url.isValid() && !m_url.protocolIs("about"))
>          return;

Is there a way to write this code so that the “why” is self-explanatory? The explicit mention of "about" here is super-mysterious!

> Source/WebCore/dom/Document.cpp:4462
> +    if (!ownerFrame) {
> +        didFailToInitializeSecurityOrigin();
> +        return;
>      }

Changing this to an early return in a first patch would make this patch easier to read; it might be clearer that the only thing added is a call to didFailToInitializeSecurityOrigin. As it is, the change of indentation hides whether there is anything changing in the code that is being unindented.

> Source/WebCore/dom/SecurityContext.h:63
> +    bool isSecureTransitionTo(const KURL&) const;

Could a separate patch move this function here to SecurityContext without changing its behavior?

> Source/WebCore/page/DOMWindow.cpp:881
>          target = SecurityOrigin::createFromString(targetOrigin);
> -        if (target->isEmpty()) {
> +        if (target->isUnique()) {

Could we find a way to be clearer that this is about failure to create a security origin from a string? It seems unpleasant that the caller needs to know specifically what kind of bad security origin is returned to indicate failure. Maybe the best design is to indicate failure separately instead of with a special value, even though unique origin is available. It’s often considered better design to avoid “in-band signaling”.

> Source/WebCore/page/SecurityOrigin.cpp:49
> +    return url.protocolIs("http") || url.protocolIs("https") || url.protocolIs("ftp");

I prefer use of protocolIsInHTTPFamily for call sites like this.

Despite the clear name, I think a brief why comment would help here.

> Source/WebCore/page/SecurityOrigin.cpp:64
> +static bool shouldUseInnerURL(const KURL& url)
> +{
> +    // FIXME: Use the inner URL even when BLOB and FILE_SYSTEM are disabled.
> +#if ENABLE(BLOB)
> +    if (url.protocolIs("blob"))
> +        return true;
> +#endif
> +#if ENABLE(FILE_SYSTEM)
> +    if (url.protocolIs("filesystem"))
> +        return true;
> +#endif
> +    return false;
> +}

To me the relationship between these protocols and the concept “use inner URL” is unclear, so I’d like to see a why comment that explains what an inner URL is and what the connection is. For example, this function can’t correctly answer the question whether this URL has an inner URL. It can only answer the question of whether it is known to use an inner URL, or the question of whether it uses an inner URL and has processing implemented by WebKit itself.

> Source/WebCore/page/SecurityOrigin.h:170
> +    enum UniqueOriginTag {
> +        UniqueOrigin,
> +    };

I’d do this all on one line, and omit the comma.

Also, I’m not sure that this is significantly better than using a private constructor with no arguments. If it was public, then I clearly see the value of the tag.

> Source/WebKit/chromium/src/WebSecurityOrigin.cpp:93
> +// FIXME: Renamed this API to isUnique().

Typo: "Renamed".

[Darin Adler]

It’d be really good if the changes to policy were isolated from the refactoring. I can’t tell whether this patch includes any changes to policy.

[Adam Barth]

(In reply to comment #15)
> (From update of attachment 114523 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=114523&action=review
> 
> > Source/WebCore/page/SecurityOrigin.cpp:98
> > +    // We use unique origins for directory listings to make it harder to crawl a local filesystem.
> > +    if (SchemeRegistry::shouldTreatURLSchemeAsLocal(protocol)
> > +        && !shouldUseInnerURL(url) // Note the use of url rather than effectiveURL.
> > +        && (!effectiveURL.hasPath() || isDirectory(effectiveURL.path()))) {
> 
> I don't understand this strange heuristic?  Do we have tests for this?

I believe so.  I can hunt around for their names if that would be helpful.  Generally, this is trying to catch these sorts of URLs:

A) file:///Users/fred/

versus these sort of URLs:

B) file:///Users/fred/Downloads/resume.html
C) filesystem:file:///temporary/directory/

For the URLs of type A, the browser supplies a directory listing, which we place in a unique origin.  That does a couple things:

1) Any bugs in the directory listing code aren't exploitable to read the contents of your local filesystem.
2) A malicious local HTML file (e.g., a downloaded resume) can't use these directory listings to crawl your filesystem.  Instead, they need to guess at paths.

For URLs for type B, their security model is controlled by some settings.  For example, Chrome and Safari place them in a unique origin too, but other Mac OS X apps might use WebKit's legacy behavior of granting them more privileges.

For URLs of type C (the nested case), we're not worried about crawling the file system because we're not really dealing with the user's local file system.  In the case of "filesystem", we're dealing with per-origin sandboxed file system, here the one associated with the "file" origin.  The Blob case is similar, in that there isn't any directory listing to protect.

> > LayoutTests/ChangeLog:23
> > +            - Previously sandboxed local iframes still got universal access
> > +              when we're running with universal access for file URLs!  Now that
> > +              they correctly get unique origins, we need to update these tests
> > +              to allow-same-origin access in order for them to function
> > +              properly.
> 
> So did you update the tests, or do we still need to?

Yes, all the tests should be updated.  I can add some new tests targeted at this issue if that would be helpful.

[Adam Barth]

> I think you could do some preparation steps and make the patch a bit smaller, especially the changes to Document.cpp. Not sure it’s worthwhile.

I've done a few of those already, but I can look at this patch again to see if I can do more.

> > Source/WebCore/dom/Document.cpp:1732
> > -    ScriptExecutionContext::setSecurityOrigin(SecurityOrigin::create(url(), true));
> > +    ScriptExecutionContext::setSecurityOrigin(SecurityOrigin::createUnique());
> 
> This change could be done in a separate initial step where we introduce the new create functions but otherwise make no changes.

Ok.

> > Source/WebCore/dom/Document.cpp:4414
> > -    ScriptExecutionContext::setSecurityOrigin(SecurityOrigin::create(m_url, isSandboxed(SandboxOrigin)));
> > +    ScriptExecutionContext::setSecurityOrigin(isSandboxed(SandboxOrigin) ? SecurityOrigin::createUnique() : SecurityOrigin::create(m_url));
> 
> This change could be done in a separate initial step where we introduce the new create functions but otherwise make no changes.

Ok.

> > Source/WebCore/dom/Document.cpp:4448
> > -    if (!securityOrigin()->isEmpty())
> > +    // FIXME: We should not inherit our security origin if we're sandboxed.
> > +    if (m_url.isValid() && !m_url.protocolIs("about"))
> >          return;
> 
> Is there a way to write this code so that the “why” is self-explanatory? The explicit mention of "about" here is super-mysterious!

I can certainly split the check off into a helper function with a descriptive name.  We can also link to the relevant requirement in HTML5.

> > Source/WebCore/dom/Document.cpp:4462
> > +    if (!ownerFrame) {
> > +        didFailToInitializeSecurityOrigin();
> > +        return;
> >      }
> 
> Changing this to an early return in a first patch would make this patch easier to read; it might be clearer that the only thing added is a call to didFailToInitializeSecurityOrigin. As it is, the change of indentation hides whether there is anything changing in the code that is being unindented.

Sure.  That's easy to do.

> > Source/WebCore/dom/SecurityContext.h:63
> > +    bool isSecureTransitionTo(const KURL&) const;
> 
> Could a separate patch move this function here to SecurityContext without changing its behavior?

Yes.

> > Source/WebCore/page/DOMWindow.cpp:881
> >          target = SecurityOrigin::createFromString(targetOrigin);
> > -        if (target->isEmpty()) {
> > +        if (target->isUnique()) {
> 
> Could we find a way to be clearer that this is about failure to create a security origin from a string? It seems unpleasant that the caller needs to know specifically what kind of bad security origin is returned to indicate failure. Maybe the best design is to indicate failure separately instead of with a special value, even though unique origin is available. It’s often considered better design to avoid “in-band signaling”.

We can return null if the string fails to parse, if that's what you're getting at.

> > Source/WebCore/page/SecurityOrigin.cpp:49
> > +    return url.protocolIs("http") || url.protocolIs("https") || url.protocolIs("ftp");
> 
> I prefer use of protocolIsInHTTPFamily for call sites like this.

Will do.

> Despite the clear name, I think a brief why comment would help here.

Ok.

> > Source/WebCore/page/SecurityOrigin.cpp:64
> > +static bool shouldUseInnerURL(const KURL& url)
> > +{
> > +    // FIXME: Use the inner URL even when BLOB and FILE_SYSTEM are disabled.
> > +#if ENABLE(BLOB)
> > +    if (url.protocolIs("blob"))
> > +        return true;
> > +#endif
> > +#if ENABLE(FILE_SYSTEM)
> > +    if (url.protocolIs("filesystem"))
> > +        return true;
> > +#endif
> > +    return false;
> > +}
> 
> To me the relationship between these protocols and the concept “use inner URL” is unclear, so I’d like to see a why comment that explains what an inner URL is and what the connection is.

I'll add a comment and link to the specs for these schemes.

> For example, this function can’t correctly answer the question whether this URL has an inner URL. It can only answer the question of whether it is known to use an inner URL, or the question of whether it uses an inner URL and has processing implemented by WebKit itself.

I'll rename the function to be clear that this function returns an answer that's to the best of its knowledge.

> > Source/WebCore/page/SecurityOrigin.h:170
> > +    enum UniqueOriginTag {
> > +        UniqueOrigin,
> > +    };
> 
> I’d do this all on one line, and omit the comma.

Ok.

> Also, I’m not sure that this is significantly better than using a private constructor with no arguments. If it was public, then I clearly see the value of the tag.

I'm happy to make that change.

> > Source/WebKit/chromium/src/WebSecurityOrigin.cpp:93
> > +// FIXME: Renamed this API to isUnique().
> 
> Typo: "Renamed".

Thanks.

> It’d be really good if the changes to policy were isolated from the refactoring. I can’t tell whether this patch includes any changes to policy.

Your comments above are good suggestions for breaking this change down into smaller pieces.  I'll make those changes first and then take another stab at making the core part of the change in smaller pieces.

Thanks for the review.

[Darin Adler]

(In reply to comment #19)
> > > Source/WebCore/page/DOMWindow.cpp:881
> > >          target = SecurityOrigin::createFromString(targetOrigin);
> > > -        if (target->isEmpty()) {
> > > +        if (target->isUnique()) {
> > 
> > Could we find a way to be clearer that this is about failure to create a security origin from a string? It seems unpleasant that the caller needs to know specifically what kind of bad security origin is returned to indicate failure. Maybe the best design is to indicate failure separately instead of with a special value, even though unique origin is available. It’s often considered better design to avoid “in-band signaling”.
> 
> We can return null if the string fails to parse, if that's what you're getting at.

I’d make it more like this:

    static bool parseXXX(const String& string, SecurityOrigin& origin);

The boolean would be true if you can parse. The XXX would be words for whatever we call the string that describes a security origin.

Not sure if that becomes too inelegant, though.

> I'll rename the function to be clear that this function returns an answer that's to the best of its knowledge.

I think another point important for someone reading the code is: What are the consequences for getting the answer wrong?

[Darin Adler]

(In reply to comment #20)
> > I'll rename the function to be clear that this function returns an answer that's to the best of its knowledge.
> 
> I think another point important for someone reading the code is: What are the consequences for getting the answer wrong?

For example, lets say I find a W3C spec that makes it clear some particular URL has this kind of inner URL. Is it helpful to the WebKit project to add it to this function?

[Adam Barth]

> I’d make it more like this:
> 
>     static bool parseXXX(const String& string, SecurityOrigin& origin);
> 
> The boolean would be true if you can parse. The XXX would be words for whatever we call the string that describes a security origin.
> 
> Not sure if that becomes too inelegant, though.

I need to look at the spec to see why we're supposed to throw this exception here.  For most of the callers, returning a unique origin is the right behavior for this function.  This seems to be the only caller that wants to do something unusual in the "fails-to-parse" case.

> > I'll rename the function to be clear that this function returns an answer that's to the best of its knowledge.
> 
> I think another point important for someone reading the code is: What are the consequences for getting the answer wrong?

The consequences depend on whether the engine understands these sorts of URLs in general.  For Blob and Filesystem, other parts of WebKit rip them apart and look inside.  If WebKit doesn't understand the URLs, there isn't too bad that will happen.

None of these issues are new in this patch.  This patch just refactors the code to make it clearer what's going on.

> For example, lets say I find a W3C spec that makes it clear some particular URL has this kind of inner URL. Is it helpful to the WebKit project to add it to this function?

That depends whether WebKit supports that spec.  For example, jar is another scheme that has an inner URL.  In Firefox's implementation of this function, they list jar because Firefox implements the rest of the jar protocol.  Similarly, Firefox doesn't list "filesystem" in their implementation of this function because they don't yet support the Filesystem API.

I'll add some more documentation to the code along these lines.

[Adam Barth]

Created attachment 115710 [details]
Work in progress (needs WebCore.exp.in tweaks)

[Eric Seidel (no email)]

Comment on attachment 115710 [details]
Work in progress (needs WebCore.exp.in tweaks)

LGTM.

[Adam Barth]

Created attachment 115712 [details]
Patch

[WebKit Review Bot]

Comment on attachment 115712 [details]
Patch

Clearing flags on attachment: 115712

Committed r100716: <http://trac.webkit.org/changeset/100716>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.