978596 - Implement MAR hash verification with native APIs for OS X 10.7 and above

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Brian R. Bondy [:bbondy]]

This bug is to implement MAR hash verification using native APIs on OS X 10.7 and up using Security Transforms.

This is to avoid having to include libnss and its dependencies inside the udpater.app package. 

The goal is to make these tests work on OSX without using NSS for verifying (which it currently does):
http://dxr.mozilla.org/mozilla-central/source/modules/libmar/tests/unit/test_sign_verify.js

test_sign_verify.js is an xpcshell test that uses the signmar binary which is created from here:
http://dxr.mozilla.org/mozilla-central/source/modules/libmar/tool/mar.c

To create the signmar binary in your build, you must have this in your .mozconfig:
ac_add_options --enable-signmar

The xpcshell test will always use NSS to sign things, but will use Win32 native APIs on Windows to verify, and currently NSS on other platforms to verify. 

Once you implement the extra native verification, you'll need to add the needed linking here:
http://dxr.mozilla.org/mozilla-central/source/modules/libmar/tool/Makefile.in#33
and here:
http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/updater/Makefile.in#70

The goal is to fill in cryptox.h and cryptox.c with native OSX APIs, hopefully fitting into this abstraction here:
http://dxr.mozilla.org/mozilla-central/source/modules/libmar/verify/cryptox.h#52
http://dxr.mozilla.org/mozilla-central/source/modules/libmar/verify/cryptox.c
It currently selects at compile time whether to use Win32 native APIs if you're on Windows, or NSS otherwise. 

Once you adapt cryptox.h and cryptox.c then you should be done, and the xpcshell test should be verifying correctly. 
Feel free to push to the oak repository for testing if you want to test updates.
https://tbpl.mozilla.org/?tree=Oak

Comment 1

[Stephen A Pohl [:spohl]]

Attachment: Part 1: Verification via Security Transforms API

This passes all the tests in test_sign_verify.js and should be in sufficient shape for initial feedback. There are three things missing from this patch:
1. Ensure that this can build with the 10.6 SDK on the build machines. I'll address this in a second patch and upload it here.
2. Ensure that verification fails gracefully on OSX 10.6. Will address in the same patch as point 1 above and upload it here.
3. Currently, only signmar uses this new functionality (due to the way .der files are being read at the moment). I will file a separate bug for the updater binary to start using this functionality once bug 902761 is fixed.

(In reply to Brian R. Bondy [:bbondy] from comment #0)
> Once you implement the extra native verification, you'll need to add the
> needed linking here:
> http://dxr.mozilla.org/mozilla-central/source/modules/libmar/tool/Makefile.
> in#33
> and here:
> http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/updater/
> Makefile.in#70

Since frameworks on OSX are 'simply available' when supported by the version of OSX, I don't believe there is anything to add here.

Comment 2

[Brian R. Bondy [:bbondy]]

Comment on attachment 8398760 [details] [diff] [review]
Part 1: Verification via Security Transforms API

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

I think this should land first and independent of enabling it for use on updater by the way.

::: modules/libmar/tool/mar.c
@@ +117,5 @@
>    char *DERFilePaths[MAX_SIGNATURES];
> +#elif defined(XP_MACOSX) && !defined(MAR_NSS) && !defined(NO_SIGN_VERIFY)
> +  uint32_t fileSizes[MAX_SIGNATURES];
> +  uint32_t read;
> +  char* DERFilePaths[MAX_SIGNATURES];

I think these 2 blocks can be combined, with the exception of 'HANDLE certFile' and 'uint8_t *certBuffers'.  I.e. you can rename the DWORDs to uint32_t.

@@ +369,5 @@
> +      return -1;
> +    }
> +
> +    rv = mar_verify_signatures(argv[2], (const uint8_t* const*)DERFilePaths,
> +                               NULL, NULL, certCount);

The way that this works on Windows is that it loads the data from the DER file and then passes it to mar_verify_signatures.  I think this is fine though as long as it works. I think some param names should be renamed though to reflect what is being passed.

@@ +382,5 @@
> +        fprintf(stderr, "ERROR: The MAR file is in the old format so has"
> +                        " no signature to verify.\n");
> +      }
> +      return -1;
> +    }

Let's combine the 2 win/osx blocks since it's the same code here and with the if(certCount == 0) block.

Comment 3

[Stephen A Pohl [:spohl]]

Attachment: Part 1: Verification via Security Transforms API

This patch now compiles with the 10.6 SDK on the build machines and we gracefully fall back to NSS on OSX 10.6 for MAR verification. I've also addressed Brian's feedback, with the exception of:

(In reply to Brian R. Bondy [:bbondy] from comment #2)
> @@ +369,5 @@
> > +      return -1;
> > +    }
> > +
> > +    rv = mar_verify_signatures(argv[2], (const uint8_t* const*)DERFilePaths,
> > +                               NULL, NULL, certCount);
> 
> The way that this works on Windows is that it loads the data from the DER
> file and then passes it to mar_verify_signatures.  I think this is fine
> though as long as it works. I think some param names should be renamed
> though to reflect what is being passed.

Right, this difference does exist between the two platforms. The reason why I did this is because I preferred using OSX APIs to read the files, since it makes it easier to work with the rest of the native APIs. If I had chosen to mirror Windows' behavior, I would have had to add the ability to call into Obj-c code from mar.c to read the files with OSX APIs (currently only happening from mar_verify.c), or I would have had to use a platform independent way of reading the data. The argument name "certData" in mar_verify_signatures may not immediately reflect that we're passing "paths to", instead of "the contents of" certificates, but I thought it could be valid either way. Let me know if this isn't acceptable.


The functionality implemented here was tested in this try push, which should turn completely green shortly:
https://tbpl.mozilla.org/?tree=Try&rev=6272b74988fd

