Bug 223956

Summary: PCM: PrivateClickMeasurementManager::getTokenPublicKey() should not use PrivateClickMeasurement::PcmDataCarried::PersonallyIdentifiable when validating the token before the attribution report is sent
Product: WebKit Reporter: John Wilander <wilander>
Component: WebKit Misc.Assignee: John Wilander <wilander>
Status: RESOLVED FIXED    
Severity: Normal CC: jiewen_tan, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=222217
https://bugs.webkit.org/show_bug.cgi?id=223957
Bug Depends on:    
Bug Blocks: 221291    
Attachments:
Description Flags
Patch
none
Patch none

Description John Wilander 2021-03-30 14:30:36 PDT
PrivateClickMeasurementManager::getTokenPublicKey() is currently configured to use PrivateClickMeasurement::PcmDataCarried::PersonallyIdentifiable when minting the unlinkable and secret tokens. However, PrivateClickMeasurementManager::getTokenPublicKey() is used a second time when validating the secret token before the attribution report is sent. On the second occasion, PrivateClickMeasurement::PcmDataCarried::NonPersonallyIdentifiable should be used. Which to use should probably be controlled by the caller.
Comment 1 Radar WebKit Bug Importer 2021-03-31 21:13:54 PDT
<rdar://problem/76086936>
Comment 2 John Wilander 2021-03-31 22:38:02 PDT
Created attachment 424872 [details]
Patch
Comment 3 John Wilander 2021-03-31 22:38:43 PDT
This patch also fixes https://bugs.webkit.org/show_bug.cgi?id=223957.
Comment 4 Jiewen Tan 2021-04-01 00:57:30 PDT
Comment on attachment 424872 [details]
Patch

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

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:143
> +    auto pcmDataCarried = UNLIKELY(debugModeEnabled()) ? PrivateClickMeasurement::PcmDataCarried::PersonallyIdentifiable : dataCarried;

Can we do this within generateNetworkLoadParameters? such that we can handle this logic for all traffics?
Comment 5 youenn fablet 2021-04-01 02:43:31 PDT
Comment on attachment 424872 [details]
Patch

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

>> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:143
>> +    auto pcmDataCarried = UNLIKELY(debugModeEnabled()) ? PrivateClickMeasurement::PcmDataCarried::PersonallyIdentifiable : dataCarried;
> 
> Can we do this within generateNetworkLoadParameters? such that we can handle this logic for all traffics?

Looking in the code, I do not see where is used pcmDataCarried?
Can you detail its use?
Would it make sense to pass pcmDataCarried directly to m_networkLoadFunction and remove it from NetworkLoadParameters.
I do not see it set in WebProcess for instance.
Comment 6 John Wilander 2021-04-01 09:44:20 PDT
(In reply to youenn fablet from comment #5)
> Comment on attachment 424872 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=424872&action=review
> 
> >> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:143
> >> +    auto pcmDataCarried = UNLIKELY(debugModeEnabled()) ? PrivateClickMeasurement::PcmDataCarried::PersonallyIdentifiable : dataCarried;
> > 
> > Can we do this within generateNetworkLoadParameters? such that we can handle this logic for all traffics?
> 
> Looking in the code, I do not see where is used pcmDataCarried?
> Can you detail its use?
> Would it make sense to pass pcmDataCarried directly to m_networkLoadFunction
> and remove it from NetworkLoadParameters.
> I do not see it set in WebProcess for instance.

It's is not in use now and intended for future things. Since PCM makes network requests that are not tied to any page load, we wanted to provide some guidance for the network loader on the privacy guarantees of the requests. That will allow WebKit clients with control over the network stack to make decisions on how to handle those requests. I will also mention this in the update to the spec once we have the whole fraud prevention thing settled.
Comment 7 John Wilander 2021-04-01 11:00:24 PDT
(In reply to Jiewen Tan from comment #4)
> Comment on attachment 424872 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=424872&action=review
> 
> > Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:143
> > +    auto pcmDataCarried = UNLIKELY(debugModeEnabled()) ? PrivateClickMeasurement::PcmDataCarried::PersonallyIdentifiable : dataCarried;
> 
> Can we do this within generateNetworkLoadParameters? such that we can handle
> this logic for all traffics?

Nice idea. Let me see if that's possible.
Comment 8 John Wilander 2021-04-01 14:50:22 PDT
(In reply to John Wilander from comment #7)
> (In reply to Jiewen Tan from comment #4)
> > Comment on attachment 424872 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=424872&action=review
> > 
> > > Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:143
> > > +    auto pcmDataCarried = UNLIKELY(debugModeEnabled()) ? PrivateClickMeasurement::PcmDataCarried::PersonallyIdentifiable : dataCarried;
> > 
> > Can we do this within generateNetworkLoadParameters? such that we can handle
> > this logic for all traffics?
> 
> Nice idea. Let me see if that's possible.

generateNetworkLoadParameters() is static so we have to send the flag in as a parameter. New patch coming up.
Comment 9 John Wilander 2021-04-01 14:58:11 PDT
Created attachment 424952 [details]
Patch
Comment 10 John Wilander 2021-04-01 17:53:56 PDT
Failing layout tests are unrelated.
Comment 11 John Wilander 2021-04-02 06:52:21 PDT
Comment on attachment 424952 [details]
Patch

Thanks, Youenn and Jiewen!
Comment 12 EWS 2021-04-02 07:01:12 PDT
Committed r275419: <https://commits.webkit.org/r275419>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 424952 [details].