Bug 79704 - Restrict access to notifications for unique origins and file URLs with no local file access
Summary: Restrict access to notifications for unique origins and file URLs with no loc...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.7
: P2 Normal
Assignee: Jon Lee
URL:
Keywords: InRadar
Depends on: 81708
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-27 14:21 PST by Jon Lee
Modified: 2012-03-27 13:16 PDT (History)
13 users (show)

See Also:


Attachments
Patch (18.45 KB, patch)
2012-02-27 16:26 PST, Jon Lee
no flags Details | Formatted Diff | Diff
Patch (2.34 KB, patch)
2012-02-28 02:56 PST, Jon Lee
no flags Details | Formatted Diff | Diff
Patch (4.75 KB, patch)
2012-02-28 13:56 PST, Jon Lee
no flags Details | Formatted Diff | Diff
WIP (3.98 KB, patch)
2012-02-28 16:42 PST, Jon Lee
no flags Details | Formatted Diff | Diff
WIP #2 (3.97 KB, patch)
2012-02-29 10:16 PST, Jon Lee
no flags Details | Formatted Diff | Diff
Patch (5.87 KB, patch)
2012-03-19 00:52 PDT, Jon Lee
no flags Details | Formatted Diff | Diff
Patch (17.28 KB, patch)
2012-03-20 11:16 PDT, Jon Lee
no flags Details | Formatted Diff | Diff
Rename enums to try to fix GTK (17.26 KB, patch)
2012-03-20 17:04 PDT, Jon Lee
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jon Lee 2012-02-27 14:21:57 PST
For now, this includes geolocation and notifications.

<rdar://problem/10912430>
Comment 1 Jon Lee 2012-02-27 16:26:40 PST
Created attachment 129123 [details]
Patch
Comment 2 Jon Lee 2012-02-28 02:10:46 PST
After some discussion, reconsidering approach to this issue. Renaming appropriately.
Comment 3 Jon Lee 2012-02-28 02:56:24 PST
Created attachment 129219 [details]
Patch
Comment 4 Adam Barth 2012-02-28 08:55:21 PST
Comment on attachment 129219 [details]
Patch

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

> Source/WebKit2/WebProcess/Notifications/WebNotificationManager.cpp:102
> +    if (originString == "null")

Why not origin->isUnique() or add origin->canPresentNotifications?  Testing against a string constant is kind of ugly...
Comment 5 Jon Lee 2012-02-28 09:50:45 PST
Comment on attachment 129219 [details]
Patch

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

>> Source/WebKit2/WebProcess/Notifications/WebNotificationManager.cpp:102
>> +    if (originString == "null")
> 
> Why not origin->isUnique() or add origin->canPresentNotifications?  Testing against a string constant is kind of ugly...

The first would only catch one of the two cases, and I was reluctant to expose a bit needed to detect the file:// case. As for the second, I don't think we want to pollute the SecurityOrigin interface with a function specific to this feature.

