See https://wiki.mozilla.org/FirefoxOS/New_security_model#Origins_and_cookie_jars Here's what I proposed on an email thread: ==== First, we give nsPrincipal a field called |signature|, which is empty for unsigned content. nsPrincipal::Equals requires signatures to be equal, analogous to the AppAttributesEqual check we have there now. The |signature| field is set to the hash of the signed content by Necko after verifying that it matches the package contents. This effectively means that, excluding appid/mozbrowser for now, the origin tuple grows from (protocol, host, port) to (protocol, host, signatureOrNull, port). Second, for the benefit of serialization, we expose the signature as appropriate in nsIPrincipal::origin. The invariants surrounding this field are currently ill-defined, and I propose upgrading them to guarantee the following: |prinA.origin == prinB.origin => prinA.equals(prinB)|. This invariant currently holds modulo nsExpandedPrincipal, which I will fix. To make it hold for principal-with-signature, we can concatenate the signature with the host via some special reserved character - I'm not sure if |!| is reserved or not, but assuming it is, we'd have origins of the form: http://foo.example.com!SIGNATURE:80. This should meet the requirements for a serialization token for consumers like indexedDB. And as a string, it should be a drop-in replacement for any consumer that is currently using prepath etc. I think the possibility for widespread breakage of current nsIPrincipal::origin consumers is low - most seem to use it as an opaque identifier, and those that don't would probably parse the signature as part of the host, which is what we want. We could also opt for a cleaner |:|-based delimiter, though that has more risk of breaking things. Finally, for the various consumers for whom nsIPrincipal::equals and nsIPrincipal::origin, we implement the appropriate comparison/serialization using the primitive components of nsIPrincipal. For example, nsCookieService would check signature but not scheme, whereas the QuotaManager would check eTDL+1 but not signature. Repeat for any of the other special snowflakes where we currently handle AppID/mozBrowser. ===== To which Jonas' only proposed change was to use an app id/name instead of a signature, since we don't want app upgrades to change the origin.

