Bug 233673 – PCM: Unlinkable tokens for triggering event to prevent fraud

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[John Wilander]

We recently introduced click fraud prevention with unlinkable tokens on the source side in Private Click Measurement (PCM). We should design and implement a way for click destinations to sign unlinkable tokens too and have those tokens be part of the resulting ad attribution report. This helps fight at least three types of fraud:

1. A fraudulent user induces a triggering event that the merchant doesn’t trust. The new feature would allow them to simply not sign a token for that event.
2. A fraudulent publisher creates an attribution report with a signed token from itself and submits it to the merchant to claim attribution.
3. A fraudulent merchant claims that a specific attribution report was fraudulent so that they don’t have to pay for attribution.

[John Wilander]

<rdar://79426347>

[John Wilander]

Created attachment 445507 [details]
Patch

[John Wilander]

Created attachment 445509 [details]
Patch

[John Wilander]

Created attachment 445521 [details]
Patch

[John Wilander]

The iOS layout test failure is just a remaining text diff change I need to make. I’ll do that as part of addressing code review comments.

[John Wilander]

The iOS API test failure is unrelated and being addressed similar to https://trac.webkit.org/changeset/286368/webkit.

[Alex Christensen]

Comment on attachment 445521 [details]
Patch

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

> Source/WebCore/loader/PrivateClickMeasurement.cpp:95
> +PrivateClickMeasurement::UnlinkableToken PrivateClickMeasurement::UnlinkableToken::isolatedCopy() const

nit: If you use trailing return type syntax, it reduces the number of "PrivateClickMeasurement::" needed.

> Source/WebCore/loader/PrivateClickMeasurement.cpp:107
> +PrivateClickMeasurement::SourceUnlinkableToken PrivateClickMeasurement::SourceUnlinkableToken::isolatedCopy() const

This is a lot of duplicate code.  Can we just have UnlinkableToken have an isolatedCopy() without having 3 copies of the exact same code?  Same with the secret tokens.

