1259871 - BackgroundUtils ignores origin attributes when app ID is unknown.

Status: RESOLVED FIXED

(Core :: IPC, defect, P1)

Description

[Dave Huseby [:huseby]]

in ipc/glue/BackgroundUtils.cpp when converting a PrincipalInfo into a Principal, if the PrincipalInfo has an unknown app ID, the rest of the origin attributes are ignored.  This seems wrong.  Here's the offending code:

> 79       if (info.attrs().mAppId == nsIScriptSecurityManager::UNKNOWN_APP_ID) {
> 80         rv = secMan->GetSimpleCodebasePrincipal(uri, getter_AddRefs(principal));
> 81       } else {
> 82         principal = BasePrincipal::CreateCodebasePrincipal(uri, info.attrs());
> 83         rv = principal ? NS_OK : NS_ERROR_FAILURE;
> 84       }

Comment 1

[Dave Huseby [:huseby]]

Jonas,

Do you know why the code ignores all of the other origin attributes (info.attrs()) if the app ID is UNKNOWN_APP_ID?

This doesn't seem correct to me.

--dave

Comment 2

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Dave Huseby [:huseby] from comment #1)
> Jonas,
> 
> Do you know why the code ignores all of the other origin attributes
> (info.attrs()) if the app ID is UNKNOWN_APP_ID?
> 
I can answer some of that.
Generally speaking UNKNOWN_APP_ID is used when we don't have enough information to know the attributes yet, and the principal we create won't be used to access any data-jars or permissions related code.

That's also why you can see lots of MOZ_ASSERT(appId == UNKNOW_APP_ID); in data-jar/permissions related codebase to gurantee this.

In the container project it might have some problems, as some codebase will have userContextId information where it also has UNKNOW_APP_ID, that's your bug 1234390.

I have also filed Bug 1259341 last week to track this.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yoshi is right.

I think UNKNOWN_APP_ID was more important when we were originally rolling out cookie jars since it helped us find pieces of code that hadn't been improved to set the correct cookie jar yet. This was especially important when it was a FirefoxOS-only feature since we couldn't expect the platform team to pay attention to the feature all the time.

I think at this point we can just replace UNKNOWN_APP_ID with the default appid, i.e. with NO_APP_ID. Though ideally appids will go away entirely soon as the FirefoxOS code is moving away from them.

Comment 5

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Hi Honza
As Jonas mentioned in Comment 3, we should replace UNKNOWN_APP_ID with NO_APP_ID, do you also agree we should do this in Necko side?

I found nsILoadContextInfo.idl also has this and it's done by you in Bug 913807.

Thanks

Comment 6

[Honza Bambas (:mayhemer)]

