Use DeviceIdHashSaltStorage instead of host application to generate device ID hash salt
<rdar://problem/45486085>
Created attachment 352973 [details] Patch
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
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
Created attachment 353324 [details] Patch
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.
Created attachment 354203 [details] Patch
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?
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.
Created attachment 354234 [details] Patch
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.
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.
(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.
Created attachment 354239 [details] Patch for landing
Comment on attachment 354239 [details] Patch for landing Clearing flags on attachment: 354239 Committed r237988: <https://trac.webkit.org/changeset/237988>
All reviewed patches have been landed. Closing bug.