Bug 231258 - Prevent test functionality in AdAttributionDaemon when not running tests
Summary: Prevent test functionality in AdAttributionDaemon when not running tests
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-10-05 15:02 PDT by Alex Christensen
Modified: 2021-12-20 14:04 PST (History)
3 users (show)

See Also:


Attachments
Patch (5.90 KB, patch)
2021-10-05 15:07 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (11.22 KB, patch)
2021-12-18 16:47 PST, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (12.45 KB, patch)
2021-12-18 17:04 PST, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (12.50 KB, patch)
2021-12-20 14:03 PST, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2021-10-05 15:02:54 PDT
Prevent test functionality in AdAttributionDaemon when not running tests
Comment 1 Alex Christensen 2021-10-05 15:07:05 PDT
Created attachment 440273 [details]
Patch
Comment 2 Alexey Proskuryakov 2021-10-05 15:43:14 PDT
Comment on attachment 440273 [details]
Patch

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

> Source/WebKit/ChangeLog:10
> +        AdAttributionDaemon receives many messages, some of which do things that we want to test but want the normal daemon
> +        to be unable to do, such as setting the report endpoint or bundle identifier.  When we receive such a message
> +        in the non-test daemon, ignore it.

How bad is it for the non-test daemon to handle these messages? One can launch the OS supplied binary and provide an arbitrary service name to it.
Comment 3 Alex Christensen 2021-10-05 17:06:13 PDT
This prevents a fraudulent app from using SPI to do things that it shouldn't.  The daemon will have access to a shared storage location that apps do not, so this is one layer of defense to prevent a rogue app from corrupting the data or state of the daemon that is storing data in the shared location.
Comment 4 Alexey Proskuryakov 2021-10-05 17:23:32 PDT
I see. I don't think that the defense works then, per the above (the rogue app can just confuse AdAttributionDaemon by launching its own copy).
Comment 5 Alex Christensen 2021-10-05 17:28:24 PDT
This is intended to be a protection on iOS, where a mach service named "com.apple.webkit.adattributiond.service" will already be registered with launchd.  A rogue app can't register a new mach service with that same name.  WebKit network processes will connect to a mach service with that name.  So even if an app does manage to launch an instance of AdAttributionDaemon, network processes won't connect to it.

Also, if an app does manage to start its own copy of AdAttributionDaemon, then that copy will not have access to the shared storage location, which an app will not have direct access to.
Comment 6 Alexey Proskuryakov 2021-10-05 17:31:57 PDT
The concern is not about having a second com.apple.webkit.adattributiond.service, it is about launching the system binary, and telling it that it's org.webkit.pcmtestdaemon.service - and then it won't be Networking process talking to it, but the rogue app.

> Also, if an app does manage to start its own copy of AdAttributionDaemon, then that copy will not have access to the shared storage location

Is this really and robustly true? I'm intentionally not saying how the launching will happen, because that's quite a few scenarios to verify safety of.
Comment 7 Alex Christensen 2021-10-05 20:40:45 PDT
I think the focus of this review is off.

Before this patch, any app can use SPI to send a SetAttributionReportURLsForTesting message to the real AdAttributionDaemon, which would then cause that URL to intercept all attribution reports for that device.  This patch is to prevent such a thing from happening.

There may be other attacks that can still happen.  There is probably more hardening that should be done after this.  I am simply requesting review for this particular hardening.
Comment 8 Alexey Proskuryakov 2021-10-05 22:11:17 PDT
Comment on attachment 440273 [details]
Patch

I don't see a reason to land code that incompletely fixes the problem unless there is a plan for how to fix it fully that depends on it.
Comment 9 Alex Christensen 2021-10-05 22:42:20 PDT
Comment on attachment 440273 [details]
Patch

This does fix this problem completely.  This problem is that by calling SPI a rogue app can get the AdAttributionDaemon to do things that it should not.  What you are describing is a different problem that I don't think exists, but if it does then it needs a different mitigation.  Re-requesting review.  If you are still opposed to this reducing of attack surface area, then we should probably discuss it in a meeting next week.
Comment 10 Alexey Proskuryakov 2021-10-06 08:54:53 PDT
Comment on attachment 440273 [details]
Patch

Launching a privileged process in a way that confuses it is a super common attack technique, so I don't think that patch meaningfully improves anything.
Comment 11 Radar WebKit Bug Importer 2021-10-12 15:03:17 PDT
<rdar://problem/84168088>
Comment 12 Alex Christensen 2021-12-18 16:47:14 PST
Created attachment 447535 [details]
Patch
Comment 13 Alex Christensen 2021-12-18 17:04:09 PST
Created attachment 447536 [details]
Patch
Comment 14 Alex Christensen 2021-12-20 14:03:00 PST
Created attachment 447637 [details]
Patch
Comment 15 Alex Christensen 2021-12-20 14:04:48 PST
http://trac.webkit.org/r287276