We need to come up with some way that a store can sign an app package, and for Gecko to verify that signature against a list of stores that we trust.

I mean, verify the signature against the public keys against the set of stores that we trust to sign trusted apps.

Hi Brian, do you have any updates on your general approach?

I'm very interested on knowing what's the general approach too. Any pointers will be appreciated.

I have started writing up some documentation but I'm not done yet. A very quick summary of the approach I've been working on: 1. We will use the same signed JAR format that Android apps, Firefox extensions, and Android update packages. This is not a particularly nice format but it simplifies the tooling required and makes it as similar to Android as possible. (In fact, I believe it may be possible to create a single JAR that runs as either a packaged web app AND an Android app, depending on what platform it gets installed on.) 2. At build time, we will add a new CA certificate that will become the trust anchor for the verification of certificates used to sign certified apps. Only apps signed by a certificate issued by this CA will be trusted with certified privileges. For trusted apps, s/certified/trusted/ in the previous sentence. (I am still working on how to automate the addition of these two certificates into the root trust store in the build.) 3. For testing, we will have environment variables that allows us to temporarily substitute different trust anchors. 4. I have modified the JAR signature verification code in Gecko so that it can properly handle JARs that have multiple signatures, so that (in the future) we can require certified apps to be signed by multiple trust anchors (e.g. Mozilla AND the hardware partner). I have also modified it to be asynchronous.

For step 2, have you considered having a CA hierarchy instead of just a CA? That is, allowing the pinned CAs to be root CAs that sign CAs that sign developer/store/whatever certs? Also... what happens if I (as an operator, or hardware builder) don't want my devices to run code signed by another operator/hardware builder/whatever as certified code? I know, those two questions go into completely different directions but then again trust is a finicky issue On a non entirely unrelated note, if you need/want any help with this let me know.

