Bug 190825 – [MediaStream] Use DeviceIdHashSaltStorage to generate device ID hash salt

• Component: WebRTC
• Status: RESOLVED FIXED

[Eric Carlson]

Use DeviceIdHashSaltStorage instead of host application to generate device ID hash salt

[Radar WebKit Bug Importer]

<rdar://problem/45486085>

[Eric Carlson]

Created attachment 352973 [details]
Patch

[EWS Watchlist]

Comment on attachment 352973 [details]
Patch

Attachment 352973 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9706026

New failing tests:
http/tests/media/media-stream/enumerate-devices-source-id-persistent.html
http/tests/media/media-stream/enumerate-devices-source-id.html

[EWS Watchlist]

Created attachment 352976 [details]
Archive of layout-test-results from ews105 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Eric Carlson]

Created attachment 353324 [details]
Patch

[youenn fablet]

Comment on attachment 353324 [details]
Patch

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

> Source/WebKit/UIProcess/API/C/WKUserMediaPermissionCheck.cpp:-45
> -    toImpl(userMediaPermissionRequestRef)->setUserMediaAccessInfo(toWTFString(mediaDeviceIdentifierHashSalt), allowed);

Should we add a FIXME to change the API to remove mediaDeviceIdentifierHashSalt.

> Source/WebKit/UIProcess/API/glib/WebKitDeviceInfoPermissionRequest.cpp:76
>      auto salt = priv->deviceIdHashSaltStorage->deviceIdHashSaltForOrigin(priv->request->topLevelDocumentSecurityOrigin());

Do we need this call to deviceIdHashSaltForOrigin?

> Source/WebKit/UIProcess/API/glib/WebKitDeviceInfoPermissionRequest.cpp:96
> +    auto salt = priv->deviceIdHashSaltStorage->regenerateDeviceIdHashSaltForOrigin(priv->request->topLevelDocumentSecurityOrigin());

s/auto salt//
I wonder whether regenerateDeviceIdHashSaltForOrigin is what we want or we should do deleteDeviceIdHashSaltForOrigin (maybe should be deleteDeviceIdHashSaltForOriginIfAny).

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:49
> +const String& DeviceIdHashSaltStorage::regenerateDeviceIdHashSaltForOrigin(SecurityOrigin& documentOrigin)

const SecurityOrigin&?

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:53
>          return keyAndValue.value->documentOrigin == documentOriginData;

Code below is similar to deleteDeviceIdHashSaltForOrigin, maybe we can share it.

> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:382
> +        return;

I am not sure we should introduce m_deviceIDHashSalt.
In case the page is navigated, we need to make sure we get it right for instance.
And we will probably no longer need it when not exposing device IDs in enumerateDevice if gum is not granted.

Instead, in the meantime, we might be able to do something like calling regenerateDeviceIdHashSaltForOrigin whenever gum prompt is called and succeeds but not persistent.
Then in UserMediaPermissionRequestManagerProxy::grantAccess, get the deviceIdHashSalt using m_page.websiteDataStore().deviceIdHashSaltStorage()->deviceIdHashSaltForOrigin(request.topLevelDocumentSecurityOrigin())

There is a temporary issue in case two web pages of the same domain do prompts one for audio, the second for audio or video (thus regenerating the hash) and then the first back for video (hence new hash salt), but maybe this is ok.
Maybe we should make independent the fact of setting the device ID hash salt from granting access (two different IPC messages?).

> LayoutTests/http/tests/media/media-stream/enumerate-devices-source-id.html:45
> +                    testPassed(`: device IDs are not unique`);

As is, the test is a bit frightening since it allows a third party iframe to get the IDs although they should not.
Maybe we should first disable enumerateDevices for third party iframes that are not allowed to do gum?
Or modify this test to already allow such iframes to call gum.

[Eric Carlson]

Created attachment 354203 [details]
Patch

[Alejandro G. Castro]

Comment on attachment 354203 [details]
Patch

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

LGTM, thanks for the changes in the GTK port! :-) Just minor nits, I'll let youenn do a final review.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.h:49
> +    void deleteDeviceIdHashSaltForOrigin(const WebCore::SecurityOriginData&);
> +

Is this function defined?

> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:359
> +        // std::optional

Probably a leftover.

> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.h:111
> +    String m_deviceIDHashSalt;

Is this needed?

[Eric Carlson]

Comment on attachment 354203 [details]
Patch

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

>> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.h:49
>> +
> 
> Is this function defined?

No, removed.

>> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:359
>> +        // std::optional
> 
> Probably a leftover.

Indeed, removed.

>> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.h:111
>> +    String m_deviceIDHashSalt;
> 
> Is this needed?

Also not needed, removed.

[Eric Carlson]

Created attachment 354234 [details]
Patch

[youenn fablet]

Comment on attachment 354234 [details]
Patch

r=me.
It seems that there might be a follow-up which will be to delete the salt in case some access is not granted anymore, right?
Are we missing anything for enabling persistent device ids after grant?
Should we add some tests doing grant gum/reload page/grant gum/check ids before and after reload, or grant gum/reload page/try gum with device ids from initial page load?



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

> Source/WebKit/UIProcess/API/glib/WebKitDeviceInfoPermissionRequest.cpp:-96
> -    auto salt = priv->deviceIdHashSaltStorage->regenerateDeviceIdHashSaltForOrigin(*priv->request);

Do we need regenerateDeviceIdHashSaltForOrigin anymore?

> Source/WebKit/UIProcess/UserMediaPermissionCheckProxy.cpp:47
>      if (!m_completionHandler)

I would add ASSERT(!m_completionHandler);
I would be tempted to remove the if statement, but this might be too much for now.

[Eric Carlson]

Comment on attachment 354234 [details]
Patch

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

>> Source/WebKit/UIProcess/API/glib/WebKitDeviceInfoPermissionRequest.cpp:-96
>> -    auto salt = priv->deviceIdHashSaltStorage->regenerateDeviceIdHashSaltForOrigin(*priv->request);
> 
> Do we need regenerateDeviceIdHashSaltForOrigin anymore?

Probably not. It isn't used now in any case, so I will remove it and we can always add it back later if necessary.

>> Source/WebKit/UIProcess/UserMediaPermissionCheckProxy.cpp:47
>>      if (!m_completionHandler)
> 
> I would add ASSERT(!m_completionHandler);
> I would be tempted to remove the if statement, but this might be too much for now.

I'll add an ASSERT.

[Eric Carlson]

(In reply to youenn fablet from comment #11)
> Comment on attachment 354234 [details]
> Patch
> 
> It seems that there might be a follow-up which will be to delete the salt in
> case some access is not granted anymore, right?
> 

I don't think so, I think regenerating the salt as a side effect of denying access would be surprising. The salt for a domain is deleted whenever cookies are deleted, or the UA can add custom UI to delete gUM settings.

> Are we missing anything for enabling persistent device ids after grant?
>

No, a salt is now always persistent (until deleted) but IDs are not revealed until after a page has been granted access to capture.

> Should we add some tests doing grant gum/reload page/grant gum/check ids
> before and after reload, or grant gum/reload page/try gum with device ids
> from initial page load?
> 

Yes, we do need more tests. I will add some in a followup.

[Eric Carlson]

Created attachment 354239 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 354239 [details]
Patch for landing

Clearing flags on attachment: 354239

Committed r237988: <https://trac.webkit.org/changeset/237988>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.