(In reply to Yoshi Huang[:allstars.chh] from comment #5)
> Hi Honza
> As Jonas mentioned in Comment 3, we should replace UNKNOWN_APP_ID with
> NO_APP_ID, do you also agree we should do this in Necko side?
> 
> I found nsILoadContextInfo.idl also has this and it's done by you in Bug
> 913807.
> 
> Thanks

nsILoadContextInfo::UNKNOWN_APP_ID seems to be unused.  Feel free to remove it.

Comment 7

[Jonathan Hao (inactive) [:jhao]]

So, I'm going to replace all UNKNOWN_APP_ID and NECKO_UNKNOWN_APP_ID (which are UINT_MAX) to NO_APP_ID (which is 0) and remove every check and assertion related to UNKNOWN_APP_ID and NECKO_UNKNOWN_APP_ID.

Hi S.C.,
I hope TV will not be affected by this change. I need your confirmation. Thanks.

Comment 8

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

No TV-specific feature is bounded to unknown app ID AFAIK. I'll suggest you to land your patch after TV 2.6 branch out from master (which will happened on 4/28), so that you won't hit by TV regression.

Comment 9

[Jonathan Hao (inactive) [:jhao]]

Attachment: Replace UNKNOWN_APP_ID with NO_APP_ID (WIP)

I'm not really sure about some of the cases.

if (appId == UNKNOWN_APP_ID) {
    ...
}

I could just replace UNKNOW_APP_ID with NO_APP_ID, but sometimes I might have to delete the whole if-statement.

Comment 10

[Jonathan Hao (inactive) [:jhao]]

Yoshi suggests that we wait until TV 2.6 branch out. B2G related code may be removed from central after that, and we can do this replacement after the removal.

Comment 11

[Jonathan Hao (inactive) [:jhao]]

Hi Jonas,

I'm considering opening another bug to remove UNKNOWN_APP_ID, since I'm not sure when will b2g code be removed.

As for what this bug was about, do you think we need to fix anything about it? When info.attrs() has an UNKNOWN_APP_ID, do we really care about its user context ID?

> 79       if (info.attrs().mAppId == nsIScriptSecurityManager::UNKNOWN_APP_ID) {
> 80         rv = secMan->GetSimpleCodebasePrincipal(uri, getter_AddRefs(principal));
> 81       } else {
> 82         principal = BasePrincipal::CreateCodebasePrincipal(uri, info.attrs());
> 83         rv = principal ? NS_OK : NS_ERROR_FAILURE;
> 84       }

Thank you.

Comment 12

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Hi Jonas
Since I found most of the callers of getSimpleCodebasePrincipal were done by your commmit 
https://hg.mozilla.org/mozilla-central/rev/2328647c5d6d

It looks to me that we could replace them with createCodebasePrincipal with default origin attributes.

Do you have any special reason that you need to use getSimpleCodebaePrincipal for these? for example, not to share with XUL?

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

We mainly created getSimpleCodebasePrincipal so that we could add assertions to make sure that such principals were never used to look up permissions or localStorage/IndexedDB data.

I.e. we did it to try to find code that created principals without passing in the correct appid when the appid really mattered.

This was especially important since appid was a FirefoxOS-only thing, so not something that we expected Gecko engineers to care about or do correctly.

Since OriginAttributes are used by Firefox, I think we have a much larger chance of getting them used correctly.

So yeah, I think we should replace createSimpleCodebasePrincipal with createCodebasePrincipal using default OriginAttributes. Though we might want to audit the callers to see if we should pass in a non-default OriginAttribute.

Probably good to get Huseby to look at these patches since he's audited a lot of createCodebasePrincipal callers to see if they need to use non-default OriginAttributes.

Comment 14

[Jonathan Hao (inactive) [:jhao]]

Attachment: Assert UNKNOWN_APP_ID is only used in B2G

Trying to make sure no UNKNOWN_APP_ID in desktop.

Comment 15

[Jonathan Hao (inactive) [:jhao]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2089e5a65d57

Comment 16

[Jonathan Hao (inactive) [:jhao]]

Attachment: Replace getSimpleCodebasePrincipal with createCodebasePrincipal

Hi Jonas,

I replace getSimpleCodebasePrincipal with createCodebasePrincipal with default origin attributes, except one in TestCSPParser.cpp. Would you please review this patch?

There is already a bug 1234390 to determine if those getSimpleCodebasePrincipal call sites need to user non-default OriginAttributes, but I'll ask Dave to take a look.

Thanks.

Comment 17

[Jonathan Hao (inactive) [:jhao]]

Comment on attachment 8755792 [details] [diff] [review]
Replace getSimpleCodebasePrincipal with createCodebasePrincipal

Oops! Bzexport finds the wrong reviewer.

Comment 18

[Jonathan Hao (inactive) [:jhao]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a59b3b3294e&selectedJob=21573777

Comment 19

[Jonathan Hao (inactive) [:jhao]]

The patch is ready to be checked in if Dave is okay with it.

Comment 20

[Dave Huseby [:huseby]]

This patch looks fine by me.  Let's land it! :)  Thanks.

Comment 21

[Jonathan Hao (inactive) [:jhao]]

Thank you, Jonas and Dave.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/62903b5a04a7
Replace getSimpleCodebasePrincipal with createCodebasePrincipal. r=sicking

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/62903b5a04a7