I cleaned up the patch a little bit after the try push above. The patch that I'm uploading here has no functional changes, but I submitted it to try again just to be safe. Should turn green in a couple of hours:
https://tbpl.mozilla.org/?tree=Try&rev=3ab3020927a7

Asking rstrong and smichaud for a review, as suggested by Brian. Carrying over Brian's f+.

Comment 4

[Stephen A Pohl [:spohl]]

Attachment: Diff between patches

This may be helpful to see the differences between the two patches. These changes were necessary to compile with the 10.6 SDK and to fall back to NSS on OSX 10.6.

Comment 5

[Stephen A Pohl [:spohl]]

Comment on attachment 8400797 [details] [diff] [review]
Part 1: Verification via Security Transforms API

Of course, something during cleanup broke Windows. Clearing r? requests for now.
https://tbpl.mozilla.org/?tree=Try&rev=3ab3020927a7

Comment 6

[Brian R. Bondy [:bbondy]]

Comment on attachment 8400797 [details] [diff] [review]
Part 1: Verification via Security Transforms API

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

r+ from me but I'd like rstrong and/or smichaud to go through it.

Comment 7

[Stephen A Pohl [:spohl]]

Attachment: Part 1: Verification via Security Transforms API

Windows is building again: https://tbpl.mozilla.org/?tree=Try&rev=71f579135f04

Comment 8

[Stephen A Pohl [:spohl]]

Attachment: Diff between patches

Updated diff.

Comment 9

[Robert Strong (they/them - no direct email)]

Comment on attachment 8400921 [details] [diff] [review]
Part 1: Verification via Security Transforms API

I didn't review the Mac parts and only have a couple of nits.

>diff --git a/modules/libmar/tool/mar.c b/modules/libmar/tool/mar.c
>--- a/modules/libmar/tool/mar.c
>+++ b/modules/libmar/tool/mar.c
>@@ -53,17 +53,25 @@ static void print_usage() {
>          "signed_input_archive.mar base_64_encoded_signature_file "
>          "changed_signed_output.mar\n");
>   printf("(i) is the index of the certificate to extract\n");
> #if defined(XP_WIN) && !defined(MAR_NSS)
>   printf("Verify a MAR file:\n");
>   printf("  mar [-C workingDir] -D DERFilePath -v signed_archive.mar\n");
>   printf("At most %d signature certificate DER files are specified by "
>          "-D0 DERFilePath1 -D1 DERFilePath2, ...\n", MAX_SIGNATURES);
>-#else 
>+#elif defined(XP_MACOSX)
>+  printf("Verify a MAR file:\n");
>+  printf("  mar [-C workingDir] -d NSSConfigDir -n certname "
>+    "-v signed_archive.mar -D DERFilePath\n");
nit: indent the same as elsewhere

>+  printf("At most %d signature certificate names are specified by "
>+         "-n0 certName -n1 certName2, ...\n", MAX_SIGNATURES);
>+  printf("At most %d signature certificate DER files are specified by "
>+         "-D0 DERFilePath1 -D1 DERFilePath2, ...\n", MAX_SIGNATURES);
>+#else
>   printf("Verify a MAR file:\n");
>   printf("  mar [-C workingDir] -d NSSConfigDir -n certname "
>     "-v signed_archive.mar\n");
nit: oh, copy / paste. fix this while you're here.

Comment 10

[Stephen A Pohl [:spohl]]

Attachment: Part 1: Verification via Security Transforms API

Thanks, Robert! Addressed feedback.

Comment 11

[Stephen A Pohl [:spohl]]

The easiest way to test the patch is by running the following command from the root source directory (after applying and building with the patch, of course):
./mach xpcshell-test modules/libmar/tests/unit/test_sign_verify.js

On 10.6, this will exercise the NSS route. On 10.7 and above, this will exercise the native Security Transforms route.

Comment 12

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8400991 [details] [diff] [review]
Part 1: Verification via Security Transforms API

This looks fine to me.

But yesterday I suggested that Stephen try getting rid of all uses of the autorelease pool.  This would simplify the code, and possibly avoid trouble.

Since Stephen was out today, I decided to try this myself.  In fact I was able to get rid of every instance of Objective-C syntax, and to change MacVerifyCrypto.mm to MacVerifyCrypto.cpp.  I'll post my patch in my next comment.

I tested both this patch and my alternative.  I didn't see any problems.

Note that you need to build with "ac_add_options --enable-signmar" in your mozconfig file.  It took me a while to discover this :-)

Comment 13

[Steven Michaud [:smichaud] (Retired)]

Attachment: Part1: Alternate version with MacVerifyCrypto.cpp

Comment 14

[Stephen A Pohl [:spohl]]

(In reply to Steven Michaud from comment #12)
> In fact I was able to get rid of every instance of Objective-C syntax,
> and to change MacVerifyCrypto.mm to MacVerifyCrypto.cpp.  I'll post my
> patch in my next comment.

Hah, sweet! Thanks for spending all this time on this!

> I tested both this patch and my alternative.  I didn't see any problems.

Very nice. I'll test your patch on Monday, but from looking at the code I think this will work just great!

> Note that you need to build with "ac_add_options --enable-signmar" in your
> mozconfig file.  It took me a while to discover this :-)

Sorry... :-( It was in the description of the bug, but I really should have repeated it in comment 11.

Comment 15

[Stephen A Pohl [:spohl]]

Attachment: Part 1: Verification via Security Transforms API

The alternate patch with MacVerifyCrypto.cpp worked great! This is the patch the way it landed on inbound this morning.

Landed on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a502db5137ab.

Try run was green:
https://tbpl.mozilla.org/?tree=Try&rev=3e84a8a4d616

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a502db5137ab