(In reply to Antonio Manuel Amaya Calvo from comment #5) > For step 2, have you considered having a CA hierarchy instead of just a CA? > That is, allowing the pinned CAs to be root CAs that sign CAs that sign > developer/store/whatever certs? That is exactly what I am intending--that's why I specifically said it must be a CA certificate, as opposed to an EE certificate. The trust anchors should be able to make things as complicated as they want. > Also... what happens if I (as an operator, > or hardware builder) don't want my devices to run code signed by another > operator/hardware builder/whatever as certified code? Who decides what trust anchors to include on the device is yet to be decided. However, the idea of making it build-time configurable is specifically to make it possible to require different trust anchors and/or different certificate policy extensions to be present on a per-build basis. > On a non entirely unrelated note, if you need/want any help with this let me > know. Sure. Will follow up by email. Does the choice of the JAR (APK) file format seem reasonable to you? I had originally planned on wrapping the JAR in a signed CMS message, because that's a much simpler format for us to process, but it seems all signing in Android is done with signed JARs so I've switched to that to allow people to build upon their Android experience and to allow people to use their Android toolchains/workflows.

Frankly I was thinking it was going to be a just a PKCS#7/CMS (I'm old :P ). For the use we're going to give it I don't know if we're going to get any value of having separate hashes for each file. Besides, it makes checking them much more of a hassle without, as I said, any palpable return from the added complexity. When are you thinking on checking the signatures? Just on install time or on load time? But on the other hand it's true this might make the format more successful since it's what people already do on Android. Although... I thought signing wasn't something end developers were going to do, but something stores/distributors would do. If that's so still, then maybe a new format wouldn't be as much problem. So in short, I don't know :) Both proposals have advantages.

(In reply to Antonio Manuel Amaya Calvo from comment #7) > Frankly I was thinking it was going to be a just a PKCS#7/CMS (I'm old :P ). > For the use we're going to give it I don't know if we're going to get any > value of having separate hashes for each file. Besides, it makes checking > them much more of a hassle without, as I said, any palpable return from the > added complexity. I agree with you. CMS makes more sense for this application, technically. But, in Gecko, we already have the need to verify JAR signatures (for Firefox addons) and if we reuse the Android system update infrastructure wholesale (which we are, AFAIK) then AFAICT we'll need the code for verifying JAR signatures for dealing with update.zip too. > When are you thinking on checking the signatures? Just on > install time or on load time? Just at install time and maybe update time. > But on the other hand it's true this might make the format more successful > since it's what people already do on Android. Although... I thought signing > wasn't something end developers were going to do, but something > stores/distributors would do. If that's so still, then maybe a new format > wouldn't be as much problem. I agree that changing to CMS wouldn't present too many challenges. I have actually written most of the CMS-based implementation already. The difference between the CMS version of things and the JAR version of things isn't huge, so I'm going to finish the JAR-based implementation first so we have something now, and if we decide that we want to use CMS I'll just write another patch.

Ping... how are we doing here? This is essential to getting packaged & privileged apps done.

Do we really need to do this for v1? We are getting very close to feature freeze and Brian is doing a lot of other important work. Would it be ok to simply rely on using https to the mozilla store for this in v1?

I for one would rather do away with privileged apps for V1 than to keep them but to do away with their only integrity/local verification measures, as they were. I offered before to Brian and I do it again, if he wants any help I'll be glad to pitch in to try and finish this before the 28th.

We'll need to know how the marketplace should sign apps for v1, which of these approaches is being used?

We've already got a system for signing receipts: https://wiki.mozilla.org/Apps/WebApplicationReceipt/SigningService Although the format of a receipt vs a package is different, the actual requirements of signing a package in terms of crypto is very similar. If we use this, we can re-use the same processes and code. Would this be worth exploring? cc @rtilder who wrote the signing service.

I will be working on this again this week, and it will be done by the 28th for sure. (Note, though, that it is my team's workweek this week.)

If you can tell us how you want the marketplace to sign apps as soon as you know that will be appreciated, since we need to get that done by the 28th too.

Spoke to bsmith on irc and confirmed this will be the same as add-on signing. The actual certificate etc. will be confirmed, but this is enough to move forward with a prototype for the marketplace.

Add-on process: https://developer.mozilla.org/en-US/docs/Signing_a_XPI

There should be a pref to disable sig verification or whitelist signing certs. This is necessary for app reviewers (bug 791743) and developers to test their apps.

(In reply to Lucas Adamski from comment #18) > There should be a pref to disable sig verification or whitelist signing > certs. This is necessary for app reviewers (bug 791743) and developers to > test their apps. Setting dom.mozApps.dev_mode should handle the latter case. However a pref or something would still be needed for the review case See bug 768868

Why would those two be different enough to require a separate pref?

(In reply to Lucas Adamski from comment #20) > Why would those two be different enough to require a separate pref? Actually you're right, the same pref could be used for both. I was stuck on the bit where the manifest would need to specify the correct type, but if they are submitting a privileged / certified app, then the manifest must have that type.

How is this coming along?

This is the most important & urgent remaining security feature bug for basecamp; its blocking Marketplace and end-to-end testing.

Brian, how are we doing here? This is extremely urgent at this point.

I understand the marketplace team wants to use xpisign.py to sign the apps. I modified the tool to generate (optimized) signatures that will be accepted by the verifier I wrote. You can see WIP at https://github.com/briansmith/xpisign.py/commits/master Who is going to be responsible for the code review on xpisign.py? Not only my small changes need to be reviewed, but also the original code. In order to understand if/how safe using this code is, we need to have documentation on the environment in which it will be run. In particular, this tool doesn't support storing the private key that signs the app on a smartcard, and it doesn't even support using a password to protect the private key. So, how is the private key going to be protected? Also, at least one feature (the "compression optimizer") of the xpisign.py are nicely documented as being unsafe for concurrent use. So, we need to make sure we don't use that feature.

(In reply to Brian Smith (:bsmith) from comment #25) > You can see WIP at > https://github.com/briansmith/xpisign.py/commits/master Now on the "b2g" branch, and simplified: https://github.com/briansmith/xpisign.py/commits/b2g. Also, here's the "final" script I am using for my test cert. It would be helpful to have new test certs generated using the parameters in this script (including the .cfg file also attached). In particular, the key usage, extended key usage, Netscape cert type, and signature key size (RSA 2048) and hash algorithm (SHA256) are all different than in the original test cert. The serial numbers and the actual names used in certs can be changed freely. I used "Examplla" because it doesn't seem like a good idea to generate certs that say "Mozilla" unless they are for production.

(In reply to Brian Smith (:bsmith) from comment #25) > I understand the marketplace team wants to use xpisign.py to sign the apps. > I modified the tool to generate (optimized) signatures that will be accepted > by the verifier I wrote. You can see WIP at > https://github.com/briansmith/xpisign.py/commits/master > > Who is going to be responsible for the code review on xpisign.py? Not only > my small changes need to be reviewed, but also the original code. In order > to understand if/how safe using this code is, we need to have documentation > on the environment in which it will be run. At this stage we might not use xpisign. Was chatting to Ryan about this the other day and he's rewritten most of the code in order to support our needs and infrastructure. xpisign was used on the -dev server to provide us with a basic functioning walk through whilst the proper back end was built. > In particular, this tool doesn't support storing the private key that signs > the app on a smartcard, and it doesn't even support using a password to > protect the private key. So, how is the private key going to be protected? We intend to use the HSM for the private key generation. > Also, at least one feature (the "compression optimizer") of the xpisign.py > are nicely documented as being unsafe for concurrent use. So, we need to > make sure we don't use that feature. Good to know.

This patch removes the nsNSSCertHeader.h file. This header is used for the declarations of some internal NSS functions that are exposed especially for PSM. However, now there is only one such function. The existing mechanism using this header causes linking errors if you don't include nsNSSCertHeader.h before any header that includes cert.h. This patch avoids the whole issue.

Bob, this is the patch we discussed yesterday during the teleconference. This should be the only temporary patch to NSS that we'll need (besides the trivial permanent patch in bug 808218). Basically, we maintain two certdata.txt files; one with the normal existing content and one with the B2G-specific content; then we basically combine them together and generate certdata.c at build time from the merged result. Earlier, you mentioned that it would be better to avoid CKT_NSS_NOT_TRUSTED, because it might cause problems if/when we try to validate a cert for multiple usages or all usages. I investigated this and I don't think there's any place in B2G where we do such verification. In Firefox we do that for the certificate UI, but B2G doesn't have that feature. So, I think CKT_NSS_NOT_TRUSTED is still the best choice, especially because our partners are very interested in minimizing the possibility that other code signing roots could work. The long-term plan is to use libpkix, which allows us to control over what roots are considered trust anchors for a given validation. But, that requires us to write a new version of SEC_PKCS7VerifyDetachedSignature that integrates with libpkix instead of the classic cert verification. I filed bug 808224 for the necessary changes to NSS needed to do that.

Comment on attachment 677924 [details] [diff] [review] Remove code signing capabilities of existing built-in CAs and add the new test root for B2G app signing r- I'm OK with most of the patch. The main issue I have is removing certdata. The main reason we don't always build certdata is because it generated a requirement for a specific version of perl on all platforms. At the time that could be problematic. I can be convinced to change this to an r+ if we have convincing evidence that this change won't break any platforms we care about (I suspect you built on Windows, which is probably the most problematic, so it may not take too much to convince me). bob

re CKT_NSS_NOT_TRUSTED versus CKT_NSS_TRUST_MUST_VERIFY.... Just so you know, this would make certs in the database less capable than certs that we don't have in the database. The only difference between these for root certs is the error code the application gets. I suspect that you wouldn't allow an override on any error code in this scenario anyway.

Comment on attachment 677810 [details] script used to generate test CA cert and test EE cert Script move to bug 793876.

I changed the cert trust flags based on rrelyea's feedback. I don't think that the perl dependency is an issue, because the main place it would bite us would be Windows, and I did all the testing on Windows.

These are unit tests for the JAR signature verification part. These tests use a custom root cert, so that we can run them on all platforms. We will need additional tests for testing the integration with installPackage, for testing the actual built-in app signing root certificate.

This is the script I am using to generate the JAR files for testing.

Are there any automated tests for the installPackage API? I don't see any in mozilla-central. Are they in the Gaia test suite? Is the only way to test installPackage to do an actual B2G build? Also, what is the UI supposed to be when the package fails to verify? Just some error message?

(In reply to Brian Smith (:bsmith) from comment #42) > Are there any automated tests for the installPackage API? I don't see any in > mozilla-central. Are they in the Gaia test suite? Is the only way to test > installPackage to do an actual B2G build? Also, what is the UI supposed to > be when the package fails to verify? Just some error message? To my knowledge - there's no automated tests for installPackage (which isn't good :() in the mozilla-central and Gaia test suite. The API recently changed and just started functioning again, so it's a good time to revisit this though. For now in the short-term - I'd use an actual b2g build running on unagi. We do need to resolve the automated tests issue however. For the UI question - I'll send that question over to Josh.

> Also, what is the UI supposed to be when the package fails to verify? Just some error message? This is the first time I've heard of this possibility. The latest App Install specs (which I can't link to ATM apparently because of DB technical problems), provide error handling UX for failed/stopped downloads, but not for unverified packages. Off the top of my head I think we'd want to alert the user to the situation (app failed to verify) and provide them with a means to delete said app (or force the deletion). Let's talk this week in SF about this. cc'ing Ben Francis, who is working on the Gaia implementation of all things Install-related.

For the corrupted package UI, something generic like "this application appears to be corrupted, please try installing again later" would suffice.

We're marking this bug with the C1 milestone since it follows the criteria of "unfinished feature work" (see https://etherpad.mozilla.org/b2g-convergence-schedule). If this work is not finished by Nov19, this bug will need an exception and will be called out at the upcoming Exec Review.

Comment on attachment 678235 [details] [diff] [review] Part 0: Remove nsNSSCertHeader.h https://hg.mozilla.org/integration/mozilla-inbound/rev/4ed8af50dc10

https://hg.mozilla.org/integration/mozilla-inbound/rev/0aa4bfb23063 fixes build bustage caused by cset 4ed8af50dc10.

https://hg.mozilla.org/mozilla-central/rev/4ed8af50dc10 https://hg.mozilla.org/mozilla-central/rev/0aa4bfb23063

Comment on attachment 678238 [details] [diff] [review] Part 2: Add configure option to switch to B2G code signing semantics Review of attachment 678238 [details] [diff] [review]: ----------------------------------------------------------------- Is there a reason to not just force this on by default for B2G builds? Also, if you're going to do this, please don't add the actual configure option. Just add the AC_SUBST and export the variable from the mozconfigs. I don't think this is something we want people fiddling with.

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #50) > Is there a reason to not just force this on by default for B2G builds? That's basically what I did, right? I guess you mean that there is a better way of doing it; please let me know what it is and I will gladly do it that way. > Also, > if you're going to do this, please don't add the actual configure option. > Just add the AC_SUBST and export the variable from the mozconfigs. I don't > think this is something we want people fiddling with. OK. I added the configure option because that was the simplest way I know of to allow me to enable this in my own mozconfig on Windows and on non-B2G Linux builds. This is important because my primary development platform is Windows and because most of the NSS developers do not ever build B2G.

By "default" I meant "without having to specify it in the mozconfig". The simplest way to do this would be to remove everything but the AC_SUBST from configure.in, and add MOZ_B2G_CODE_SIGNING_HACK=1 to confvars.sh: http://mxr.mozilla.org/mozilla-central/source/b2g/confvars.sh You could then enable this behavior in your own Firefox builds by adding MOZ_B2G_CODE_SIGNING_HACK=1 to your mozconfig.

Comment on attachment 681691 [details] [diff] [review] Add build option to switch to B2G code signing semantics Review of attachment 681691 [details] [diff] [review]: ----------------------------------------------------------------- Much simpler, thanks!

Ted, I tried many, many things to fix this bug, but I couldn't figure out anything that actually works besides doing this.

Ted, I tried many, many things to fix this bug, but I couldn't figure out anything that actually works besides doing this.

This is the updated script that generates certificates and signs a test app with a trusted cert and an untrusted cert. The new script is based on NSS (certutil) instead of OpenSSL.

The instructions for generating new certificates/JARs using patch 2.1 are contained in a comment in test_signed_apps.js in this patch.

Of the headers that are directly included from ScopedNSSTypes.h, secpkcs7.h is missing in config/system-headers. That would certainly explain visibility issues.

Comment on attachment 682525 [details] [diff] [review] Part 2.1: Generate test cases for signed app signature verification r+ for the shell script with some comments.... 1) probably should warn that the first argument in replace_zip must be an absolute path. (It is in it's use of the script). 2) The comment in sign_app_with_new_cert() isn't totally correct. You don't have to make the subjects the same, The reason you get an error is because you have specified a fixed serial number (-m). If you used a random or different serial number for each cert, you would not have the issuer_and_serial number issue. Solving the problem by changing the subject is probably good, however. You really don't want two certs with the same subject but different keys unless you are careful about including key authority identifier extensions. 3) one potential issue... it looks like you've marked the untrusted root as trusted it it's database. I suspect that you don't use that database when you test to see if the signature succeeds, however. 4) Just a style thing.. it might be easier to read the response files if you created them with something like: cat > $ca_responses << __EOF__ y y n y __EOF_ With an appropriate preceding comment. Not required, just a suggestion (an argument could be made that the way you've commented the code, your existing construct is easier to read). 5) I'm not python expert, so I've left the python code unreviewed... bob

(In reply to Robert Relyea from comment #62) > 2) The comment in sign_app_with_new_cert() isn't totally correct. You don't > have to make the subjects the same, The reason you get an error is because > you have specified a fixed serial number (-m). If you used a random or > different serial number for each cert, you would not have the > issuer_and_serial number issue. Solving the problem by changing the subject > is probably good, however. You really don't want two certs with the same > subject but different keys unless you are careful about including key > authority identifier extensions. So, my goal is to have the "untrusted" cert be just like the "trusted" cert as much as possible (ideally only the key is different), because the test is trying to ensure that we accept the "trusted" cert and we reject the "untrusted" cert only on the basis of the trust bits in the NSS trust database (the "trusted" root is given code-signing trust, but the "untrusted" root is not). > 3) one potential issue... it looks like you've marked the untrusted root as > trusted it it's database. I suspect that you don't use that database when > you test to see if the signature succeeds, however. Your suspicion is correct. We actually throw away these databases after we generate the cert DER files. > 5) I'm not python expert, so I've left the python code unreviewed... I hope oremj will do the "python expert" code review. Thanks for the quick turnaround!

Comment on attachment 682522 [details] [diff] [review] Part 1.2: Implement JAR signature verification based on nsJAR's verification code [WIP] Review of attachment 682522 [details] [diff] [review]: ----------------------------------------------------------------- some white spaces visible in splinter '"Name:" attribute, which specifies either a relative file path or URL' - do we consider possibility of Name being a URL? I think not and as I understand we fail for that case since we do not find the file in the JAR, right? ::: security/manager/ssl/public/nsIX509CertDB.idl @@ +19,5 @@ > > +[scriptable, function, uuid(48411e2d-85a9-4b16-bec8-e30cde801f9e)] > +interface nsIOpenSignedJARFileCallback : nsISupports > +{ > + void OpenSignedJARFileFinished(in nsresult rv, first letter lower case ::: security/manager/ssl/src/JARSignatureVerification.cpp @@ +32,5 @@ > +extern PRLogModuleInfo* gPIPNSSLog; > + > +namespace { > + > +// Finds exactly one (signatujre metadata) entry that matches the given signatuJre @@ +46,5 @@ > +{ > + nsCOMPtr<nsIUTF8StringEnumerator> files; > + nsresult rv = zip->FindEntries(searchPattern, getter_AddRefs(files)); > + if (!files) rv = NS_ERROR_SIGNED_JAR_MANIFEST_INVALID; > + if (NS_FAILED(rv)) return NS_ERROR_SIGNED_JAR_MANIFEST_INVALID; body on a new line please and could be merged to one condition. @@ +69,5 @@ > + } > + > + nsCOMPtr<nsIInputStream> stream; > + rv = zip->GetInputStream(filename, getter_AddRefs(stream)); > + if (NS_FAILED(rv)) return NS_ERROR_FILE_TARGET_DOES_NOT_EXIST; Shouldn't this have NS_ENSURE.. ? (also on some other places) @@ +96,5 @@ > + // buf.len == len64 + 1. If we read more or less than len64 bytes then the > + // metadata in the ZIP for the entry is corrupted and we should stop reading > + // it. > + uint32_t bytesRead; > + rv = stream->Read(char_ptr_cast(buf.data), buf.len, &bytesRead); buf.len - 1 actually, but that probably doesn't matter. @@ +120,5 @@ > +// Verify the digest of an entry. We avoid loading the entire entry into memory > +// at once, which would require memory in proportion to the size of the largest > +// entry. Instead, we require only a small, fixed amount of memory. > +// > +// @param digest The digest that we're supposed to check the file's contents the arg is actually digestFromMF Best would be digestFromManifest (I was a moment wondering what MF was) @@ +150,5 @@ > + if (len64 > UINT32_MAX) { > + return NS_ERROR_SIGNED_JAR_ENTRY_TOO_LARGE; > + } > + > + uint64_t totalBytesRead = 0; declare before for (;;) { @@ +197,5 @@ > + return NS_OK; > +} > + > +int32_t > +ReadLine(const char** src) Wouldn't it be nicer API to let this just return the complete line string? src should be called |caret| instead. The same for nextLineStart in the following code, IMO. @@ +208,5 @@ > + if (eol == nullptr) // Probably reached end of file before newline > + { > + length = PL_strlen(*src); > + if (length == 0) // immediate end-of-file > + *src = nullptr; This can turn nextLineStart (the caret) to become null. You have a code that dereferences nextLineStart after call to this function. @@ +242,5 @@ > + /*out*/ nsAutoCString & attrValue) > +{ > + //-- Look for continuations (beginning with a space) on subsequent lines > + // and append them to the current attribute. > + while(*nextLineStart == ' ') while ( nextLineStart can be null here! @@ +246,5 @@ > + while(*nextLineStart == ' ') > + { > + const char* curPos = nextLineStart; > + int32_t continuationLen = ReadLine(&nextLineStart) - 1; > + nsAutoCString continuation(curPos + 1, continuationLen); Nit: Not sure, but you could use dependent string here. @@ +253,5 @@ > + } > + > + //-- Find colon in current line, this separates name from value > + int32_t colonPos = curLine.FindChar(':'); > + if (colonPos == -1) // No colon on line, ignore line kNotFound @@ +273,5 @@ > + int32_t linelen; > + const char* curPos = nextLineStart; > + linelen = ReadLine(&nextLineStart); > + curLine.Assign(curPos, linelen); > + if (!curLine.Equals(expectedHeader)) Any casing exception here possible? @@ +279,5 @@ > + > + return NS_OK; > +} > + > +// Parses an signature file (SF) as defined in the JDK 8 JAR Specification. a signature @@ +332,5 @@ > + continue; // No colon on line, ignore line > + } > + > + if (attrName.LowerCaseEqualsLiteral("sha1-digest-manifest")) { > + rv = MapSECStatus(ATOB_ConvertAsciiToItem(&mfDigest, attrValue.get())); attrValue.BeginReading() @@ +338,5 @@ > + return rv; > + > + // There could be multiple SHA1-Digest-Manifest attributes, which > + // would be an error, but it's better to just skip any erroneous > + // duplicate entries rather than trying to detect them, because: The spec says: If more than one x-Digest-Manifest attribute exists in the signature file, verify that at least one of them matches the calculated digest value. I'm also not keen to respect that. However some compatibility concern is possible here. @@ +365,5 @@ > + nsresult rv; > + > + const char* nextLineStart = filebuf; > + > + rv = ParseManifestHeader(nextLineStart, nsLiteralCString(JAR_MF_HEADER)); This should be called CheckManifestHeader. @@ +375,5 @@ > + int32_t linelen; > + do { > + linelen = ReadLine(&nextLineStart); > + } while (linelen > 0); > + } This code can make nextLineStart null. @@ +377,5 @@ > + linelen = ReadLine(&nextLineStart); > + } while (linelen > 0); > + } > + > + const char* sectionStart = nextLineStart; Why do you need sectionStart ? @@ +381,5 @@ > + const char* sectionStart = nextLineStart; > + nsAutoCString curItemName; > + ScopedAutoSECItem digest; > + > + for(;;) for ( @@ +411,5 @@ > + // Verify that the entry's content digest matches the digest from this > + // MF section. > + rv = VerifyEntryContentDigest(zip, curItemName, digest, buf); > + if (NS_FAILED(rv)) > + return rv; There should be logs or some console logging that will tell us about verification failure reasons. This is a bit similar to how offline cache updates has recently worked (absolutely no info about what is failing). @@ +442,5 @@ > + if (attrName.LowerCaseEqualsLiteral("sha1-digest")) > + //-- This is a digest line, save the data in the appropriate place > + { > + if (digest.len > 0) > + return NS_ERROR_SIGNED_JAR_MANIFEST_INVALID; Add a comment: // Duplicate digest found @@ +463,5 @@ > + > + curItemName = attrValue; > + > + continue; > + } Should we be somehow strict on ordering the attributes? (Name first, then signatures) @@ +491,5 @@ > +PRBool DecryptionAllowedCallback(SECAlgorithmID *algid, PK11SymKey *bulkkey) > +{ > + return false; > +} > +void * GetPasswordKeyCallback(void *arg, void *handle) { return nullptr; } { return nullptr; } @@ +533,5 @@ > + ScopedSEC_PKCS7ContentInfo p7_info(SEC_PKCS7DecodeItem(&sigBuffer, > + ContentCallback, nullptr, > + GetPasswordKeyCallback, nullptr, > + GetDecryptKeyCallback, nullptr, > + DecryptionAllowedCallback)); Indention @@ +599,5 @@ > + } > + > + // Verify every entry in the file. > + nsCOMPtr<nsIUTF8StringEnumerator> entries; > + rv = zip->FindEntries(nsLiteralCString(""), getter_AddRefs(entries)); NS_LITERAL_CSTRING ?

Comment on attachment 682528 [details] [diff] [review] Part 2.2: Test JAR signature verification Review of attachment 682528 [details] [diff] [review]: ----------------------------------------------------------------- I didn't run the tests personally (might be a mistake, but it is after midnight here...) I think we can land this. ::: security/manager/ssl/tests/unit/test_signed_apps.js @@ +6,5 @@ > + > +/* To regenerate the certificates and apps for this test: > + > + cd security/manager/ssl/tests/unit/test_signed_apps > + PATH=$NSS/bin:$NSS/lib:$PATH ./generate.sh And where is this script? @@ +134,5 @@ > +function run_test() { > + var root_cert_der = > + do_get_file("test_signed_apps/trusted_ca1.der", false); > + var der = readFile(root_cert_der); > + certdb.addCert(der, ",,CTu", "test-root"); Just curious why you cannot add it as base64 (wouldn't need to modify the interface) @@ +247,5 @@ > + Cr.NS_ERROR_SIGNED_JAR_UNSIGNED_ENTRY)); > +}); > + > +// TODO: tampered MF, tampered SF > +// TODO: too-large MF, too-large RSA, too-large SF // TODO: MF and SF that end immediately after the last main header (no CR nor LF) // TODO: broken headers to exercise the parser

