Closed Bug 798415 Opened 7 years ago Closed 7 years ago

Add the ability to import signatures to a MAR file

Categories

(Toolkit :: Application Update, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19
blocking-basecamp +
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: bbondy, Assigned: bbondy)

References

Details

Attachments

(3 files, 4 obsolete files)

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.
Attachment #670066 - Flags: review?(bsmith)
Attached patch Patch v1 - Import tests (obsolete) — Splinter Review
Attachment #670067 - Flags: review?(bsmith)
Attached patch Patch v1 - Import implementation (obsolete) — Splinter Review
Attachment #670068 - Flags: review?(bsmith)
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.
Attached patch Patch v2 - Import implementation (obsolete) — Splinter Review
Rebased from libmar multi signing review comments, and carried forward similar comments.
Attachment #670068 - Attachment is obsolete: true
Attachment #670068 - Flags: review?(bsmith)
Attachment #671429 - Flags: review?(bsmith)
Attached patch Patch v2 - Import implementation (obsolete) — Splinter Review
attached export patch by accident, fixed.
Attachment #671429 - Attachment is obsolete: true
Attachment #671429 - Flags: review?(bsmith)
Attachment #671443 - Flags: review?(bsmith)
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.
Attachment #671443 - Flags: review?(bsmith) → review+
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.
Attachment #670067 - Flags: review?(bsmith) → review+
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.
Attachment #670066 - Flags: review?(bsmith) → review+
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?
(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/
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+.
Attachment #670067 - Attachment is obsolete: true
Attachment #671708 - Flags: review+
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+.
Attachment #671443 - Attachment is obsolete: true
Attachment #671710 - Flags: review+
You need to log in before you can comment on or make changes to this bug.