Bug 232770

Summary: trustd network connections occasionally omitted from App Privacy report
Product: WebKit Reporter: Kate Cheney <katherine_cheney>
Component: WebKit Misc.Assignee: Kate Cheney <katherine_cheney>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, benjamin, bfulgham, cdumez, cmarcelo, ddkilzer, ews-watchlist
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch for landing none

Description Kate Cheney 2021-11-05 15:24:00 PDT
trustd network connections occasionally omitted from App Privacy report
Comment 1 Kate Cheney 2021-11-05 15:24:12 PDT
rdar://83840427
Comment 2 Kate Cheney 2021-11-05 15:27:20 PDT
Created attachment 443442 [details]
Patch
Comment 3 Alex Christensen 2021-11-05 17:14:57 PDT
Comment on attachment 443442 [details]
Patch

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

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:706
> +        NSData *token = [NSData dataWithBytes:(uint8_t *)&tokenValue length:sizeof(tokenValue)];

nit: adoptNS([[NSData alloc] initWithBytes...

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:707
> +        SecTrustSetClientAuditToken(challenge.nsURLAuthenticationChallenge().protectionSpace.serverTrust, (__bridge CFDataRef)token);

SecuritySPI.h
Comment 4 Kate Cheney 2021-11-07 08:11:23 PST
Created attachment 443510 [details]
Patch
Comment 5 Brent Fulgham 2021-11-07 08:42:55 PST
Comment on attachment 443510 [details]
Patch

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

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:704
> +    if (auto auditToken = networkProcess().sourceApplicationAuditToken()) {

It seems like all of this work is only needed if APP_PRIVACY_REPORT IS ENABLED, SO I’d widen the scope so you don’t create objects you don’t need for other build targets. You could use UNUSED_PARAM for the non-APR case.

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:706
> +        RetainPtr<NSData> token = adoptNS([NSData dataWithBytes:(uint8_t *)&tokenValue length:sizeof(tokenValue)]);

I think dataWithBytes returns an auto released value, so this adopt may cause an over release. Maybe check with Kilzer.
Comment 6 Brent Fulgham 2021-11-07 08:48:12 PST
Comment on attachment 443510 [details]
Patch

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

>> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:706
>> +        RetainPtr<NSData> token = adoptNS([NSData dataWithBytes:(uint8_t *)&tokenValue length:sizeof(tokenValue)]);
> 
> I think dataWithBytes returns an auto released value, so this adopt may cause an over release. Maybe check with Kilzer.

I also wonder if we could use dataWithBytesNoCopy to avoid copying the token (but perhaps the token is small enough to make the allocation a trivial cost)
Comment 7 David Kilzer (:ddkilzer) 2021-11-07 10:03:47 PST
Comment on attachment 443510 [details]
Patch

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

>> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:704
>> +    if (auto auditToken = networkProcess().sourceApplicationAuditToken()) {
> 
> It seems like all of this work is only needed if APP_PRIVACY_REPORT IS ENABLED, SO I’d widen the scope so you don’t create objects you don’t need for other build targets. You could use UNUSED_PARAM for the non-APR case.

+1

>>> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:706
>>> +        RetainPtr<NSData> token = adoptNS([NSData dataWithBytes:(uint8_t *)&tokenValue length:sizeof(tokenValue)]);
>> 
>> I think dataWithBytes returns an auto released value, so this adopt may cause an over release. Maybe check with Kilzer.
> 
> I also wonder if we could use dataWithBytesNoCopy to avoid copying the token (but perhaps the token is small enough to make the allocation a trivial cost)

Do not use adoptNS() here because as Brent noted it causes an over-release.

Use adootNS([[NSData alloc] initWith…:]) to avoid creating an autoreleased object and to use adoptNS() correctly.

Using a “NoCopy” variant means that the lifetime of the backing buffer must be the same as or longer than the NS object.  Since the NSData object “escapes” via call to  SecTrustSetClientAuditToken(), I would not recommend doing that.

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:708
> +        SecTrustSetClientAuditToken(challenge.nsURLAuthenticationChallenge().protectionSpace.serverTrust, (__bridge CFDataRef)token.get());

You may use `bridge_cast(token.get())` here to magically do the cast to CFDataRef from NSData.

Just #import <wtf/cocoa/TypeCastsCocoa.h> above.
Comment 8 Kate Cheney 2021-11-07 10:14:04 PST
(In reply to David Kilzer (:ddkilzer) from comment #7)
> Comment on attachment 443510 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=443510&action=review
> 
> >> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:704
> >> +    if (auto auditToken = networkProcess().sourceApplicationAuditToken()) {
> > 
> > It seems like all of this work is only needed if APP_PRIVACY_REPORT IS ENABLED, SO I’d widen the scope so you don’t create objects you don’t need for other build targets. You could use UNUSED_PARAM for the non-APR case.
> 
> +1
> 

Will fix.

> >>> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:706
> >>> +        RetainPtr<NSData> token = adoptNS([NSData dataWithBytes:(uint8_t *)&tokenValue length:sizeof(tokenValue)]);
> >> 
> >> I think dataWithBytes returns an auto released value, so this adopt may cause an over release. Maybe check with Kilzer.
> > 
> > I also wonder if we could use dataWithBytesNoCopy to avoid copying the token (but perhaps the token is small enough to make the allocation a trivial cost)
> 
> Do not use adoptNS() here because as Brent noted it causes an over-release.
> 
> Use adootNS([[NSData alloc] initWith…:]) to avoid creating an autoreleased
> object and to use adoptNS() correctly.
> 

Ah, I misread Alex's comment above and didn't add the [NSData alloc] part, only the adoptNS, I will fix.

> Using a “NoCopy” variant means that the lifetime of the backing buffer must
> be the same as or longer than the NS object.  Since the NSData object
> “escapes” via call to  SecTrustSetClientAuditToken(), I would not recommend
> doing that.
> 
> > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:708
> > +        SecTrustSetClientAuditToken(challenge.nsURLAuthenticationChallenge().protectionSpace.serverTrust, (__bridge CFDataRef)token.get());
> 
> You may use `bridge_cast(token.get())` here to magically do the cast to
> CFDataRef from NSData.
> 
> Just #import <wtf/cocoa/TypeCastsCocoa.h> above.

Nice! Will change.
Comment 9 Kate Cheney 2021-11-07 10:14:58 PST
API test failures also look related :(
Comment 10 Kate Cheney 2021-11-07 10:29:16 PST
Created attachment 443513 [details]
Patch
Comment 11 Alex Christensen 2021-11-08 07:12:21 PST
Comment on attachment 443513 [details]
Patch

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

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:707
> +        auto tokenValue = *auditToken;

nit: I think auto& would save an unnecessary copy of a few bytes.
Comment 12 Kate Cheney 2021-11-08 07:45:55 PST
Created attachment 443545 [details]
Patch for landing
Comment 13 Kate Cheney 2021-11-08 07:46:24 PST
(In reply to Alex Christensen from comment #11)
> Comment on attachment 443513 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=443513&action=review
> 
> > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:707
> > +        auto tokenValue = *auditToken;
> 
> nit: I think auto& would save an unnecessary copy of a few bytes.

Thanks, fixed in PFL.

Thanks Dave and Brent also for the review comments!
Comment 14 EWS 2021-11-08 08:39:28 PST
Committed r285404 (243961@main): <https://commits.webkit.org/243961@main>

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