Comment on attachment 682525 [details] [diff] [review] Part 2.1: Generate test cases for signed app signature verification Review of attachment 682525 [details] [diff] [review]: ----------------------------------------------------------------- I'll sr this nextweek. Feel free to land this w/o my sr, I think that is good enough to consider this patch reviewed when tests just work.

I didn't read security/manager/ssl/tests/unit/test_signed_apps/sign_b2g_app.py closely enough to understand it all but I did catch a few Python things: > from __future__ import with_statement This is only needed on Python 2.5. We use Python 2.6 on our servers typically so I'm guessing we don't need this. > ++entry_count This doesn't do what you might think if you're coming from C. Python doesn't have a increment unary operator. What this does is essentially (+(+entry_count)), which doesn't increment the number. It looks like you want `entry_count += 1` instead.

(In reply to Mike Hommey [:glandium] from comment #61) > Of the headers that are directly included from ScopedNSSTypes.h, secpkcs7.h > is missing in config/system-headers. That would certainly explain visibility > issues. Ted and Mike, thanks for helping with this. Unfortunately, adding secpkcs7.h to system-headers didn't solve the problem, and adding all NSS header files to config/system-headers caused a different failure. I filed bug 813241 to fix this the right way, but for now let's just land the workaround in attachment 682520 [details] [diff] [review]. I've spent *several* hours on this problem already and it is important that this feature land ASAP.

Honza, here's the patch on top of Part 1.2 that addresses your review comments. There are too many critical changes to go without another review. (In reply to Honza Bambas (:mayhemer) from comment #64) > some white spaces visible in splinter Fixed. > '"Name:" attribute, which specifies either a relative file path or URL' - do > we consider possibility of Name being a URL? I think not and as I > understand we fail for that case since we do not find the file in the JAR, > right? Right. You cannot (or at least, should not) use the URL form to reference entries within the JAR, so it is working as intended. > @@ +69,5 @@ > > + } > > + > > + nsCOMPtr<nsIInputStream> stream; > > + rv = zip->GetInputStream(filename, getter_AddRefs(stream)); > > + if (NS_FAILED(rv)) return NS_ERROR_FILE_TARGET_DOES_NOT_EXIST; > > Shouldn't this have NS_ENSURE.. ? (also on some other places) I changed some uses of to NS_ENSURE*, but not all of them. My understanding is that we're supposed to use NS_ENSURE* for things that are basically never supposed to happen, and we're supposed to use "if (NS_FAILED(rv))" for conditions that we're expecting to fail sometimes. So, for example, we're not expecting to have file->Available() fail but we do expect the input JAR's manifests to have errors some times (otherwise we wouldn't need to write all this cryptic code). > > @@ +96,5 @@ > > + // buf.len == len64 + 1. If we read more or less than len64 bytes then the > > + // metadata in the ZIP for the entry is corrupted and we should stop reading > > + // it. > > + uint32_t bytesRead; > > + rv = stream->Read(char_ptr_cast(buf.data), buf.len, &bytesRead); > > buf.len - 1 actually, but that probably doesn't matter. I clarified the comments for this. It is actually important that we use buf.len instead of buf.len - 1, because we need to check to see if the file has more data available than Available() reported. > > +int32_t > > +ReadLine(const char** src) > > Wouldn't it be nicer API to let this just return the complete line string? Yes, but note that the Manifest-Version and Signature-Version lines are not allowed to be split. > @@ +273,5 @@ > > + int32_t linelen; > > + const char* curPos = nextLineStart; > > + linelen = ReadLine(&nextLineStart); > > + curLine.Assign(curPos, linelen); > > + if (!curLine.Equals(expectedHeader)) > > Any casing exception here possible? No. The spec says that Signature-Version and Manifest-Version are the *only* attributes that are case-sensitive. > > @@ +332,5 @@ > > + continue; // No colon on line, ignore line > > + } > > + > > + if (attrName.LowerCaseEqualsLiteral("sha1-digest-manifest")) { > > + rv = MapSECStatus(ATOB_ConvertAsciiToItem(&mfDigest, attrValue.get())); > > attrValue.BeginReading() Why? I am not sure what the advantage of BeginReading() is over get(), and I've had reviewers say I should only use BeginReading() if I use EndReading(). > @@ +338,5 @@ > > + return rv; > > + > > + // There could be multiple SHA1-Digest-Manifest attributes, which > > + // would be an error, but it's better to just skip any erroneous > > + // duplicate entries rather than trying to detect them, because: > > The spec says: If more than one x-Digest-Manifest attribute exists in the > signature file, verify that at least one of them matches the calculated > digest value. The spec means that there might be a SHA1-Digest-Manifest and a SHA256-Digest-Manifest. But, there cannot be two SHA1-Digest-Manifest entries, because attribute names must be unique (case insensitive) in each section. > This should be called CheckManifestHeader. Renamed to CheckManifestVersion() > There should be logs or some console logging that will tell us about > verification failure reasons. This is a bit similar to how offline cache > updates has recently worked (absolutely no info about what is failing). The code is very careful to return the correct error code. I will add the logging to the code that calls this function, because different callers need to log the failure in different ways (e.g. web console vs. error console). > Should we be somehow strict on ordering the attributes? (Name first, then > signatures) There's no need for security reasons and the spec seems to allow them in any order. > @@ +533,5 @@ > > + ScopedSEC_PKCS7ContentInfo p7_info(SEC_PKCS7DecodeItem(&sigBuffer, > > + ContentCallback, nullptr, > > + GetPasswordKeyCallback, nullptr, > > + GetDecryptKeyCallback, nullptr, > > + DecryptionAllowedCallback)); > > Indention I think this is OK as-is. There's not really a great way to indent it. Thanks for the careful reviews!

