Build: Device: Unagi Build: B2G 18 2/14/2013 Reviewer Certs Included Steps: 1. Try installing https://marketplace.firefox.com/reviewers/apps/review/image-uploader or https://marketplace.firefox.com/reviewers/apps/review/filatours Expected: The app should install successfully. Actual: The app fails to install with INVALID_SIGNATURE. Additional Notes: The investigation I've done so far hasn't shown any obvious evidence that this is a client-side bug. Could be a signing bug on the marketplace side.

Mentioned this to Ryan in IRC a couple of days ago to see if he could investigate why this is failing. Have you found why this is failing?

ping?

Per my conversation with Jason on IRC last week, I can't access those Marketplace URLs since I am not a reviewer. He was going to send me the necessary data.

(In reply to Ryan Tilder [:rtilder] from comment #3) > Per my conversation with Jason on IRC last week, I can't access those > Marketplace URLs since I am not a reviewer. He was going to send me the > necessary data. I just emailed you the zip files from reviewer tools.

-> rtilder I think. Let me know if you need something

Is it possible for you to re-submit the packages for review and thereby have them re-signed, Jason? I'm curious to see if a newer signature attempt will have the same problem.

(In reply to Ryan Tilder [:rtilder] from comment #6) > Is it possible for you to re-submit the packages for review and thereby have > them re-signed, Jason? I'm curious to see if a newer signature attempt will > have the same problem. Redirecting to Lisa - can we try resubmitting these apps and seeing if we can install it after we resubmit?

Please check the expiration date of the certificates used for the signing. Over the weekend, the test certificates used for the Gecko unit tests expired because I put "expires in 90 days" in those certificates. I don't remember what expiration date we picked for the test certs used on the servers, but I wouldn't be surprised if there is a similar problem with them. Attach one of the signed apps to this bug if you are still having trouble after confirming the test certs aren't expired.

As asked by lisa, I've resubmitted both the applications.

Still unable to install. Alexandre, by chance did you delete the old versions and resubmit with the same version number? If so, we're also seeing Bug 843419, and you'll need to change the version number.

Yesterday, I've resubmitted as version 1.1. I did not deleted the old versions.

I've deleted old 1.0 that were marked as « Disabled by Mozilla ».

So we're stuck on resolution in this bug. bsmith - Can help shed some light on what you think we need to do here to diagnose and resolve this bug?

Working with rtilder on this bug, the signatures for packaged apps that work vs. not work don't have distinct differences.

Updated to new certs, still unable to install these apps.

I'm removing the dependency on the packaged apps bug and closing that bug out. Packaged apps are live, I'm treating this as a separate bug.

Camilo, can you take a look at this? Probably the fastest way to figure out what to do would be to replace the privileged-app-test-1.0.zip file with the image-uploader ZIP, make the marketplace reviewer root cert (root-ca-reviewers-marketplace.der in the github repo below) trusted, and then run the test_signed_apps-marketplace.js or similar test in a debugger to find out what the problem with the package is. In order to test it out end-to-end, you would have to use B2G Desktop (not Firefox) or a B2G device and do this craziness: https://github.com/briansmith/marketplace-certs And, I guess in order for cviecco to be able to test everything, he needs to have a reviewer account on marketplace.firefox.com so that these things can be accessible. https://marketplace.firefox.com/reviewers/apps/review/image-uploader or https://marketplace.firefox.com/reviewers/apps/review/filatours Ask cgalimidi who you need to ask for help for the marketplace.firefox.com account.

Nominating tef+ because this is a potential bug in the client that could affect app installation.

Bumping over to core under the assumption that this is now a client-side bug. Will bump back if it turns out this isn't right.

Over to Camilio to explore this a bit further. If it turns out that this can be worked around w/o a client side change, we wouldn't need to block on this, but until we know that, we'll block on this.

Camilo, send eviljeff (awilliamson@mozilla.com) the email address you're using for Marketplace and he can get you reviewer privs.

We want this so that we don't have to provide marketplace reviewers with a custom build. Marking as tef+ for now. Camilo - if simple fixes can be made to the submitted apps, please mark this as [NPOTB].

Got access to the marketplace. Now tring to solve.

Camilo, any estimation for this bug ?

Internal error-> NS_FILE_CORRUPTED. Besides those two apps, any other apps with the problem? ETA, tomorrow with p=.90 today: p=0.3

(In reply to Camilo Viecco (:cviecco) from comment #27) > Internal error-> NS_FILE_CORRUPTED. > > Besides those two apps, any other apps with the problem? IQ FitFun Lite is another app. I think I'm also hitting the problem with WP Photos now too.

The manifests are invalid. The maximum data lenght for each line cannot exceed 72 bytes, For the app the initiated the problem this is the problem line: Name: style/bb/menus-dialogs/valueselector/date/images/ui/shadow-invert.png http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/JARSignatureVerification.cpp#248

I'm assuming you are referring to the manifest.mf file in the META-INF directory with that line you are referring, right? Does that imply then this is no longer a client-side bug then, since we're following the spec here?

Correct it is still a bug, but not a client side bug.

Thanks Camilo. I think we've reached the conclusion here that this now back to being a signing bug. Sending this back over to marketplace to rtilder.

(In reply to Jason Smith [:jsmith] from comment #32) > Thanks Camilo. I think we've reached the conclusion here that this now back > to being a signing bug. Sending this back over to marketplace to rtilder. When I meant "signing bug," I meant a bug on the signing service on the marketplace side.

Ryan - Can you take a look?

Good work! See http://docs.oracle.com/javase/7/docs/technotes/guides/jar/jar.html "Line length: No line may be longer than 72 bytes (not characters), in its UTF8-encoded form. If a value would make the initial line longer than this, it should be continued on extra lines (each starting with a single SPACE)." For example: Name: style/bb/menus-dialogs/valueselector/date/images/ui/shadow-invert .png

(In reply to Camilo Viecco (:cviecco) from comment #27) > Internal error-> NS_FILE_CORRUPTED. Camilo, why did the openSignedJarFileAsync return NS_FILE_CORRUPTED instead of NS_ERROR_SIGNED_JAR_MANIFEST_INVALID? In ParseAttribute in JARSignatireVerification.cpp: if (len > 72) { // The spec says "No line may be longer than 72 bytes (not characters)" // in its UTF8-encoded form. This check also ensures that len < INT32_MAX, // which is required below. return NS_ERROR_SIGNED_JAR_MANIFEST_INVALID; } and that error code seems to be propogated all the way back. Is nsJAR.cpp also doing its own parsing/checking of the manifest? If so, then that might be a bug in the client. The intent is that the nsJAR.cpp code should ignore all the stuff in META-INF because I re-implemented all that code in JARSignatureVerification.cpp in a better way.

Was a bad test file during testing (Ctrl + C) during copy.

Talked with Ryan in IRC - he said he would take a look into this today.

Still no news, we are now nearly at 4 months since I submitted my apps ... and no idea of the issue ?

I did work on the application this weekend, tried to take a chance and submit a new version on the Marketplace ... After uploading this v1.2 (and removing any previous version), I'm able to download the manifest which is at https://marketplace.firefox.com/app/bf191091-abef-4443-ae1b-f955068d1875/manifest.webapp (this URL is listed in the edit listing page). However, this manifest is empty, and contains only "{}".

(In reply to Alexandre LISSY :gerard-majax from comment #40) > I did work on the application this weekend, tried to take a chance and > submit a new version on the Marketplace ... > > After uploading this v1.2 (and removing any previous version), I'm able to > download the manifest which is at > https://marketplace.firefox.com/app/bf191091-abef-4443-ae1b-f955068d1875/ > manifest.webapp (this URL is listed in the edit listing page). > > However, this manifest is empty, and contains only "{}". Further attemps to download the file results in either 404 or 500 errors.

Here is logcat matching this: I/Gecko ( 5684): -*-*- Webapps.jsm : Got http status=500 for https://marketplace.firefox.com/app/bf191091-abef-4443-ae1b-f955068d1875/manifest.webapp E/GeckoConsole( 5684): [JavaScript Error: "Error installing packaged app from: https://marketplace.firefox.com: MANIFEST_URL_ERROR" {file: "resource://gre/modules/Webapps.jsm" line: 1687}] E/GeckoConsole( 5982): Content JS LOG at https://marketplace.cdn.mozilla.net/media/js/mkt/consumer-min.js?build=b8f21b0:5 in anonymous: Error code: MANIFEST_URL_ERROR

(In reply to Alexandre LISSY :gerard-majax from comment #42) > Here is logcat matching this: > > I/Gecko ( 5684): -*-*- Webapps.jsm : Got http status=500 for > https://marketplace.firefox.com/app/bf191091-abef-4443-ae1b-f955068d1875/ > manifest.webapp > E/GeckoConsole( 5684): [JavaScript Error: "Error installing packaged app > from: https://marketplace.firefox.com: MANIFEST_URL_ERROR" {file: > "resource://gre/modules/Webapps.jsm" line: 1687}] > E/GeckoConsole( 5982): Content JS LOG at > https://marketplace.cdn.mozilla.net/media/js/mkt/consumer-min. > js?build=b8f21b0:5 in anonymous: Error code: MANIFEST_URL_ERROR You can't install a packaged app from the public listing page until its been approved. The bug is about being able to install via our reviewer tools before approval.

(In reply to Andrew Williamson [:eviljeff] from comment #43) > (In reply to Alexandre LISSY :gerard-majax from comment #42) > > Here is logcat matching this: > > > > I/Gecko ( 5684): -*-*- Webapps.jsm : Got http status=500 for > > https://marketplace.firefox.com/app/bf191091-abef-4443-ae1b-f955068d1875/ > > manifest.webapp > > E/GeckoConsole( 5684): [JavaScript Error: "Error installing packaged app > > from: https://marketplace.firefox.com: MANIFEST_URL_ERROR" {file: > > "resource://gre/modules/Webapps.jsm" line: 1687}] > > E/GeckoConsole( 5982): Content JS LOG at > > https://marketplace.cdn.mozilla.net/media/js/mkt/consumer-min. > > js?build=b8f21b0:5 in anonymous: Error code: MANIFEST_URL_ERROR > > You can't install a packaged app from the public listing page until its been > approved. The bug is about being able to install via our reviewer tools > before approval. I'm not talking about public listing, I'm talking about application for which I'm developper, thus I have access even if it's not yet approved.

You /are/ talking about the public listing page, even though the app isn't publicly installable yet. A packaged app is only signed on approval. The app isn't approved so isn't signed so isn't installable. This bug is about a separate file; only available from the reviewer pages, with an account with appropriate permissions; which is signed with a different certificate that isn't present on FirefoxOS by default. This /is/ available before approval.

(In reply to Andrew Williamson [:eviljeff] from comment #45) > You /are/ talking about the public listing page, even though the app isn't > publicly installable yet. A packaged app is only signed on approval. The > app isn't approved so isn't signed so isn't installable. > This bug is about a separate file; only available from the reviewer pages, > with an account with appropriate permissions; which is signed with a > different certificate that isn't present on FirefoxOS by default. This /is/ > available before approval. Then there is another bug here: that I'm being able to see the app with an « Install » button.

Please find attached a screenshot showing that I can actually install the app from my Nexus S, after adding the Marketplace account used on the device as a owner team member and accepting the developper agreement.

Patched and will work with ops to make sure it gets deployed as soon as possible. https://github.com/mozilla/signing-clients/pull/5

Just for information: I am the author of app "IQ Fit Fun Lite" and got my Peak today. I was able to install the app using simulator's new push feature. Macbook Retina -> FF 20.0 -> simulator 3.0pre8

(In reply to michbier from comment #49) > Just for information: > I am the author of app "IQ Fit Fun Lite" and got my Peak today. > I was able to install the app using simulator's new push feature. > Macbook Retina -> FF 20.0 -> simulator 3.0pre8 Seems consistent since the issue is with signing on marketplace.

(In reply to Ryan Tilder [:rtilder] from comment #48) > Patched and will work with ops to make sure it gets deployed as soon as > possible. https://github.com/mozilla/signing-clients/pull/5 I see this has been merged, any idea when it will be live ?

(In reply to Alexandre LISSY :gerard-majax from comment #50) > (In reply to michbier from comment #49) > > Just for information: > > I am the author of app "IQ Fit Fun Lite" and got my Peak today. > > I was able to install the app using simulator's new push feature. > > Macbook Retina -> FF 20.0 -> simulator 3.0pre8 > > Seems consistent since the issue is with signing on marketplace. Right. The push feature skips all the JAR integrity and signature checking, so signature-related errors will never be caught with it.

(In reply to Alexandre LISSY :gerard-majax from comment #51) > I see this has been merged, any idea when it will be live ? > It should be pushed to prod with tomorrow regularly scheduled release. At that point affected packages can be re-signed with the production key. --Ryan

I am a little bit confused, do the authors of the apps like mine (IQ FitFun) need to upload a new app version or just wait until approval will continue?

Wil - Should this be closed, given that a fix has been pushed and merged?

btw, I've just tried to install filatour, and IQ Fitfun Lite, and installing is failing for both of them.

(In reply to Andrew Williamson [:eviljeff] from comment #56) > btw, I've just tried to install filatour, and IQ Fitfun Lite, and installing > is failing for both of them. Well, I guess that answers my needinfo request. This isn't fixed.

I'm not sure if the reviewer cert signing is done on upload, on first use, or on-demand each time. If its anything other than on-demand something might need to be run to force re-signing on existing apps.

Okay, all the cached reviewer cert signed packaged apps have been cleared off the disk now so Marketplace should be signing with the new signing code. They're still failing though for me though. Ryan, do they work for you? (You now have reviewer access on Marketplace)

(In reply to Andrew Williamson [:eviljeff] from comment #58) > I'm not sure if the reviewer cert signing is done on upload, on first use, > or on-demand each time. If its anything other than on-demand something > might need to be run to force re-signing on existing apps. For the record, they are currently signed on demand and then saved on disk permanently. Another bug is filed to gc the cached versions on disk once a day.

The re-signed packages look correct for the 72 byte line length limit. I fear we're going to need Brian to see what the error being generated is now.

Camilo, you investigated this before. Do you remember anything else wrong other than the 72-byte line issue? Is there any way you can investigate this again?

Sure, but it will have to wait until tomorrow morning. Wait for more info.

it is possible to get a raw copy of the new app?

Camilo, what do you mean by "raw copy of the new app?" Btw, the same INVALID_SIGNATURE error is also showing in logcat when trying to install the packaged version of the preinstalled Notes app. https://marketplace.firefox.com/reviewers/apps/review/notes-packaged

FYI - with those meta bugs like bug 869212, I was only intending to track app-specific issues there, not other issues such as this bug.

Alexandre: can you attempt a possible workaround for us to get your app listed? If this bug is still caused by long file paths, shorter ones will not be affected. So can you rename some files/folders so no file path is longer than 67 characters?

IQ Fit Fun manifest looks like this: Any recommendation how to change it would be great: { "name": "IQ FitFunLite", "version": "1.2.6.1", "description": "Have fun with 43 free brain teasers for young and old (lite version).", "icons": { "16": "/IQFitFun/res/drawable/iqfitfun_lite_icon_round_16.png", "128": "/IQFitFun/res/drawable/iqfitfun_lite_icon_round_128.png" }, "developer": { "name": "DsignMatters, Michael Biermann", "url": "http://sites.google.com/site/dsignmatters/" }, "default_locale": "en", "launch_path": "/IQFitFun/index.html", "content_security_policy": "script-src 'self' 'unsafe-eval'; object-src 'self'", "installs_allowed_from": ["*"] }

(In reply to michbier from comment #68) > IQ Fit Fun manifest looks like this: > > Any recommendation how to change it would be great: There isn't any issue in the app manifest. Its the file paths - i.e. folders+filename. From a quick look I think its the filenames of the image assets e.g. IQFitFun/res/drawable/background_blackboard_desc_164x59_wo_shadow.png

Line size restrictions where being enforced the decoded line not on a per line basis. Patch coming soon.

(In reply to Camilo Viecco (:cviecco) from comment #70) > Line size restrictions where being enforced the decoded line not on a per > line basis. Patch coming soon. This needs a gecko patch I take it, right? If so, I'd close this bug for the work rtilder did and open a new bug in the core to track the gecko work. Nom that also for tef.

tef? since we now know there's a client change required here. Marketplace piece is finished per an earlier comment.

Comment on attachment 749140 [details] [diff] [review] move size check from parseline to readline Review of attachment 749140 [details] [diff] [review]: ----------------------------------------------------------------- Please add a test for this to test_signed_apps.js (*not* test_signed_apps-marketplace.js). It should be easy to modify the existing test case to include an entry with a path that causes the manifest entry to wrap. ::: security/manager/ssl/src/JARSignatureVerification.cpp @@ +197,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, size_t lineLimit = 72 ) Why is lineLimit a parameter? AFAICT, it should be a local static constant, or even just use a literal "72" below with a comment that says this restriction is from the spec. @@ +222,5 @@ > + } > + > + // We can probably be more strict as the spec says: "Implementations > + // should support 65535-byte (not character) header values..." > + if (currentLength > INT32_MAX) { whitespace around ">". Also, I suggest we compare against 65535. Even 65535 is ridiculously long for our needs. Also, IIRC, elsewhere in JarSignatureVerification.cpp we limit the total size of the manifest file to be fairly small to prevent OOM since we read the entire file into memory.

Comment on attachment 749566 [details] [diff] [review] move size check (v2.1) Review of attachment 749566 [details] [diff] [review]: ----------------------------------------------------------------- previous review comments addressed

tef+ since this is blocking marketplace testing.

Comment on attachment 749566 [details] [diff] [review] move size check (v2.1) Review of attachment 749566 [details] [diff] [review]: ----------------------------------------------------------------- The patch looks good. Please add the test. ::: security/manager/ssl/src/JARSignatureVerification.cpp @@ +215,5 @@ > + currentLength = line.Length(); > + > + // The spec says "No line may be longer than 72 bytes (not characters)" > + // in its UTF8-encoded form. > + const size_t lineLimit = 72; nit: make this static. @@ +726,5 @@ > NS_ENSURE_TRUE(signerCert, NS_ERROR_OUT_OF_MEMORY); > > signerCert.forget(aSignerCert); > } > + Undo this whitespace change.

push to try with tests before cleanup: https://tbpl.mozilla.org/?tree=Try&rev=fc00df66c7cb

now with tests!

Comment on attachment 750100 [details] [diff] [review] move size check (v3.1) Review of attachment 750100 [details] [diff] [review]: ----------------------------------------------------------------- There is no test that verifies that we reject a line longer than 72 bytes. However, that isn't the important thing here; I don't actually care whether we accept lines that are too long or not, as long as we correctly accept the continuation form of lines, which is what this new test *does* test. ::: security/manager/ssl/tests/unit/test_signed_apps/sign_b2g_app.py @@ +14,5 @@ > max_entry_count = 100 * 1000 > max_entry_filename_len = 1024 > max_mf_len = max_entry_count * 50 > max_sf_len = 1024 > Add a comment along the lines of "Lines in a JAR manifest file are required to be 72 bytes or less." @@ +18,5 @@ > > +def reformat_line_to_JAR_line(inline): > + newline = inline[0:72] > + leftover = inline[72:] > + while len(leftover) > 0 : s/ :/:/ Add a comment along the lines of "On continuation lines, the leading space allows only 71 bytes from the original line to be used." Nit: The words "newline" and "inline" both have completely different meanings from what you are using them here and those names don't match the naming convention used throughout the file. I suggest using "input_line" and "output_line".

comments addressed. keeping r+ from bsmith

v4 push to try https://tbpl.mozilla.org/?tree=Try&rev=53c8c83a85bc

(In reply to Camilo Viecco (:cviecco) from comment #86) > v4 push to try > > https://tbpl.mozilla.org/?tree=Try&rev=53c8c83a85bc Please land or add checkin-needed if the try results look good.

try failed on win32 due to filename length limitations. Since this only affects the test there will be a new bug on the tests and land the fix. ASAP try: https://tbpl.mozilla.org/?tree=Try&rev=c641101def5c

removal of tests so tha we can land this. keeping r+ from bsmith

(In reply to Camilo Viecco (:cviecco) from comment #89) > Created attachment 750813 [details] [diff] [review] > move size check (v5) > > removal of tests so tha we can land this. > keeping r+ from bsmith Are we good to land this now? If so I think we just need to request check-in

https://hg.mozilla.org/releases/mozilla-b2g18/rev/b87559cef261 https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/67ac7c9e74fb

So Andrew ended up testing this today on a 5/21 build and this isn't working for him. I'm following up with Rob & Ryan to find out why.

Looks like comment 93 is a false alarm. We didn't have the reviewer certs installed when we tested this. The two apps in question here can now be installed on 1.01. Marking as verified.

My application has now been validated :)

My app "IQ FitFun Lite" still pending review "Estimated waiting time: 1 working day", but nothing seems to happen for more than 40 days.

(In reply to michbier from comment #96) > My app "IQ FitFun Lite" still pending review "Estimated waiting time: 1 > working day", but nothing seems to happen for more than 40 days. My app "IQ FitFun Lite" has been approved today and is listed on the Firefox Marketplace. Many thx Michael

(In reply to michbier from comment #96) > My app "IQ FitFun Lite" still pending review "Estimated waiting time: 1 > working day", but nothing seems to happen for more than 40 days. bug 845263 was tracking that app - but it seems to have slipped through the cracks. I've reviewed it now.