BackgroundUtils ignores origin attributes when app ID is unknown.

RESOLVED FIXED in Firefox 49

Status

()

defect
P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: huseby, Assigned: jhao)

Tracking

(Blocks 1 bug)

unspecified
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

(Whiteboard: btpp-active[OA])

Attachments

(2 attachments, 1 obsolete attachment)

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       }
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
Flags: needinfo?(jonas)
(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.
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.
Flags: needinfo?(jonas)
Whiteboard: btpp-active
Assignee: huseby → nobody
Whiteboard: btpp-active → btpp-active[OA]
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
Flags: needinfo?(honzab.moz)
(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.
Flags: needinfo?(honzab.moz)
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.
Assignee: nobody → jhao
Flags: needinfo?(schien)
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.
Flags: needinfo?(schien)
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.
Status: NEW → ASSIGNED
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.
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.
Flags: needinfo?(jonas)
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?
Priority: -- → P1
Iteration: --- → 49.2 - May 23
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.
Flags: needinfo?(jonas)
Trying to make sure no UNKNOWN_APP_ID in desktop.
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.
Attachment #8755792 - Flags: review?(jopsen)
Attachment #8755792 - Flags: feedback?(huseby)
Attachment #8754755 - Attachment is obsolete: true
Comment on attachment 8755792 [details] [diff] [review]
Replace getSimpleCodebasePrincipal with createCodebasePrincipal

Oops! Bzexport finds the wrong reviewer.
Attachment #8755792 - Flags: review?(jopsen) → review?(jonas)
Blocks: 1276412
The patch is ready to be checked in if Dave is okay with it.
Flags: needinfo?(huseby)
This patch looks fine by me.  Let's land it! :)  Thanks.
Flags: needinfo?(huseby)
Attachment #8755792 - Flags: feedback?(huseby) → feedback+
Thank you, Jonas and Dave.
Keywords: checkin-needed
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/62903b5a04a7
Replace getSimpleCodebasePrincipal with createCodebasePrincipal. r=sicking
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/62903b5a04a7
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Blocks: 1277293
Blocks: 1277299
You need to log in before you can comment on or make changes to this bug.