> Source/WebCore/loader/PrivateClickMeasurement.cpp:164
> +    for (auto& parameter : parameters) {
> +        if (parameter.key == "attributionSource") {

This seems to allow two attributionDestinationNonces or two attributionSources.  I think the intent of the two parameters is that there is one attributionSource and one attributionDestinationNonce.

> Source/WebCore/loader/PrivateClickMeasurement.cpp:183
> +    AttributionTriggerData attributionTriggerData;

I prefer the pattern of calling the constructor with all the members when returning it over calling the default constructor then filling in the members.  It makes it easier to remember all the members.

> Source/WebCore/loader/PrivateClickMeasurement.cpp:343
> +bool PrivateClickMeasurement::EphemeralDestinationNonce::isValid() const

Can we make EphemeralSourceNonce and EphemeralDestinationNonce share a parent class to have less duplicate code like we do with the tokens?

> Source/WebCore/loader/PrivateClickMeasurement.cpp:365
> +const std::optional<const URL> PrivateClickMeasurement::tokenPublicKeyURL(const RegistrableDomain& registrableDomain)

A URL already has a null state.  std::optional<URL> is almost always overkill.  Just check if the URL is non-null and valid.

> Source/WebCore/loader/PrivateClickMeasurement.cpp:413
> +        return reportDetails;

Do we want an empty object here instead of nullptr?

> Source/WebCore/loader/cocoa/PrivateClickMeasurementCocoa.mm:91
> +Expected<PrivateClickMeasurement::DestinationSecretToken, String> PrivateClickMeasurement::calculateAndUpdateDestinationSecretToken(const String& serverResponseBase64URL, DestinationUnlinkableToken& unlinkableToken)

Why is this signature so different from calculateAndUpdateSourceSecretToken?

> Source/WebKit/NetworkProcess/PrivateClickMeasurement/PrivateClickMeasurementDatabase.cpp:719
>  bool Database::needsUpdatedSchema()

I'm not sure if this needs updating.  Schema migration needs a test like MigrateFromPCM1

> Source/WebKit/NetworkProcess/PrivateClickMeasurement/PrivateClickMeasurementDatabase.h:104
> +    std::optional<WebCore::PrivateClickMeasurement::DestinationSecretToken> m_destinationSecretToken;

Why do we need to keep a destination secret token here but not a source secret token here?

> Source/WebKit/NetworkProcess/PrivateClickMeasurement/PrivateClickMeasurementManager.cpp:283
> +void PrivateClickMeasurementManager::getSignedUnlinkableTokenForDestination(SourceSite&& sourceSite, AttributionDestinationSite&& destinationSite, AttributionTriggerData&& attributionTriggerData, const ApplicationBundleIdentifier& applicationBundleIdentifier)

This shares a lot of code with PrivateClickMeasurementManager::getSignedUnlinkableToken.  Can they be better abstracted?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/EventAttribution.mm:349
> +TEST(PrivateClickMeasurement, DestinationClickFraudPrevention)

This has a lot of duplicate code with TEST(PrivateClickMeasurement, FraudPrevention).  Can they share anything?

[Kate Cheney]

Comment on attachment 445521 [details]
Patch

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

> Source/WebKit/NetworkProcess/DatabaseUtilities.cpp:163
> +    // These indices are zero-based for some reason. There should be a comment here explaining why.

This is explained in the SQLite docs: https://www.sqlite.org/c3ref/column_blob.html "The leftmost column of the result set has the index 0".

> Source/WebKit/NetworkProcess/PrivateClickMeasurement/PrivateClickMeasurementDatabase.cpp:-714
> -    checkColumns(unattributedTableName);

Removing this will cause an API test to fail, MigrateFromPCM1. We should replace it with a test for the new columns.

[John Wilander]

(In reply to Alex Christensen from comment #7)
> Comment on attachment 445521 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=445521&action=review
> 
> > Source/WebCore/loader/PrivateClickMeasurement.cpp:95
> > +PrivateClickMeasurement::UnlinkableToken PrivateClickMeasurement::UnlinkableToken::isolatedCopy() const
> 
> nit: If you use trailing return type syntax, it reduces the number of
> "PrivateClickMeasurement::" needed.
> 
> > Source/WebCore/loader/PrivateClickMeasurement.cpp:107
> > +PrivateClickMeasurement::SourceUnlinkableToken PrivateClickMeasurement::SourceUnlinkableToken::isolatedCopy() const
> 
> This is a lot of duplicate code.  Can we just have UnlinkableToken have an
> isolatedCopy() without having 3 copies of the exact same code?  Same with
> the secret tokens.

That's what I did first but the compiler did not accept my cast to the specific type. Any idea how it should be done? I specifically want to have distinct types to minimize the risk of tokens getting mixed up in the code.

> > Source/WebCore/loader/PrivateClickMeasurement.cpp:164
> > +    for (auto& parameter : parameters) {
> > +        if (parameter.key == "attributionSource") {
> 
> This seems to allow two attributionDestinationNonces or two
> attributionSources.  I think the intent of the two parameters is that there
> is one attributionSource and one attributionDestinationNonce.

True.

> > Source/WebCore/loader/PrivateClickMeasurement.cpp:183
> > +    AttributionTriggerData attributionTriggerData;
> 
> I prefer the pattern of calling the constructor with all the members when
> returning it over calling the default constructor then filling in the
> members.  It makes it easier to remember all the members.

Some of them are optional which is why it was done this way. Are you saying you want them to have default values in the constructor?

> > Source/WebCore/loader/PrivateClickMeasurement.cpp:343
> > +bool PrivateClickMeasurement::EphemeralDestinationNonce::isValid() const
> 
> Can we make EphemeralSourceNonce and EphemeralDestinationNonce share a
> parent class to have less duplicate code like we do with the tokens?

I actually don't think they need to be distinct since they are not stored or included in attribution reports. I'll try to change that.

> > Source/WebCore/loader/PrivateClickMeasurement.cpp:365
> > +const std::optional<const URL> PrivateClickMeasurement::tokenPublicKeyURL(const RegistrableDomain& registrableDomain)
> 
> A URL already has a null state.  std::optional<URL> is almost always
> overkill.  Just check if the URL is non-null and valid.

The problem here is that checking if the URL is non-empty has to be done by the caller and it's easy to forget. We already had that problem in how we create the token URLs which is why I made them optional. When they are optional, the caller *has to check*. Mandatory check is what I want. Another way to achieve that is by having an optional error/exception. Is that preferable?

> > Source/WebCore/loader/PrivateClickMeasurement.cpp:413
> > +        return reportDetails;
> 
> Do we want an empty object here instead of nullptr?

The function returns a Ref<JSON::Object>. Can that be null?

> > Source/WebCore/loader/cocoa/PrivateClickMeasurementCocoa.mm:91
> > +Expected<PrivateClickMeasurement::DestinationSecretToken, String> PrivateClickMeasurement::calculateAndUpdateDestinationSecretToken(const String& serverResponseBase64URL, DestinationUnlinkableToken& unlinkableToken)
> 
> Why is this signature so different from calculateAndUpdateSourceSecretToken?

It's static since the DestinationUnlinkableToken is not a member. I at one point made them both static but removed that to make fewer changes.

> > Source/WebKit/NetworkProcess/PrivateClickMeasurement/PrivateClickMeasurementDatabase.cpp:719
> >  bool Database::needsUpdatedSchema()
> 
> I'm not sure if this needs updating.  Schema migration needs a test like
> MigrateFromPCM1

Is this what Kate referred to in her subsequent comment?

> > Source/WebKit/NetworkProcess/PrivateClickMeasurement/PrivateClickMeasurementDatabase.h:104
> > +    std::optional<WebCore::PrivateClickMeasurement::DestinationSecretToken> m_destinationSecretToken;
> 
> Why do we need to keep a destination secret token here but not a source
> secret token here?

Sorry, this is a leftover from before I had implemented the DB part and wanted to build a full roundtrip test case. I kept the destination token in memory to build the test case and then continued to implement the DB part. Will remove.

> > Source/WebKit/NetworkProcess/PrivateClickMeasurement/PrivateClickMeasurementManager.cpp:283
> > +void PrivateClickMeasurementManager::getSignedUnlinkableTokenForDestination(SourceSite&& sourceSite, AttributionDestinationSite&& destinationSite, AttributionTriggerData&& attributionTriggerData, const ApplicationBundleIdentifier& applicationBundleIdentifier)
> 
> This shares a lot of code with
> PrivateClickMeasurementManager::getSignedUnlinkableToken.  Can they be
> better abstracted?

Agreed. I started out trying to combine them but the flow is quite different. The first one gets the signature before a PCM object is persisted. The second gets the signature before trying to match with an already stored PCM object. I'll see if I can extract some of the code into a helper function.

> > Tools/TestWebKitAPI/Tests/WebKitCocoa/EventAttribution.mm:349
> > +TEST(PrivateClickMeasurement, DestinationClickFraudPrevention)
> 
> This has a lot of duplicate code with TEST(PrivateClickMeasurement,
> FraudPrevention).  Can they share anything?

I'll try.

Thanks!

[John Wilander]

(In reply to Kate Cheney from comment #8)
> Comment on attachment 445521 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=445521&action=review
> 
> > Source/WebKit/NetworkProcess/DatabaseUtilities.cpp:163
> > +    // These indices are zero-based for some reason. There should be a comment here explaining why.
> 
> This is explained in the SQLite docs:
> https://www.sqlite.org/c3ref/column_blob.html "The leftmost column of the
> result set has the index 0".

Thanks! I'll add that link in a comment so that others won't get as confused as I was.

> > Source/WebKit/NetworkProcess/PrivateClickMeasurement/PrivateClickMeasurementDatabase.cpp:-714
> > -    checkColumns(unattributedTableName);
> 
> Removing this will cause an API test to fail, MigrateFromPCM1. We should
> replace it with a test for the new columns.

I got no API test failures though, neither locally nor on the bots. Any clues as to why?

Thanks!

[John Wilander]

Created attachment 445793 [details]
Patch

[John Wilander]

Changes from last patch:
- Extracted a bunch of shared code to reduce duplication, both in implementation and test code.
- Made sure duplicate query parameters are not allowed.
- Collapsed source and destination nonces into once class.
- Removed stray member variable.
- Added comment and link on zero-based indices.

Remaining things to discuss:

# Constructor with default values vs filling in members

PrivateClickMeasurement::parseAttributionRequestQuery() returns a PrivateClickMeasurement::AttributionTriggerData object for further population which is why it's done that way. The alternative I considered was to create the PrivateClickMeasurement::AttributionTriggerData object and submit it in the call to PrivateClickMeasurement::parseAttributionRequestQuery() but that still means filling in members after construction.

# Use of std::optional<URL>

I want to force the caller to have to check instead of relying on the caller to check if the returned URL is empty or not. This is because the signature URL stuff doesn't react on empty URLs and I don't want to risk us producing tokens for invalid URLs.

# MigrateFromPCM1

You both commented on this but the test passes with my changes. There's a comment in the code that we should remove the migration code late 2021. I looked at how the test is set up and wasn't able to figure out if it's supposed to fail with my changes or not. Kate suggested I repurpose this test for the new columns. Guidance would be appreciated. :)

[Alex Christensen]

Comment on attachment 445793 [details]
Patch

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

My comment about MigrateFromPCM1 was that we need a MigrateFromPCM2 to test what happens when we open a database written before this schema change.

> Source/WebCore/loader/PrivateClickMeasurement.cpp:75
> +    copy.tokenBase64URL = tokenBase64URL.isolatedCopy();

This is all duplicate code.  It should just call SecretToken::isolatedCopy.

> Source/WebCore/loader/PrivateClickMeasurement.cpp:84
> +    copy.tokenBase64URL = tokenBase64URL.isolatedCopy();

ditto.  Actually, I think if you just completely remove these 4 functions it will do the right thing.

> Source/WebCore/loader/PrivateClickMeasurement.cpp:111
> +    copy.blinder = blinder;

ditto with UnlinkableToken::isolatedCopy

> Source/WebCore/loader/PrivateClickMeasurement.cpp:123
> +    copy.blinder = blinder;

ditto

[John Wilander]

(In reply to Alex Christensen from comment #13)
> Comment on attachment 445793 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=445793&action=review
> 
> My comment about MigrateFromPCM1 was that we need a MigrateFromPCM2 to test
> what happens when we open a database written before this schema change.
> 
> > Source/WebCore/loader/PrivateClickMeasurement.cpp:75
> > +    copy.tokenBase64URL = tokenBase64URL.isolatedCopy();
> 
> This is all duplicate code.  It should just call SecretToken::isolatedCopy.
> 
> > Source/WebCore/loader/PrivateClickMeasurement.cpp:84
> > +    copy.tokenBase64URL = tokenBase64URL.isolatedCopy();
> 
> ditto.  Actually, I think if you just completely remove these 4 functions it
> will do the right thing.
> 
> > Source/WebCore/loader/PrivateClickMeasurement.cpp:111
> > +    copy.blinder = blinder;
> 
> ditto with UnlinkableToken::isolatedCopy
> 
> > Source/WebCore/loader/PrivateClickMeasurement.cpp:123
> > +    copy.blinder = blinder;
> 
> ditto

I did that originally and ran into problems casting to the specific type that I want the respective isolatedCopy() to return. DestinationSecretToken::isolatedCopy() should return a DestinationSecretToken and vice versa. I may have done it wrong.

return static_cast<DestinationSecretToken>(SecretToken::isolatedCopy());

… results in …

error: no matching conversion for static_cast from 'PrivateClickMeasurement::SecretToken' to 'WebCore::PrivateClickMeasurement::SourceSecretToken'

[Alex Christensen]

Don't static_cast, call the constructor.

[John Wilander]

Created attachment 445902 [details]
Patch

[John Wilander]

(In reply to Alex Christensen from comment #15)
> Don't static_cast, call the constructor.

That worked! Thanks

[John Wilander]

Comment on attachment 445902 [details]
Patch

Thank you, Alex!

[EWS]

Committed r286519 (244854@main): <https://commits.webkit.org/244854@main>

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