Bug 79704 – Restrict access to notifications for unique origins and file URLs with no local file access

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jon Lee]

For now, this includes geolocation and notifications.

<rdar://problem/10912430>

[Jon Lee]

Created attachment 129123 [details]
Patch

[Jon Lee]

After some discussion, reconsidering approach to this issue. Renaming appropriately.

[Jon Lee]

Created attachment 129219 [details]
Patch

[Adam Barth]

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...

[Jon Lee]

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.

[Jon Lee]

(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.

[Jon Lee]

Created attachment 129325 [details]
Patch

[Alexey Proskuryakov]

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?

[Jon Lee]

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.

[Jon Lee]

Created attachment 129353 [details]
WIP

[WebKit Review Bot]

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

[Jon Lee]

Created attachment 129469 [details]
WIP #2

[Jon Lee]

Created attachment 132555 [details]
Patch

[Adam Barth]

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.

[Jon Lee]

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?

[Adam Barth]

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.

[Jon Lee]

Created attachment 132860 [details]
Patch

[Adam Barth]

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.

[Gustavo Noronha (kov)]

Comment on attachment 132860 [details]
Patch

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

[Jon Lee]

(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!

[Early Warning System Bot]

Comment on attachment 132860 [details]
Patch

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

[WebKit Review Bot]

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

[Jon Lee]

Committed r111445: <http://trac.webkit.org/changeset/111445>

[Csaba Osztrogonác]

(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?

[Csaba Osztrogonác]

And GTK build too ... 

Are you going to fix it or roll it out now?

[Csaba Osztrogonác]

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.

[Jon Lee]

Created attachment 132933 [details]
Rename enums to try to fix GTK

[Jon Lee]

Committed r111476: http://trac.webkit.org/changeset/111476

[Eric Seidel (no email)]

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).