Closed Bug 1466683 Opened 4 years ago Closed 4 years ago

autograph: debug "this addon appears to be corrupt" errors for addons with COSE signatures

Categories

(Cloud Services :: Security, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: u581815, Assigned: u581815)

References

Details

Attachments

(3 files, 8 obsolete files)

No description provided.
Attachments generated with https://github.com/mozilla-services/autograph/blob/30a60cb46504fbea2f588f07d93e18223c8924a7/tools/autograph-client/build_test_xpis.sh on 30a60cb4 of https://github.com/mozilla-services/autograph/pull/76 using the stage and prod autograph configs from an unsigned zip built from the tomato-clock source.

XPIs with only the PKCS7 signature install properly on new profiles (Fx Nightly 62.0a1 (2018-06-04) (64-bit) osx with xpinstall.signatures.dev-root=true (bool) for stage), but the XPIs with COSE signatures raise "This add-on could no be installed because it appears to be corrupt".

I think this is related to autograph not providing the correct cert chain.

franziskus: can you reproduce the corruption error? Also, we're reusing the same cert spec for PKCS7 and COSE EE certs, does the COSE verification run additional checks (e.g. on timestamps) on the certs?
Assignee: nobody → gguthe
Status: NEW → ASSIGNED
Flags: needinfo?(franziskuskiefer)
franziskus tracked this down:

> the only difference in the bytes Fx puts into the signature verification and autograph puts in is 0xf6 vs 0x40
> so it should work if you use `nil` here .... https://github.com/mozilla-services/go-cose/blob/master/sign_verify.go#L115

so autograph was signing an empty bstr for the external_aad data instead of nil
Flags: needinfo?(franziskuskiefer)
working stage webextension with cose and pkcs7 signatures
Attachment #8983198 - Attachment is obsolete: true
Attachment #8983199 - Attachment is obsolete: true
Attachment #8983200 - Attachment is obsolete: true
Attachment #8983201 - Attachment is obsolete: true
Attachment #8983203 - Attachment is obsolete: true
Attachment #8983204 - Attachment is obsolete: true
Attachment #8983206 - Attachment is obsolete: true
Attachment #8983207 - Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
The pk7 signature uses SHA1 in the attached zip. Is that intentional? Can we have one with SHA256 for unit tests in Firefox as well?
Flags: needinfo?(gguthe)
pk7 with sha256 digest and no COSE signatures
 
from https://github.com/mozilla-services/autograph/pull/76/commits/20772dcd013303014eb32b014879be0b43bc7e20

signed with the autograph stage config

Produces an "addon appears to be corrupt" error in Nightly 63.0a1 (2018-06-29) (64-bit) with the dev-root enabled.
pk7 with sha256 digest and an ES256 COSE signature
 
from https://github.com/mozilla-services/autograph/pull/76/commits/20772dcd013303014eb32b014879be0b43bc7e20

signed with the autograph stage config

Produces an "addon appears to be corrupt" error in Nightly 63.0a1 (2018-06-29) (64-bit) with the dev-root enabled.
(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #12)
> The pk7 signature uses SHA1 in the attached zip. Is that intentional? 

It is hardcoded here: https://github.com/mozilla-services/autograph/blob/06ac681623466de13809f08351d15428d9cabe96/signer/xpi/xpi.go#L198-L199

> Can we have one with SHA256 for unit tests in Firefox as well?

Yep, attached one without and one with a COSE signature.

I changed the digest in this commit: https://github.com/mozilla-services/autograph/pull/76/commits/b277d036d0e7bc35bccb042b8cb806c1fbbab1e0

It's entirely possible that the other refactoring in that PR is causing the errors though.


:franziskus Is changing the digest something autograph should support as an option? Or should we do something like only use SHA256 for the pk7 digest when a COSE signature is requested too?
Flags: needinfo?(gguthe)
I think we shold always use SHA256 (or better ;) for PK7 when doing COSE. There shouldn't be a reason to use SHA1.
I don't think that's right. If we force SHA256 for PKCS7 then we'll break older versions of Firefox that don't support COSE and only support SHA1.
SHA256 support for PK7 landed in 58 so if we only care about supported versions of Firefox that shouldn't be an issue. Not sure where you want to make the cut there.
For testing it would be great to have both.
> if we only care about supported versions of Firefox

This is a question for add-on product mgmt, but I'd suggest addressing it in a separate bug.

That said, adding a signing request option to sign with SHA256 instead of SHA1 is a good idea.
See Also: → 1473674
You need to log in before you can comment on or make changes to this bug.