1209843 - Throw in the towel on killing UNKNOWN_APP_ID

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect)

Description

[Bobby Holley (:bholley)]

UNKNOWN_APP_ID indicates that the caller created a principal without having a good idea of what the appId should be. The idea was that we'd eventually stamp it out by incrementally making more of the core machinery reject it.

When I added the OriginAttributes machinery, the possibility of encountering UNKNOWN_APP_ID seemed like an annoying reason to make everything fallible, so I tried to just release-assert against that case, figuring that we could fix any crash reports that came in.

In retrospect, this was clearly a mistake. UNKNOWN_APP_ID is too easy to generate from script (which addons seem to do copiously), and checking for it in all the API entry points is too error prone to get right. Bug 1209587, bug 1171432, bug 1205456, bug 1182610, and bug 1172542 are all testaments to the failure of this strategy.

Given that the new security model involves eliminating appId in the long run, this doesn't feel like the right battle to fight. In the modern world, we _really_ want to battle the consumers that create arbitrary OriginAttributes without inheriting them from the proper thing. We need a strategy for that, but this isn't it, so let's stop paying the price for it.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Stop checking for UNKNOWN_APP_ID in all places except those where AppId() is explicitly queried. v1

Comment 2

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3361722bd547

Comment 3

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/88072c76529a

Comment 4

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/88072c76529a

Comment 5

[Bobby Holley (:bholley)]

Comment on attachment 8667678 [details] [diff] [review]
Stop checking for UNKNOWN_APP_ID in all places except those where AppId() is explicitly queried. v1

Approval Request Comment
[Feature/regressing bug #]: bug 1165162
[User impact if declined]: A bunch of unnecessary release-mode crashes (See all the bugs about crashing in CreateSuffix).
[Describe test coverage new/current, TreeHerder]: None
[Risks and why]: Low risk - this is basically just removing a bunch of manual crashes.
[String/UUID change made/needed]: None

Comment 6

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8667678 [details] [diff] [review]
Stop checking for UNKNOWN_APP_ID in all places except those where AppId() is explicitly queried. v1

Let's take this on aurora and beta. Improving the crash rate sounds good.

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ceadb2f31200

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9f5a6eb87f21