Re: [webkit-dev] Blob changes to SecurityOrigin.cpp
On Fri, Sep 3, 2010 at 3:19 PM, Jian Li <jianli@google.com> wrote:
Some parts of changes are due to the File API work I have worked on. On Fri, Sep 3, 2010 at 2:50 PM, Adam Barth <abarth@webkit.org> wrote:
I was looking at SecurityOrigin.cpp today and I saw a bunch of code relating to Blob URLs. I don't really understand why this code is correct. Would someone be willing to explain it to me?
Some specific questions:
1) Why do blob URLs get exception from the unique origin check? How does that interact with the HTML5 sandboxing model?
The origin of blob URL is said to be the origin of the page under which the blob URL is created. It is encoded as part of the blob URL: blob:encoded_origin/id. We're not ignoring any security origin checks. Instead, we need to pull the encoded origin out of the blob URL and use it as the base for the origin check.
The reason that we skip the unique origin check here is to allow a local running worker script to be able to access a blob URL. Do we want to disallow this case?
The access rights of locally running content are controlled by a WebCore::Setting. Currently, Chrome sets that setting to the most restrictive value to mitigate the harm a downloaded HTML file can cause. It doesn't seem like a good idea to circumvent that security setting.
If there is a security reason for doing this, I can go ahead to revert this part of change.
Thanks.
2) Why does SecurityOrigin::canLoad take a document as an argument? What are the semantics of this parameter? In particular, why does a SecurityOrigin::canLoad ignore |this| when called with a document argument on a blob URL? That seems like a very bad idea.
SecurityOrigin::canLoad is a static method. Does it have |this| to use?
Oh, that's right. SecurityOrigin::canLoad is junk we moved over from FrameLoader. We need to make it an instance method since all the callers have a document anyway. I don't quite understand what this code is trying to do: bool SecurityOrigin::canLoad(const KURL& url, const String& referrer, Document* document) { #if ENABLE(BLOB) if (url.protocolIs("blob") && document) { SecurityOrigin* documentOrigin = document->securityOrigin(); RefPtr<SecurityOrigin> targetOrigin = SecurityOrigin::create(url); return documentOrigin->isSameSchemeHostPort(targetOrigin.get()); } #endif Why should canLoad care about isSameSchemeHostPort? In the past, canLoad's job was to stop web sites from loading content from your local file system (e.g., in frames or as images). Adam
On Fri, Sep 3, 2010 at 3:43 PM, Adam Barth <abarth@webkit.org> wrote:
On Fri, Sep 3, 2010 at 3:19 PM, Jian Li <jianli@google.com> wrote:
Some parts of changes are due to the File API work I have worked on. On Fri, Sep 3, 2010 at 2:50 PM, Adam Barth <abarth@webkit.org> wrote:
I was looking at SecurityOrigin.cpp today and I saw a bunch of code relating to Blob URLs. I don't really understand why this code is correct. Would someone be willing to explain it to me?
Some specific questions:
1) Why do blob URLs get exception from the unique origin check? How does that interact with the HTML5 sandboxing model?
The origin of blob URL is said to be the origin of the page under which the blob URL is created. It is encoded as part of the blob URL: blob:encoded_origin/id. We're not ignoring any security origin checks. Instead, we need to pull the encoded origin out of the blob URL and use it as the base for the origin check.
The reason that we skip the unique origin check here is to allow a local running worker script to be able to access a blob URL. Do we want to disallow this case?
The access rights of locally running content are controlled by a WebCore::Setting. Currently, Chrome sets that setting to the most restrictive value to mitigate the harm a downloaded HTML file can cause. It doesn't seem like a good idea to circumvent that security setting.
Ok, this sounds right. It means that we should not allow accessing a blob URL from a local worker script per the settings, right? If so, I can get rid of this particular step.
If there is a security reason for doing this, I can go ahead to revert this part of change.
Thanks.
2) Why does SecurityOrigin::canLoad take a document as an argument? What are the semantics of this parameter? In particular, why does a SecurityOrigin::canLoad ignore |this| when called with a document argument on a blob URL? That seems like a very bad idea.
SecurityOrigin::canLoad is a static method. Does it have |this| to use?
Oh, that's right. SecurityOrigin::canLoad is junk we moved over from FrameLoader. We need to make it an instance method since all the callers have a document anyway.
I don't quite understand what this code is trying to do:
bool SecurityOrigin::canLoad(const KURL& url, const String& referrer, Document* document) { #if ENABLE(BLOB) if (url.protocolIs("blob") && document) { SecurityOrigin* documentOrigin = document->securityOrigin(); RefPtr<SecurityOrigin> targetOrigin = SecurityOrigin::create(url); return documentOrigin->isSameSchemeHostPort(targetOrigin.get()); } #endif
Why should canLoad care about isSameSchemeHostPort? In the past, canLoad's job was to stop web sites from loading content from your local file system (e.g., in frames or as images).
Per the File API spec, the blob URL should not be accessed from the page in other security domain. Checking isSameSchemeHostPort will help us enforce this policy when other page in different domain that tries to load this URL from the cache. Probably I need to put better comment here or maybe find a better place to do such check.
Adam
On Fri, Sep 3, 2010 at 4:02 PM, Jian Li <jianli@chromium.org> wrote:
On Fri, Sep 3, 2010 at 3:43 PM, Adam Barth <abarth@webkit.org> wrote:
On Fri, Sep 3, 2010 at 3:19 PM, Jian Li <jianli@google.com> wrote:
The reason that we skip the unique origin check here is to allow a local running worker script to be able to access a blob URL. Do we want to disallow this case?
The access rights of locally running content are controlled by a WebCore::Setting. Currently, Chrome sets that setting to the most restrictive value to mitigate the harm a downloaded HTML file can cause. It doesn't seem like a good idea to circumvent that security setting.
Ok, this sounds right. It means that we should not allow accessing a blob URL from a local worker script per the settings, right? If so, I can get rid of this particular step.
Yes, great.
I don't quite understand what this code is trying to do:
bool SecurityOrigin::canLoad(const KURL& url, const String& referrer, Document* document) { #if ENABLE(BLOB) if (url.protocolIs("blob") && document) { SecurityOrigin* documentOrigin = document->securityOrigin(); RefPtr<SecurityOrigin> targetOrigin = SecurityOrigin::create(url); return documentOrigin->isSameSchemeHostPort(targetOrigin.get()); } #endif
Why should canLoad care about isSameSchemeHostPort? In the past, canLoad's job was to stop web sites from loading content from your local file system (e.g., in frames or as images).
Per the File API spec, the blob URL should not be accessed from the page in other security domain. Checking isSameSchemeHostPort will help us enforce this policy when other page in different domain that tries to load this URL from the cache. Probably I need to put better comment here or maybe find a better place to do such check.
Sorry canLoad is poorly named. I'm working on a patch now to rename it to something more sensible (canDisplay, perhaps?). We use canLoad to control things like <frame src="..."> and <img src="...">. When you say "not accessible," do you mean that they shouldn't be able to be displayed with the image element? Adam
On Fri, Sep 3, 2010 at 4:08 PM, Adam Barth <abarth@webkit.org> wrote:
On Fri, Sep 3, 2010 at 4:02 PM, Jian Li <jianli@chromium.org> wrote:
On Fri, Sep 3, 2010 at 3:43 PM, Adam Barth <abarth@webkit.org> wrote:
On Fri, Sep 3, 2010 at 3:19 PM, Jian Li <jianli@google.com> wrote:
The reason that we skip the unique origin check here is to allow a local running worker script to be able to access a blob URL. Do we want to disallow this case?
The access rights of locally running content are controlled by a WebCore::Setting. Currently, Chrome sets that setting to the most restrictive value to mitigate the harm a downloaded HTML file can cause. It doesn't seem like a good idea to circumvent that security setting.
Ok, this sounds right. It means that we should not allow accessing a blob URL from a local worker script per the settings, right? If so, I can get rid of this particular step.
Yes, great.
I don't quite understand what this code is trying to do:
bool SecurityOrigin::canLoad(const KURL& url, const String& referrer, Document* document) { #if ENABLE(BLOB) if (url.protocolIs("blob") && document) { SecurityOrigin* documentOrigin = document->securityOrigin(); RefPtr<SecurityOrigin> targetOrigin = SecurityOrigin::create(url); return documentOrigin->isSameSchemeHostPort(targetOrigin.get()); } #endif
Why should canLoad care about isSameSchemeHostPort? In the past, canLoad's job was to stop web sites from loading content from your local file system (e.g., in frames or as images).
Per the File API spec, the blob URL should not be accessed from the page in other security domain. Checking isSameSchemeHostPort will help us enforce this policy when other page in different domain that tries to load this URL from the cache. Probably I need to put better comment here or maybe find a better place to do such check.
Sorry canLoad is poorly named. I'm working on a patch now to rename it to something more sensible (canDisplay, perhaps?). We use canLoad to control things like <frame src="..."> and <img src="...">. When you say "not accessible," do you mean that they shouldn't be able to be displayed with the image element?
Yes. Let me use an example to explain this.
Suppose page1 (http://foo/page1) creates a blob URL "blob://id1" from an image file. In page1, blob://id1 is loaded and put into cache when we replace any img element. In page2 (http://bar/page2), it grabs this blob URL in some way and uses it towards its img element. We should not allow this happen per the File API spec. Without this check in canLoad(), page2 has no problem to use it directly from the cache.
Adam
On Fri, Sep 3, 2010 at 4:16 PM, Jian Li <jianli@chromium.org> wrote:
On Fri, Sep 3, 2010 at 4:08 PM, Adam Barth <abarth@webkit.org> wrote:
On Fri, Sep 3, 2010 at 4:02 PM, Jian Li <jianli@chromium.org> wrote:
On Fri, Sep 3, 2010 at 3:43 PM, Adam Barth <abarth@webkit.org> wrote:
On Fri, Sep 3, 2010 at 3:19 PM, Jian Li <jianli@google.com> wrote: I don't quite understand what this code is trying to do:
bool SecurityOrigin::canLoad(const KURL& url, const String& referrer, Document* document) { #if ENABLE(BLOB) if (url.protocolIs("blob") && document) { SecurityOrigin* documentOrigin = document->securityOrigin(); RefPtr<SecurityOrigin> targetOrigin = SecurityOrigin::create(url); return documentOrigin->isSameSchemeHostPort(targetOrigin.get()); } #endif
Why should canLoad care about isSameSchemeHostPort? In the past, canLoad's job was to stop web sites from loading content from your local file system (e.g., in frames or as images).
Per the File API spec, the blob URL should not be accessed from the page in other security domain. Checking isSameSchemeHostPort will help us enforce this policy when other page in different domain that tries to load this URL from the cache. Probably I need to put better comment here or maybe find a better place to do such check.
Sorry canLoad is poorly named. I'm working on a patch now to rename it to something more sensible (canDisplay, perhaps?). We use canLoad to control things like <frame src="..."> and <img src="...">. When you say "not accessible," do you mean that they shouldn't be able to be displayed with the image element?
Yes. Let me use an example to explain this. Suppose page1 (http://foo/page1) creates a blob URL "blob://id1" from an image file. In page1, blob://id1 is loaded and put into cache when we replace any img element. In page2 (http://bar/page2), it grabs this blob URL in some way and uses it towards its img element. We should not allow this happen per the File API spec. Without this check in canLoad(), page2 has no problem to use it directly from the cache.
Got it. Thanks for explaining it to me. So, this code doesn't quite do what you'd like it to do. The problem is that some of the callers of SecurityOrigin::canLoad assume that it applies only for local URLs. That means they'll skip the check you added and plow ahead with some kinds of loads. I'll go through all the callers and remove that assumption. Thanks, Adam
On Fri, Sep 3, 2010 at 4:21 PM, Adam Barth <abarth@webkit.org> wrote:
On Fri, Sep 3, 2010 at 4:16 PM, Jian Li <jianli@chromium.org> wrote:
On Fri, Sep 3, 2010 at 4:08 PM, Adam Barth <abarth@webkit.org> wrote:
On Fri, Sep 3, 2010 at 4:02 PM, Jian Li <jianli@chromium.org> wrote:
On Fri, Sep 3, 2010 at 3:43 PM, Adam Barth <abarth@webkit.org> wrote:
On Fri, Sep 3, 2010 at 3:19 PM, Jian Li <jianli@google.com> wrote: I don't quite understand what this code is trying to do:
bool SecurityOrigin::canLoad(const KURL& url, const String& referrer, Document* document) { #if ENABLE(BLOB) if (url.protocolIs("blob") && document) { SecurityOrigin* documentOrigin = document->securityOrigin(); RefPtr<SecurityOrigin> targetOrigin = SecurityOrigin::create(url); return documentOrigin->isSameSchemeHostPort(targetOrigin.get()); } #endif
Why should canLoad care about isSameSchemeHostPort? In the past, canLoad's job was to stop web sites from loading content from your local file system (e.g., in frames or as images).
Per the File API spec, the blob URL should not be accessed from the page in other security domain. Checking isSameSchemeHostPort will help us enforce this policy when other page in different domain that tries to load this URL from the cache. Probably I need to put better comment here or maybe find a better place to do such check.
Sorry canLoad is poorly named. I'm working on a patch now to rename it to something more sensible (canDisplay, perhaps?). We use canLoad to control things like <frame src="..."> and <img src="...">. When you say "not accessible," do you mean that they shouldn't be able to be displayed with the image element?
Yes. Let me use an example to explain this. Suppose page1 (http://foo/page1) creates a blob URL "blob://id1" from an image file. In page1, blob://id1 is loaded and put into cache when we replace any img element. In page2 (http://bar/page2), it grabs this blob URL in some way and uses it towards its img element. We should not allow this happen per the File API spec. Without this check in canLoad(), page2 has no problem to use it directly from the cache.
Got it. Thanks for explaining it to me.
So, this code doesn't quite do what you'd like it to do. The problem is that some of the callers of SecurityOrigin::canLoad assume that it applies only for local URLs. That means they'll skip the check you added and plow ahead with some kinds of loads.
Also, have you added test cases that include redirects to blob URLs? The code that handles redirects might not be smart enough to handle this additional complexity. Adam
participants (2)
-
Adam Barth
-
Jian Li