Some things to do here: * Add AbstractPrincipal (or wait until that's done in bug 1129999). * Fix up nsEP::origin. * Add nsIPrincipal::signedAppName. * Modify nsPrincipal::Subsumes to take signedAppName into account. * Modify nsPrincipal::GetOrigin to insert !signedAppName as appropriate. * Tests.

Yoshi, can you file dependent bugs for fixing up the various consumers that we need to make signedApp-aware?

(In reply to Bobby Holley (:bholley) from comment #2) > Yoshi, can you file dependent bugs for fixing up the various consumers that > we need to make signedApp-aware? Right now I've listed them in https://github.com/allstarschh/b2gSecurity/blob/master/origin.md However it's mostly for the {appId, isBrowser} changes. I'll wait for Bholley to upload more patches for nsIPrinicipal changes, and then check those consumers for nsIPrincial.{equals|subsusumes|origin}. meanwhile still keeping the ni.

(In reply to Bobby Holley (:bholley) from comment #2) > Yoshi, can you file dependent bugs for fixing up the various consumers that > we need to make signedApp-aware? Hi Bobby Can you elaborate more detail about Comment 1 so I could start some some follow-up? for example "Modify nsPrincipal::Subsumes to take signedAppName into account." So far I check the callers for using nsIPrincipal, most of them are about calling equals or subsumes, which I think should be okay for our cases (add signedApp), but please correct me if I am wrong. Or do you know what we have to change as well outside caps/ when you change Subsumes? For the callers of nsIPrincipal.origin, I think I should cover them already in the list in Comment 4. Or is there some known problem for nsIPrincipal like Bug 1164567 I can start with? thanks

(In reply to Yoshi Huang[:allstars.chh] from comment #5) > Can you elaborate more detail about Comment 1 so I could start some some > follow-up? Sure. > for example "Modify nsPrincipal::Subsumes to take signedAppName into > account." > > So far I check the callers for using nsIPrincipal, most of them are about > calling equals or subsumes, which I think should be okay for our cases (add > signedApp), but please correct me if I am wrong. > Or do you know what we have to change as well outside caps/ when you change > Subsumes? Exactly - we don't need to do anything outside of CAPS here. > > For the callers of nsIPrincipal.origin, I think I should cover them already > in the list in Comment 4. > > > Or is there some known problem for nsIPrincipal like Bug 1164567 I can start > with? The main task we need you to do, I think, is to fix up the callers that manually check mozbrowser/appid to either use .origin or, for consumers like cookies that have special requirements, us the |.originAttributes| string that I'm going to expose on nsIPrincipal. I _think_ I've got all the dependencies handled, so I'll see if I can get the actual new API implemented now. Would it be helpful if I attached a .zip of my entire patch queue so that you can build on that? If so I'll upload one once I get this patches for this bug done.

Actually, I think it makes sense to separate out the .origin-munging pieces from the actual introduction of the signed app stuff. I'll spin off another dependent bug - I think Yoshi can mostly build on those pieces without worrying about the trusted app field specifically.

Thanks, Bobby. Your patches are very helpful. :D I've filed those 'consumers' bugs, and will start to fix them. (I'll also check the list again in case I missed :P) Thanks

How do these proposed changes work with multi-threading? Right now nsIPrincipal is main-thread-only. A lot of the code doing comparisons on origin/appId/browserFlag is running off-main-thread. It seems this precludes this code from using nsIPrincipal directly.

See question in comment 9 please.

(In reply to Ben Kelly [:bkelly] from comment #9) > How do these proposed changes work with multi-threading? > > Right now nsIPrincipal is main-thread-only. A lot of the code doing > comparisons on origin/appId/browserFlag is running off-main-thread. > > It seems this precludes this code from using nsIPrincipal directly. Yeah, there are basically two use-cases where you can't easily use an nsIPrincipal: (1) When you need a canonical serialization of the origin, i.e. tagging a database name. (2) When you need to do things off-main-thread. My proposed answer to (1) in this architecture is to make nsIPrincipal::origin a string that can serve as such a canonical serialization - this is what bug 1165162 does, via |originSuffix|. This solution also solves a lot of the use-cases for (2). The one case it doesn't properly solve is where you really need to invoke ->Subsumes() off-main-thread. The main barrier to making nsIPrincipal threadsafe is JS-implemented nsIURI implementations. I think we could reasonably solve this problem, but it'd be some work, so I only want to do it if there's a clear need.

Looks like RequestSyncService.jsm will also need fixing up - is that on your list Yoshi?

More generally, we're probably going to need to handle all the usages of originNoSuffix that I'm adding in bug 1165162.

(In reply to Bobby Holley (:bholley) from comment #12) > Looks like RequestSyncService.jsm will also need fixing up - is that on your > list Yoshi? Thanks for catching that, filed Bug 1165787.

I think the ServiceWorker registration persistence code will also have to be fixed up.

(In reply to Ben Kelly [:bkelly] from comment #15) > I think the ServiceWorker registration persistence code will also have to be > fixed up. Indeed. mozilla::ipc::ContentPrincipalInfo will probably want to be modified as well to be based on OriginAttributes.

The current check will fail once we start munging the format of nsIPrincipal::Origin.

We also provide an opt-out for the original behavior, and use it in various consumers that look like they need fixing up. Most of the usage here is in code with persistence considerations, where we may need some sort of migration path.

Gah, these patches were meant for bug 1165162. Sorry for the churn.

Making it clear that this bug is about the FirefoxOS-specific stuff. The general conversion to OriginAttributes is now in bug 1179985.

Hi Bobby How is the 'signedPkg' decided? by the developer or marketplace? Last week we had a meeting with Jonas/Paul, we think we could use the guid (uuid) from marketplace. But the developer will only know the guid after he submited the app to marketplace. Also we have to make sure the 'signPkg' will remain the same even after upgrade. (I am not sure if marketplace already did this or not).

The signedPkg should be read from the package. I'm not yet sure if it'll be the developer or the marketplace that decides the exact value, but that shouldn't really affect the FirefoxOS device code. The device should simply read the value from the package.

Yeah, the attribute should be set by the network layer. The patch for this bug should end up being almost identical to the one in bug 1179557.

Dimi is doing this as part of 1178526.

Quoted from Jonas's comment in Bug 1165466 comment 1: "Eventually nsILoadContext (which generally lives on the loadgroup) should just go away and be replaced by nsILoadInfo (which lives on the channel)." So how would it look like after nsILoadContext goes away? I ask this because we are figuring out how to update the origin from parent to the child. The initial attempt is to store the package identifier into the channel's URI or loadcontext (queried by NS_QueryNotificationCallbacks) and the document's principal could then be created with signed package id [1] (sure we need to add some code to do this.) If the loadcontext is away, how would be the principal created from? [1] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.cpp?from=nsdocument.cpp#2376

Comment on attachment 8659182 [details] [diff] [review] Bug1163254.patch Hi Bobby, Can I have your review on this bug? The patch will cooperate with Bug 1178526 to let the signed package have its own origin so that each signed package can have its cookie/cache/etc even from the same host. While Bug 1178526 focuses on how to set the origin, this bug is aimed at extending OriginAttributes. The IPC (de)serialization part is also included (LoadInfo). By the way, are you also able to take a look at Bug 1178526? Not sure if you are available to review that bug as well. Thanks!

Comment on attachment 8659182 [details] [diff] [review] Bug1163254.patch Review of attachment 8659182 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsJSEnvironment.cpp @@ +2544,5 @@ > if (!JS_ReadBytes(reader, spec.BeginWriting(), specLength)) { > return nullptr; > } > > + info = mozilla::ipc::ContentPrincipalInfo(appId, isInBrowserElement, spec, EmptyCString()); I don't understand why it's ok to pass EmptyCString() here. Don't you need to properly serialize and deserialize ContentPrincipalInfo? Also, this will need to be rebased on top of bug 1203916. ::: dom/webidl/ChromeUtils.webidl @@ +44,2 @@ > * (4) Bump the CIDs (_not_ IIDs) of all the principal implementations that > * use OriginAttributes in their nsISerializable implementations. You need to follow instruction (4) here. See patch part 1 of bug 1179557 for an example. @@ +48,5 @@ > unsigned long appId = 0; > unsigned long userContextId = 0; > boolean inBrowser = false; > DOMString addonId = ""; > + DOMString packageId = ""; This should be called "signedPkg", right? The 'Id' suffix usually implies (with the exception of addonId) that it's a number, which this isn't. ::: ipc/glue/BackgroundUtils.cpp @@ +202,5 @@ > if (NS_WARN_IF(NS_FAILED(rv))) { > return rv; > } > > + const mozilla::OriginAttributes& attr = Nit: trailing whitespace

Also, is this patch supposed to touch LoadInfo? Because I don't see anything about that (and I might not be the best person to review it).

Hi Bobby, Very appreciate your review! Only one question: do you mean only the name of 'packageId' needs to be changed to 'signedPkg' but the semantics can remain (the package identifier defined in the manifest)? Regarding the LoadInfo ipc part, the reason is the loading principal and the triggering principal of a load (encapsulated in LoadInfo) would be IPC from child to parent. However, there's supposed to already be a bug to change the principal IPC de/serialization to "originAttribute"-based in "BackgroundUtils.cpp". Thanks!

(In reply to Henry Chang [:henry] from comment #34) > Very appreciate your review! Only one question: do you mean only the name of > 'packageId' needs to be changed to 'signedPkg' but the semantics can remain > (the package identifier defined in the manifest)? Correct.

Comment on attachment 8661785 [details] [diff] [review] Bug1163254-r2.patch Hi Bobby, I attached a new patch which includes a test case as well. All the comments are addressed. Regarding the serialization of LoadInfo, I can move it to Bug 1163254 if you think it's not necessary in this bug. Thanks!

Comment on attachment 8661785 [details] [diff] [review] Bug1163254-r2.patch Review of attachment 8661785 [details] [diff] [review]: ----------------------------------------------------------------- ::: caps/tests/unit/test_origin.js @@ +126,5 @@ > + var exampleOrg_signedPkg = ssm.createCodebasePrincipal(makeURI('http://example.org'), {signedPkg: 'whatever'}); > + checkOriginAttributes(exampleOrg_signedPkg, { signedPkg: 'id' }, '^signedPkg=whatever'); > + do_check_eq(exampleOrg_signedPkg.origin, 'http://example.org^signedPkg=whatever'); > + > + // signedPkg and appId Given that we'll never have something that is both in a signed package and has an appId (new vs old security models), I think we should remove this part. ::: ipc/glue/PBackgroundSharedTypes.ipdlh @@ +11,5 @@ > { > uint32_t appId; > bool isInBrowserElement; > nsCString spec; > + nsCString signedPkg; Note that these changes will end up reverted when bug 1167100 lands, but that's probably fine.

One more thing Henry - I just discussed with Jonas and Boris, and we've decided that the current serialization strategy means that we don't need to bump the CID anymore. So could you actually revert that change, and remove instruction (4) from ChromeUtils.webidl? Sorry for changing this up, but hopefully things will be easier in the future now. :-)

(In reply to Bobby Holley (:bholley) from comment #40) > One more thing Henry - I just discussed with Jonas and Boris, and we've > decided that the current serialization strategy means that we don't need to > bump the CID anymore. So could you actually revert that change, and remove > instruction (4) from ChromeUtils.webidl? > Definitely sure! Thanks for your quick review again :) > Sorry for changing this up, but hopefully things will be easier in the > future now. :-)

(In reply to Bobby Holley (:bholley) from comment #39) > Comment on attachment 8661785 [details] [diff] [review] > Bug1163254-r2.patch > > Review of attachment 8661785 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: caps/tests/unit/test_origin.js > @@ +126,5 @@ > > + var exampleOrg_signedPkg = ssm.createCodebasePrincipal(makeURI('http://example.org'), {signedPkg: 'whatever'}); > > + checkOriginAttributes(exampleOrg_signedPkg, { signedPkg: 'id' }, '^signedPkg=whatever'); > > + do_check_eq(exampleOrg_signedPkg.origin, 'http://example.org^signedPkg=whatever'); > > + > > + // signedPkg and appId > > Given that we'll never have something that is both in a signed package and > has an appId (new vs old security models), I think we should remove this > part. > Okay. I will just use other attributes to test if the 'signedPkg' could exist with other attributes. Thanks!

Waiting for try result of patch r3 https://treeherder.mozilla.org/#/jobs?repo=try&revision=55af77a77859

backed this out for causing test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=14257994&repo=mozilla-inbound

The try result for the patch just attached: https://treeherder.mozilla.org/#/jobs?repo=try&revision=db3ca16711e0