| Summary: | Prevent test functionality in AdAttributionDaemon when not running tests | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Alex Christensen <achristensen> | ||||||||||
| Component: | New Bugs | Assignee: | Alex Christensen <achristensen> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | ap, beidson, webkit-bug-importer | ||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||
| Version: | WebKit Nightly Build | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Alex Christensen
2021-10-05 15:02:54 PDT
Created attachment 440273 [details]
Patch
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. 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. 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). 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. 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.
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 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 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 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.
Created attachment 447535 [details]
Patch
Created attachment 447536 [details]
Patch
Created attachment 447637 [details]
Patch
|