Bug 231258 – Prevent test functionality in AdAttributionDaemon when not running tests

• Component: New Bugs
• Status: RESOLVED FIXED

[Alex Christensen]

Prevent test functionality in AdAttributionDaemon when not running tests

[Alex Christensen]

Created attachment 440273 [details]
Patch

[Alexey Proskuryakov]

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.

[Alex Christensen]

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.

[Alexey Proskuryakov]

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).

[Alex Christensen]

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.

[Alexey Proskuryakov]

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.

[Alex Christensen]

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.

[Alexey Proskuryakov]

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.

[Alex Christensen]

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.

[Alexey Proskuryakov]

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.

[Radar WebKit Bug Importer]

<rdar://problem/84168088>

[Alex Christensen]

Created attachment 447535 [details]
Patch

[Alex Christensen]

Created attachment 447536 [details]
Patch

[Alex Christensen]

Created attachment 447637 [details]
Patch

[Alex Christensen]

http://trac.webkit.org/r287276