798415 - Add the ability to import signatures to a MAR file

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Brian R. Bondy [:bbondy]]

We need a way to import a specific signature to a MAR file.
This should include a way to specify the position of the signature to replace.
The imported signature should be from a file that contains a base64 encoded export.

The operation should only succeed if the new MAR file verifies.
That means it may be easiest to add the ability to verify a single signature on a MAR file, which doesn't verify the whole MAR with all signatures.

Comment 1

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - Import binary data

Comment 2

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - Import tests

Comment 3

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - Import implementation

Comment 4

[Brian R. Bondy [:bbondy]]

I made the import operation always create a new MAR to be symmetric with the other command line arguments.

I also didn't bother trying to be safe about the import because that would require extra command line args to be passed (the NSS config dir and the cert to use to verify).  And people who use the tool can just run a verify operation on the generated MAR file to verify that it's good.  If they happen to have the NSS config dir and cert name then you can use the verify operation.

Comment 5

[Brian R. Bondy [:bbondy]]

Attachment: Patch v2 - Import implementation

Rebased from libmar multi signing review comments, and carried forward similar comments.

Comment 6

[Brian R. Bondy [:bbondy]]

Attachment: Patch v2 - Import implementation

attached export patch by accident, fixed.

Comment 7

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 671443 [details] [diff] [review]
Patch v2 - Import implementation

Review of attachment 671443 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/libmar/sign/mar_sign.c
@@ +685,5 @@
> +  if (ReadAndWrite(fpSrc, fpDest, &signatureCount,
> +                   sizeof(signatureCount), "signature count")) {
> +    goto failure;
> +  }
> +  signatureCount = ntohl(signatureCount);

check signatureCount <= MAX_SIGNATURES.

Comment 8

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 670067 [details] [diff] [review]
Patch v1 - Import tests

Review of attachment 670067 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/libmar/tests/unit/test_sign_verify.js
@@ +84,5 @@
> +   * @param outMAR       The same as inMAR but with the specified signature
> +   *                     swapped at the specified index.
> +   * @param wantSuccess  True if a successful signmar return code is desired
> +  */
> +  function importMARSignature(inMAR, sigIndex,sigFile, outMAR, wantSuccess) {

whitespace.

Comment 9

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 670066 [details] [diff] [review]
Patch v1 - Import binary data

Review of attachment 670066 [details] [diff] [review]:
-----------------------------------------------------------------

I reviewed the test that uses this file.

Comment 10

[Brian R. Bondy [:bbondy]]

Comment on attachment 670067 [details] [diff] [review]
Patch v1 - Import tests

Review of attachment 670067 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/libmar/tests/unit/test_sign_verify.js
@@ +84,5 @@
> +   * @param outMAR       The same as inMAR but with the specified signature
> +   *                     swapped at the specified index.
> +   * @param wantSuccess  True if a successful signmar return code is desired
> +  */
> +  function importMARSignature(inMAR, sigIndex,sigFile, outMAR, wantSuccess) {

What's wrong with the whitespace?  Perhaps your window was too small and the text flowed to the next line?

Comment 11

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Brian R. Bondy [:bbondy] from comment #10)
> > +  function importMARSignature(inMAR, sigIndex,sigFile, outMAR, wantSuccess) {
> 
> What's wrong with the whitespace?  Perhaps your window was too small and the
> text flowed to the next line?

s/sigIndex,sigFile,/sigIndex, sigFile/

Comment 12

[Brian R. Bondy [:bbondy]]

Attachment: Patch v2 - Import tests

Small Windows fix, process.run throws on windows when the process returns a negative value. So added a try catch around it and set the return value to -1 in that case manually.

Also fixed whitespace.

Carrying forward r+.

Comment 13

[Brian R. Bondy [:bbondy]]

Attachment: Patch v3 - Import implementation

Added max signatures check and rebased.
Fixed a bug in parsing command line.
Fixed this bug:
-  passedInSignatureB64 = malloc(sizeOfBase64EncodedFile);
+  passedInSignatureB64 = malloc(sizeOfBase64EncodedFile + 1);
+  passedInSignatureB64[sizeOfBase64EncodedFile] = '\0';

Which caused the base64 decode to fail sometimes intermittently.

Carrying forward r+.

Comment 14

[Brian R. Bondy [:bbondy]]

https://hg.mozilla.org/mozilla-central/rev/51ff9244a6f8
https://hg.mozilla.org/mozilla-central/rev/31534cf5dd45
https://hg.mozilla.org/mozilla-central/rev/bd12f4180358

Comment 15

[(Out Sep 17-23) Ryan VanderMeulen]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e734b00ef7ba
https://hg.mozilla.org/releases/mozilla-aurora/rev/dbe5d8e1aaf8
https://hg.mozilla.org/releases/mozilla-aurora/rev/04635d817c40