The string literal isn't great, but maybe instead we can refactor all uses of it (in SecurityOrigin::toString()) to a constant.
Comment 6 Jon Lee 2012-02-28 10:40:58 PST
(In reply to comment #5)
> (From update of attachment 129219 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=129219&action=review
> 
> >> Source/WebKit2/WebProcess/Notifications/WebNotificationManager.cpp:102
> >> +    if (originString == "null")
> > 
> > Why not origin->isUnique() or add origin->canPresentNotifications?  Testing against a string constant is kind of ugly...
> 
> The first would only catch one of the two cases, and I was reluctant to expose a bit needed to detect the file:// case. As for the second, I don't think we want to pollute the SecurityOrigin interface with a function specific to this feature.
> 
> The string literal isn't great, but maybe instead we can refactor all uses of it (in SecurityOrigin::toString()) to a constant.

Talked with ap offline, and we agreed to add a new function to SecurityOrigin as you suggested, Adam.
Comment 7 Jon Lee 2012-02-28 13:56:32 PST
Created attachment 129325 [details]
Patch
Comment 8 Alexey Proskuryakov 2012-02-28 14:01:54 PST
Comment on attachment 129325 [details]
Patch

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

> Source/WebCore/page/SecurityOrigin.cpp:360
> +    return !isUnique() && !m_enforceFilePathSeparation;

When is the second part of the test important?

> Source/WebKit2/WebProcess/Notifications/WebNotificationManager.cpp:100
> +    if (!origin->canShowNotifications())
> +        return NotificationPresenter::PermissionDenied;

It's quite confusing that there is a method on SecurityOrigin that only affects WK2. Do we want it to be used in all ports?
Comment 9 Jon Lee 2012-02-28 14:34:44 PST
Comment on attachment 129325 [details]
Patch

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

>> Source/WebCore/page/SecurityOrigin.cpp:360
>> +    return !isUnique() && !m_enforceFilePathSeparation;
> 
> When is the second part of the test important?

It looks like file URLs are not treated as unique origins by default (see SchemeRegistry.cpp: schemesWithUniqueOrigins()). So with just !isUnique() alone this test will fail if file URLs have restrictions. The second part of the test ensures that file URLs with local file access restrictions are prevented from showing notifications. See how in SecurityOrigin::toString() there is a check for this bit to return a similar string as for unique origins.

>> Source/WebKit2/WebProcess/Notifications/WebNotificationManager.cpp:100
>> +        return NotificationPresenter::PermissionDenied;
> 
> It's quite confusing that there is a method on SecurityOrigin that only affects WK2. Do we want it to be used in all ports?

For WK1 the policy can be enforced by the provider, which is left to the port to implement. But for symmetry I can add that check in WebKit as well.
Comment 10 Jon Lee 2012-02-28 16:42:08 PST
Created attachment 129353 [details]
WIP
Comment 11 WebKit Review Bot 2012-02-28 17:34:38 PST
Comment on attachment 129353 [details]
WIP

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

New failing tests:
fast/notifications/notifications-document-close-crash.html
Comment 12 Jon Lee 2012-02-29 10:16:38 PST
Created attachment 129469 [details]
WIP #2
Comment 13 Jon Lee 2012-03-19 00:52:52 PDT
Created attachment 132555 [details]
Patch
Comment 14 Adam Barth 2012-03-19 00:57:31 PDT
Comment on attachment 132555 [details]
Patch

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

> Source/WebCore/notifications/NotificationCenter.cpp:62
> +    if (scriptExecutionContext()->securityOrigin()->isLocal())
> +        return NotificationClient::PermissionAllowed;

Why should local URLs be automatically whitelisted to have PermissionAllowed?  That sounds like a security decision that should either be made by SecurityOrigin or the client (probably the client).

> Source/WebCore/notifications/NotificationCenter.cpp:70
> +    if (!scriptExecutionContext()->securityOrigin()->canShowNotifications() || scriptExecutionContext()->securityOrigin()->isLocal()) {

We shouldn't be magically whitelisting isLocal.

> Source/WebCore/notifications/NotificationCenter.cpp:71
> +        callback->handleEvent();

Does this mean we're calling the callback synchronously when before we called it async?

> Source/WebCore/page/SecurityOrigin.cpp:360
> +    return !isUnique() && (m_universalAccess || !m_enforceFilePathSeparation);

I'm not sure I understand this condition.  Is this the same thing we use for, e.g., localStorage?

> Source/WebCore/page/SecurityOrigin.h:117
>      bool canAccessCookies() const { return !isUnique(); }
>      bool canAccessPasswordManager() const { return !isUnique(); }
>      bool canAccessFileSystem() const { return !isUnique(); }

Why is canShowNotifications different from the above?  Should we change the above?  I think it make sense allow access to these things when m_universalAccess is true, but I don't understand the connection with m_enforceFilePathSeparation.
Comment 15 Jon Lee 2012-03-19 12:32:14 PDT
Comment on attachment 132555 [details]
Patch

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

>> Source/WebCore/notifications/NotificationCenter.cpp:62
>> +        return NotificationClient::PermissionAllowed;
> 
> Why should local URLs be automatically whitelisted to have PermissionAllowed?  That sounds like a security decision that should either be made by SecurityOrigin or the client (probably the client).

By the point we do the isLocal() check, we will have gone through the canShowNotifications() check, which will only be true for local protocols if they have been granted universal access, and do not have the file url restriction.

>> Source/WebCore/notifications/NotificationCenter.cpp:70
>> +    if (!scriptExecutionContext()->securityOrigin()->canShowNotifications() || scriptExecutionContext()->securityOrigin()->isLocal()) {
> 
> We shouldn't be magically whitelisting isLocal.

Should be covered by the canShowNotifications() check. See below.

>> Source/WebCore/notifications/NotificationCenter.cpp:71
>> +        callback->handleEvent();
> 
> Does this mean we're calling the callback synchronously when before we called it async?

Oops, you're right. It should be async.

>> Source/WebCore/page/SecurityOrigin.cpp:360
>> +    return !isUnique() && (m_universalAccess || !m_enforceFilePathSeparation);
> 
> I'm not sure I understand this condition.  Is this the same thing we use for, e.g., localStorage?

No, it is not the same. It looks like there was a special exception added for local/file protocols, which caused the security origin for file URLs to appear unique, even if it isn't actually marked as unique. In those cases we don't want to post notifications. This is controlled by the m_enforceFilePathSeparation variable.

But the m_enforceFilePathSeparation variable itself is confusing. The only place it seems to be used is in the toString() method, which makes the origin appear unique, and in there, it is only used against the "file" protocol. But the only call site that changes that variable is in Document::initSecurityContext(), and there it checks isLocal(). Is that right?

The boolean expression in canShowNotifications() reflects this portion of initSecurityContext():

    if (Settings* settings = this->settings()) {
        if (!settings->isWebSecurityEnabled()) {
          securityOrigin()->grantUniversalAccess();
        } else if (settings->allowUniversalAccessFromFileURLs() && securityOrigin()->isLocal()) {
          securityOrigin()->grantUniversalAccess();
        } else if (!settings->allowFileAccessFromFileURLs() && securityOrigin()->isLocal()) {
          securityOrigin()->enforceFilePathSeparation();
        }
    }

So in checkPermission() and requestPermission(), by the time we reach the isLocal() check, we know that this local URL has been granted universal access, or does not appear like a unique origin.

Maybe an alternative phrase for "enforceFilePathSeparation" would clarify this?
Comment 16 Adam Barth 2012-03-19 15:02:11 PDT
Comment #15 is very confusing to me.  Maybe it would make more sense to discuss this in a more interactive setting?  (More detailed comments below.)

> > Why should local URLs be automatically whitelisted to have PermissionAllowed?  That sounds like a security decision that should either be made by SecurityOrigin or the client (probably the client).
> 
> By the point we do the isLocal() check, we will have gone through the canShowNotifications() check, which will only be true for local protocols if they have been granted universal access, and do not have the file url restriction.

My point is that whatever logic we have for whether a document can show a notification should be in canShowNotifications().  This could shouldn't care whether an origin is local or not.  This code should not have different behavior depend on whether anything is local.  In fact, I suspect the "is local" flag should be private to SecurityOrigin.

> >> Source/WebCore/page/SecurityOrigin.cpp:360
> >> +    return !isUnique() && (m_universalAccess || !m_enforceFilePathSeparation);
> > 
> > I'm not sure I understand this condition.  Is this the same thing we use for, e.g., localStorage?
> 
> No, it is not the same. It looks like there was a special exception added for local/file protocols, which caused the security origin for file URLs to appear unique, even if it isn't actually marked as unique. In those cases we don't want to post notifications. This is controlled by the m_enforceFilePathSeparation variable.

canShowNotifications() should not depend on m_enforceFilePathSeparation.  Ideally, m_enforceFilePathSeparation wouldn't exist and when embedders wanted to put file URLs in unique origins, they'd use real unique origins.  However, there's some reason I forget why we can do that (related to files stored persistently on disk somewhere), so we have this m_enforceFilePathSeparation hack.  I need to do another round of cleaning up this issue, but I haven't gotten around to it.

In any case, notification shouldn't get tangled up in this mess.

> The boolean expression in canShowNotifications() reflects this portion of initSecurityContext():

Notifications shouldn't care at all about how SecurityOrigins get initialized.  They should base their decision entirely on the final SecurityOrigin that was constructed.

> So in checkPermission() and requestPermission(), by the time we reach the isLocal() check, we know that this local URL has been granted universal access, or does not appear like a unique origin.

Right, but Notification should not care whether a SecurityOrigin is local.  Really, nothing in WebCore should care.

> Maybe an alternative phrase for "enforceFilePathSeparation" would clarify this?

Yeah, there's likely a better name for enforceFilePathSeparation that would clarify it's purpose as state that shouldn't be used for anything new.
Comment 17 Jon Lee 2012-03-20 11:16:39 PDT
Created attachment 132860 [details]
Patch
Comment 18 Adam Barth 2012-03-20 11:21:29 PDT
Comment on attachment 132860 [details]
Patch

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

> Source/WebKit2/WebProcess/Notifications/WebNotificationManager.cpp:99
> -    HashMap<String, bool>::const_iterator it = m_permissionsMap.find(origin->toString());
> +    HashMap<String, bool>::const_iterator it = m_permissionsMap.find(origin->toRawString());

Should we ASSERT that the origin isn't unique here?  This code seems like it would do the wrong thing for unique origins, but based on the other changes in the patch, I don't think that can happen.
Comment 19 Gustavo Noronha (kov) 2012-03-20 11:22:37 PDT
Comment on attachment 132860 [details]
Patch

Attachment 132860 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12002395
Comment 20 Jon Lee 2012-03-20 11:25:46 PDT
(In reply to comment #18)
> (From update of attachment 132860 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=132860&action=review
> 
> > Source/WebKit2/WebProcess/Notifications/WebNotificationManager.cpp:99
> > -    HashMap<String, bool>::const_iterator it = m_permissionsMap.find(origin->toString());
> > +    HashMap<String, bool>::const_iterator it = m_permissionsMap.find(origin->toRawString());
> 
> Should we ASSERT that the origin isn't unique here?  This code seems like it would do the wrong thing for unique origins, but based on the other changes in the patch, I don't think that can happen.

Good point. I'll add it. Thanks!
Comment 21 Early Warning System Bot 2012-03-20 13:10:11 PDT
Comment on attachment 132860 [details]
Patch

Attachment 132860 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12014020
Comment 22 WebKit Review Bot 2012-03-20 14:40:25 PDT
Comment on attachment 132860 [details]
Patch

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

New failing tests:
fast/dom/error-to-string-stack-overflow.html
fast/notifications/notifications-request-permission.html
fast/notifications/notifications-without-permission.html
fast/notifications/notifications-check-permission.html
Comment 23 Jon Lee 2012-03-20 15:02:19 PDT
Committed r111445: <http://trac.webkit.org/changeset/111445>
Comment 24 Csaba Osztrogonác 2012-03-20 15:33:57 PDT
(In reply to comment #23)
> Committed r111445: <http://trac.webkit.org/changeset/111445>

Reopen, because it broke the Qt build. Why did you land it when EWS was red?
Comment 25 Csaba Osztrogonác 2012-03-20 15:35:56 PDT
And GTK build too ... 

Are you going to fix it or roll it out now?
Comment 26 Csaba Osztrogonác 2012-03-20 15:54:47 PDT
Rollout landed in http://trac.webkit.org/changeset/111455.

Please don't land patches and then leave if EWS bot are red ...
I willingly help you to fix the build if you ask me _before_ landing.
But I don't have time to fix build fail cause your patch at night.
Comment 27 Jon Lee 2012-03-20 17:04:27 PDT
Created attachment 132933 [details]
Rename enums to try to fix GTK
Comment 28 Jon Lee 2012-03-20 17:37:32 PDT
Committed r111476: http://trac.webkit.org/changeset/111476
Comment 29 Eric Seidel (no email) 2012-03-27 13:16:30 PDT
Comment on attachment 132933 [details]
Rename enums to try to fix GTK

Cleared review? from attachment 132933 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).