(In reply to Honza Bambas (:mayhemer) from comment #65) > ::: security/manager/ssl/tests/unit/test_signed_apps.js > @@ +6,5 @@ > > + > > +/* To regenerate the certificates and apps for this test: > > + > > + cd security/manager/ssl/tests/unit/test_signed_apps > > + PATH=$NSS/bin:$NSS/lib:$PATH ./generate.sh > > And where is this script? It is in Part 2.1. I split the patch like this because there are different sets of reviewers and other people might use parts of Part 2.1 for the server-side stuff. > @@ +134,5 @@ > > +function run_test() { > > + var root_cert_der = > > + do_get_file("test_signed_apps/trusted_ca1.der", false); > > + var der = readFile(root_cert_der); > > + certdb.addCert(der, ",,CTu", "test-root"); > > Just curious why you cannot add it as base64 (wouldn't need to modify the > interface) I couldn't figure out a way to base-64 encode the cert in xpcshell, because it seems like btoa is not available in xpcshell (because it's window.btoa, and there's no window in xpcshell).

Addressed review comments. Thanks Rob and Bob! I thought about changing generate.sh to use different serial numbers instead of different issuer names, but I punted on it due to lack of time. Bob, let me know if my previous reply to you on this is unclear or if I am overlooking something.

I added the comments about missing tests. Unfortunately, it is harder than normal to add tests for the SF and MF parsers. We verify signatures before we parse those files (which is the safest and best order of operations), but that means to test the parsers we must have signed *invalid* apps. That requires modifying sign_b2g_app.py and generate.sh to allow the creation of bad apps. But, I want that code to serve as a model for the production app signing so I don't want to obfuscate it with that functionality yet. Plus, I am running out of time. I will try to come back around with a patch that adds the additional tests later.

Comment on attachment 683376 [details] [diff] [review] Part 2.1: Generate test cases for signed app signature verification [v2] Jeremy, please review sign_b2g_app.py and nss_ctypes.py.

Comment on attachment 683376 [details] [diff] [review] Part 2.1: Generate test cases for signed app signature verification [v2] Changing review request to rtilder based on this IRC comment: <rtilder> Somewhat entertainingly related: several years ago(7 or 8 now? eek) I wrote a Python wrapper for NSS.

Fixed typo (s/dml/dnl/).

This patch: (1) Removes, for B2G, the code signing trust bit from all the CAs in our root program. (2) Adds the B2G app signing cert as a cert trusted for code signing. The certificate is actually not the one we will finally deploy, and it has the nsCertType extension that we will remove in the next iteration, but I'd like to get the logic in the patch reviewed now, before we have the final certs ready.

This patch adds a test that verifies that an app actually signed by the real Mozilla marketplace succeeds. It is a WIP, and there are a few unresolved issues: 1. Apparently B2G builds don't run xpcshell tests, but this is an xpcshell test. 2. This test should pass only under B2G, but not on other platforms, yet there is no conditional logic for that. 3. There seems to be a problem with the signature of the test app and I'm waiting for that to get resolved.

(In reply to Brian Smith (:bsmith) from comment #69) > > Wouldn't it be nicer API to let this just return the complete line string? > > Yes, but note that the Manifest-Version and Signature-Version lines are not > allowed to be split. I'm just not very happy from pointer playing code around calls to this function, that are mostly duplicated and potentially inclining to contain mistakes. > > Indention > > I think this is OK as-is. There's not really a great way to indent it. My overlook here!

Comment on attachment 682520 [details] [diff] [review] Part 0.2: Force visibility(default) for NSS headers in ScopedNSSTypes.h Fixed "for real" in bug 813241. Thanks for your help, Mike and Ted.

> ** Why the bug should remain blocking-basecamp (layman's terms, especially if the summary isn't human readable) This is a critical feature for the Mozilla Marketplace. Many types of apps will not be possible to build for b2g without this security feature. > ** Level of effort and risk involved > ** ETA for resolution > ** Whether the work is blocked ETA is early next week, mostly due to anticipated delays due to the short work week and people not being available. We are testing the final changes and most changes have been posted for review and/or already reviewed already.

Comment on attachment 683369 [details] [diff] [review] Part 1.3: Address Honza's review comments for Part 1.2 [FOLD INTO Part 1.2] Review of attachment 683369 [details] [diff] [review]: ----------------------------------------------------------------- r=honzab, there is just one small bug in the code. ::: security/manager/ssl/src/JARSignatureVerification.cpp @@ +188,5 @@ > > return NS_OK; > } > > +// On input, nextLineStart is the start of the current line. On output, // On input, nextLineStart is position in the data this function reads line from. On output it will shift to the next line position. I still think |caret| is more proper name, but it is just a nit. @@ +192,5 @@ > +// On input, nextLineStart is the start of the current line. On output, > +// nextLineStart is the start of the next line. > +nsresult > +ReadLine(/*in/out*/ const char* & nextLineStart, /*out*/ nsCString & line, > + bool allowContinuations = true) Exactly my idea! @@ +259,5 @@ > + for (;;) { > + if (nameEnd == 0) { > + return NS_ERROR_SIGNED_JAR_MANIFEST_INVALID; // colon with no name > + } > + if (curLine[nameEnd - 1] != ' ') tabs? you don't decrease nameEnd (apparently a test is missing) @@ +268,5 @@ > + // Set attrValue to the value, skipping spaces between the colon and the > + // value. The value may be empty. > + int32_t valueStart = colonPos + 1; > + int32_t curLineLength = curLine.Length(); > + while (valueStart != curLineLength && curLine[valueStart] == ' ') { tabs? @@ +271,5 @@ > + int32_t curLineLength = curLine.Length(); > + while (valueStart != curLineLength && curLine[valueStart] == ' ') { > + ++valueStart; > + } > + curLine.Mid(attrValue, valueStart, curLineLength - valueStart); .Right(...) ? @@ +412,1 @@ > const char* curPos = nextLineStart; unused @@ +456,5 @@ > nsAutoCString attrName; > nsAutoCString attrValue; > + rv = ParseAttribute(curLine, attrName, attrValue); > + if (NS_FAILED(rv)) { > + return rv; You ignored errors here in the previous patch.

Comment on attachment 683376 [details] [diff] [review] Part 2.1: Generate test cases for signed app signature verification [v2] Review of attachment 683376 [details] [diff] [review]: ----------------------------------------------------------------- sr=honzab, but I just quickly looked at the patch

Been a week since any updates. What needs to happen here?

r=honzab, r=ehsan (In reply to Honza Bambas (:mayhemer) from comment #81) > > + if (curLine[nameEnd - 1] != ' ') > > tabs? In the interest of time, I did not add support for tabs since no tools that generate JARs (that we know of or care about) actually use them. > you don't decrease nameEnd (apparently a test is missing) I fixed this. You are right that we should have testing of the parser but I don't think we'll have time to do it. > > + rv = ParseAttribute(curLine, attrName, attrValue); > > + if (NS_FAILED(rv)) { > > + return rv; > > You ignored errors here in the previous patch. The incomplete parsing logic (including error checking) was inherited from nsJAR. It is better to do stricter error checking here. I addressed your other review comments. Thank you for the careful review.

Here is the final version of the patch, which embeds the final version of the dev server's app signing certificate. I verified that this certificate is able to verify the test app given to me by the marketplace team using this tryserver build, which contains an extra patch that forces MOZ_B2G_CERTDATA=1 for browser builds too (not just B2G): https://tbpl.mozilla.org/?tree=Try&rev=357b6ebd03be. (Without that extra patch, the test_signed_app-marketplace.js tests for non-B2G builds will rightly fail.) Bob was correct that we cannot use the active distrust flag on a cert for one particular usage; if you do, then the cert becomes distrusted for all usages. So, I took his advice and used the CKT_NSS_MUST_VERIFY_TRUST flag instead.

This is the test that verifies that an app signed on the marketplace staging server verifies correctly. See https://tbpl.mozilla.org/?tree=Try&rev=357b6ebd03be for the results for when we force MOZ_B2G_CERDATA=1. Today I will attach another patch that adds the conditional logic to test_signed_apps.js and test_signed_apps-marketplace.js that handles the fact that the verifications should succeed on B2G platforms but fail on non-B2G platforms.

(In reply to Brian Smith (:bsmith) from comment #86) > Today I will attach another patch that adds the conditional logic to > test_signed_apps.js and test_signed_apps-marketplace.js that handles the > fact that the verifications should succeed on B2G platforms but fail on > non-B2G platforms. Er, I mean test_signed_apps.js will pass on all platforms because it generates and signs its own apps using its own test cert. test_signed_apps-marketplace.js is the one that needs the additional logic.

Comment on attachment 686627 [details] [diff] [review] Part 3.2: Customize NSS root trust database for B2G r+ relyea

Comment on attachment 683376 [details] [diff] [review] Part 2.1: Generate test cases for signed app signature verification [v2] Review of attachment 683376 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/ssl/tests/unit/test_signed_apps/nss_ctypes.py @@ +3,5 @@ > + > +if os.name == 'posix': > + nspr = cdll.LoadLibrary("libnspr4.so") > + nss = cdll.LoadLibrary("libnss3.so") > + smime = cdll.LoadLibrary("libsmime3.so") For OS X a change is needed: import sys if os.name == 'posix': if sys.platform == 'darwin': nspr = cdll.LoadLibrary("libnspr4.dylib") nss = cdll.LoadLibrary("libnss3.dylib") smime = cdll.LoadLibrary("libsmime3.dylib") else: nspr = cdll.LoadLibrary("libnspr4.so") nss = cdll.LoadLibrary("libnss3.so") smime = cdll.LoadLibrary("libsmime3.so") @@ +9,5 @@ > + # assume windows > + nspr = cdll.LoadLibrary("nspr4.dll") > + nss = cdll.LoadLibrary("nss3.dll") > + smime = cdll.LoadLibrary("smime3.dll") > + *Very* minor nitpick: Trailing whitespace here and on a few other lines. ::: security/manager/ssl/tests/unit/test_signed_apps/sign_b2g_app.py @@ +36,5 @@ > + mf_entries = [] > + seen_entries = set() > + > + # Change the limits in JarSignatureVerification.cpp when you change the limits > + # here. Is it possible to extract these out of NSS via nss_ctypes.py? @@ +46,5 @@ > + max_sf_len = 1024 > + > + total_uncompressed_len = 0 > + entry_count = 0 > + with zipfile.ZipFile(out_zipfile_name, 'w') as out_zip: zipfile included with Python before 2.7 doesn't have context manager support. Might want to add something akin to https://github.com/mozilla/signing-clients/blob/master/signing_clients/apps.py#L24-L31 at the head of the file.

Comment on attachment 683376 [details] [diff] [review] Part 2.1: Generate test cases for signed app signature verification [v2] Review of attachment 683376 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/ssl/tests/unit/test_signed_apps/sign_b2g_app.py @@ +53,5 @@ > + name = entry_info.filename > + > + # Check for reserved and/or insane (potentially malicious) names > + if name.endswith("/"): > + "" You could use `pass` here. @@ +56,5 @@ > + if name.endswith("/"): > + "" > + # Do nothing; we don't copy directory entries since they are just a > + # waste of space. > + elif name.lower().startswith("meta-inf/"): I don't know if it matters here or not, but after bug 814131 there will be a META-INF/ file that isn't added by the signing mechanism. This is for the "ids.json" (or whatever we end up calling it) that will contain the app and version IDs.

(In reply to Ryan Tilder [:rtilder] from comment #89) > > + # Change the limits in JarSignatureVerification.cpp when you > > + # change the limits here. > > Is it possible to extract these out of NSS via nss_ctypes.py? When I wrote that comment I was feeling ambitious and I thought I'd make the same size checks within Gecko. However, I actually do not make those size checks within Gecko. Instead, we trust the marketplace to sign only reasonable things (e.g. by enforcing these limits in the server-side signing code). > I don't know if it matters here or not, but after bug 814131 there will be a > META-INF/ file that isn't added by the signing mechanism. This is for the > "ids.json" (or whatever we end up calling it) that will contain the app and > version IDs. When that work lands, I will modify sign_b2g_app.py to add the ids.json itself. sign_b2g_app.py is designed to be a model to check the server-side code against. I heard the server-side code is split into multiple parts, where one part does the sanity checking, another part calculates the digests, and another part does the signing. If I have time (not in this bug), I will refactor sign_b2g_app.py to do those three phases separately instead of all at once, so that there is more of a one-to-one correspondence between this testing code and the server-side code. (In fact, I'd really like it if we could eventually use the exact same code for both the server-side and the tests in the client side.)

I landed Part 1 and Part 2, but Part 3 and Part 4 haven't landed yet. I will update the checkin flags on the individual patches. All review comments were addressed. Thank you for the reviews! Note that I folded Part 1.* and Part 2.* together because Part 1 (the implementation) shouldn't land without Part 2 (the tests). https://hg.mozilla.org/integration/mozilla-inbound/rev/57473f3eecec https://hg.mozilla.org/releases/mozilla-aurora/rev/88424c956cfe https://hg.mozilla.org/releases/mozilla-beta/rev/a56a7d60657b

Fix build bustage due to missing "using namespace mozilla" on mozilla-beta: https://hg.mozilla.org/releases/mozilla-beta/rev/bcb3abaaa07b

WIP. Haven't been able to test this yet due to emulator and B2G desktop bustage.

This is the diff -w version of attachment 691719 [details] [diff] [review]. With this patch, I can successfully (modulo bugs not related to signing) install a privileged app from the dev. marketplace. In particular, https://marketplace-dev.allizom.org/app/privileged-app-test/. The actual test app (privileged-app-test) has the following bugs: 1. It's manifest wrongly requests "device-storage" instead of "device-storage:pictures." 2. The contacts API portion of the test app fails. 3. The device storage portion of the app doesn't give a visible indication of passing or failing. These issues with the test app should be resolved soon, because the test app is embedded in the Gecko test suite in part 3.3 of this patch series.

(In reply to Brian Smith (:bsmith) from comment #97) > The actual test app (privileged-app-test) has the following bugs: > > 1. It's manifest wrongly requests "device-storage" instead of > "device-storage:pictures." I could fix this quickly and post either an update to this app or upload it as a new app. > 2. The contacts API portion of the test app fails. > 3. The device storage portion of the app doesn't give a visible indication > of passing or failing. I'd be happy to try to fix these but might need help as I'm not familiar with these APIs.

Comment on attachment 691721 [details] [diff] [review] Part 4: Check signature during app installation, diff -w [NOT FOR CHECKIN] Review of attachment 691721 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/apps/src/Webapps.jsm @@ +1483,5 @@ > return; > } > > + let certdb = Cc["@mozilla.org/security/x509certdb;1"] > + .getService(Ci.nsIX509CertDB); Can this fail? nit: indentation @@ +1487,5 @@ > + .getService(Ci.nsIX509CertDB); > + certdb.openSignedJARFileAsync(zipFile, function(aRv, aZipReader) { > + try { > + var zipReader; > + var isSigned; There is not a single var in this file. Rewrite it with let? @@ +1492,5 @@ > + if (Components.isSuccessCode(aRv)) { > + isSigned = true; > + zipReader = aZipReader; > + } else if (aRv != Cr.NS_ERROR_SIGNED_JAR_NOT_SIGNED) { > + throw "INVALID_SIGNATURE"; Do we have to close aZipReader in the finally? @@ +1501,2 @@ > zipReader.open(zipFile); > + } nit: indentation @@ +1526,5 @@ > + let isDevMode = Services.prefs.getBoolPref("dom.mozApps.dev_mode"); > + let maxStatus = isDevMode ? Ci.nsIPrincipal.APP_STATUS_CERTIFIED > + : isSigned ? Ci.nsIPrincipal.APP_STATUS_PRIVILEGED > + : Ci.nsIPrincipal.APP_STATUS_INSTALLED; > + if (AppsUtils.getAppManifestStatus(aManifest) > maxStatus) { nit: indentation I think you should align the first ? and : @@ +1546,5 @@ > zipReader.close(); > } > }); > }); > + }); nit: indentation

(In reply to Brian Smith (:bsmith) from comment #97) > Created attachment 691721 [details] [diff] [review] > Part 4: Check signature during app installation, diff -w [NOT FOR CHECKIN] > > This is the diff -w version of attachment 691719 [details] [diff] [review]. > With this patch, I can successfully (modulo bugs not related to signing) > install a privileged app from the dev. marketplace. In particular, > https://marketplace-dev.allizom.org/app/privileged-app-test/. > > The actual test app (privileged-app-test) has the following bugs: > > 1. It's manifest wrongly requests "device-storage" instead of > "device-storage:pictures." Yeah, I realized that after the fact when our app manifest docs were out of date with the perms matrix. Should be a quick fix. > 2. The contacts API portion of the test app fails. That piece is strange. I was just going off of the example that was given for the contacts API. Will enable dev mode to formally try this out. > 3. The device storage portion of the app doesn't give a visible indication > of passing or failing. Well, right. Although the intention originally was to have a test app that would just access the API to verify the permission prompt appears. It wasn't really to actually verify something within the API (there's separate testing happening there). What's the intention here for having this? > > These issues with the test app should be resolved soon, because the test app > is embedded in the Gecko test suite in part 3.3 of this patch series. The first issue should be a quick type-o fix in the manifest. Second one should be quick too.

Ok ignore indentation comments. Haven't seen the diff -w

https://hg.mozilla.org/mozilla-central/rev/6a0ed6484811 https://hg.mozilla.org/mozilla-central/rev/b11065872128 This is the last set of patches for this bug. With these changes, you can now install privileged apps on the phone. More patches will follow in the follow-up bugs. In particular, the changes we should make to the test privilege app should be done in another bug now.

https://hg.mozilla.org/releases/mozilla-aurora/rev/5ac24a96e732 https://hg.mozilla.org/releases/mozilla-aurora/rev/dabb4fb1d101 https://hg.mozilla.org/releases/mozilla-b2g18/rev/cd54e668230f https://hg.mozilla.org/releases/mozilla-b2g18/rev/b617be3e9770

(In reply to Gregor Wagner [:gwagner] from comment #99) > > + let certdb = Cc["@mozilla.org/security/x509certdb;1"] > > + .getService(Ci.nsIX509CertDB); > > Can this fail? > nit: indentation See https://bugzilla.mozilla.org/attachment.cgi?id=692171, which I folded into the patch before I checked it in. > @@ +1492,5 @@ > > + if (Components.isSuccessCode(aRv)) { > > + isSigned = true; > > + zipReader = aZipReader; > > + } else if (aRv != Cr.NS_ERROR_SIGNED_JAR_NOT_SIGNED) { > > + throw "INVALID_SIGNATURE"; > > Do we have to close aZipReader in the finally? No. aZipReader will always be null unless Components.isSuccessCode(aRv) is true, and we close zipReader in finally after zipReader = aZipReader. > @@ +1526,5 @@ > > + let isDevMode = Services.prefs.getBoolPref("dom.mozApps.dev_mode"); > > + let maxStatus = isDevMode ? Ci.nsIPrincipal.APP_STATUS_CERTIFIED > > + : isSigned ? Ci.nsIPrincipal.APP_STATUS_PRIVILEGED > > + : Ci.nsIPrincipal.APP_STATUS_INSTALLED; > > + if (AppsUtils.getAppManifestStatus(aManifest) > maxStatus) { > > nit: indentation > I think you should align the first ? and : Why does nobody understand how beautiful this ternary operator indention style is? This style is used all the time in high-quality parsers because it lets you right many conditions without adding a layer of indention and without needing to resort to pesky loops. Seriously, though, I didn't make this change mostly because I think it is more readable the way I did it, but more importantly I was already at the 80-char limit. If you still disagree, let me know and I will touch it up.

> Why does nobody understand how beautiful this ternary operator indention > style is? This style is used all the time in high-quality parsers because > it lets you right many conditions without adding a layer of indention and > without needing to resort to pesky loops. I agree!! It's the exact same style as we're all using for if (A) { ... } else if (B) { ... } else if (C) { ... } else { ... }

Marking as verified as we've done a bunch of sanity tests to reveal that this indeed does work with marketplace dev with some of the major kinks knocked out. Obviously there's more testing that can happen, but marking as verified as the basic flow appears to be there.

(In reply to Honza Bambas from comment #64) > (From update of attachment 682522 [details] [diff] [review]) > > + rv = ParseManifestHeader(nextLineStart, nsLiteralCString(JAR_MF_HEADER)); ... > > + rv = zip->FindEntries(nsLiteralCString(""), getter_AddRefs(entries)); > NS_LITERAL_CSTRING ? NS_LITERAL_CSTRING would have been a good idea for JAR_MF_HEADER too, but for some reason the #define for it has a (const char *) cast...

(In reply to neil@parkwaycc.co.uk from comment #107) > (In reply to Honza Bambas from comment #64) > > (From update of attachment 682522 [details] [diff] [review]) > > > + rv = ParseManifestHeader(nextLineStart, nsLiteralCString(JAR_MF_HEADER)); > ... > > > + rv = zip->FindEntries(nsLiteralCString(""), getter_AddRefs(entries)); > > NS_LITERAL_CSTRING ? > > NS_LITERAL_CSTRING would have been a good idea for JAR_MF_HEADER too, but > for some reason the #define for it has a (const char *) cast... At least for nsLiteralCString("") use EmptyCString().