Closed Bug 699700 Opened 13 years ago Closed 12 years ago

Add support for signing and verifying MAR files in libmar and the mar program

Categories

(Toolkit :: Application Update, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla12
Tracking Status
firefox8 - wontfix
firefox9 - wontfix
firefox10 - wontfix
firefox11 - wontfix
firefox12 + fixed
firefox13 + fixed
firefox-esr10 --- wontfix

People

(Reporter: bbondy, Assigned: bbondy)

References

(Depends on 1 open bug, )

Details

(Whiteboard: [sg:critical][qa-] (with a privileged service) [sec-assigned:dveditz])

Attachments

(3 files, 28 obsolete files)

16.45 KB, text/plain
Details
6.51 KB, patch
briansmith
: review+
imelven
: feedback+
Details | Diff | Splinter Review
88.74 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
Mozilla Archive files should be signed to avoid malware from exploiting our update software.

Pre bug 481815: Any malware application run from an administrative user 
1. Non elevated process watches the directory C:\Users\bbondy\AppData\Local\Mozilla\Firefox\Nightly\updates\0
2. When files are copied there, and before the next firefox restart, the attacking app replaces the MAR file
3. At the next Firefox startup it will prompt that the Mozilla signed application updater.exe would like to run.
4. User clicks yes, and the malware MAR file is installed.

Post bug 481815: Any malware application runf rom an administrative or limited account user.
1. Non elevated process writes a work item file with a path to the bad MAR file.
2. At the next Firefox startup it will extract the bad MAR file.

Both are issues that should be fixed, and will be fixed by MAR file signing.  But after bug 481815 lands there will be a lot more eyes on the updater code looking for exploits.

The older updater.exe code that runs should ideally just ignore the signatures and apply the update as normal.
The newer updater.exe code that runs should apply updates only if the signatures/hashes match.

Brian smith mentioned:
> I believe we can use our existing CMS (Cryptographic Message Syntax) support and/or our existing JAR signature support do handle all of this. 

Chris AtLee mentioned:
> Create a new file inside the MAR called e.g. update.manifest.sums
> This will contain a list SHA-256/512 checksums of all the files mentioned by update.manifest and updatev2.manifest, 
> as well as the two manifest files themselves.
> A signature for this file will then be created as e.g. update.manifest.sums.sig. 
> The keys/algorithm used here are still TBD.
> The updater will be modified to contain the public key (and an alternate/backup key) and to 
> validate the signed manifest first before proceeding.
> We'll have to modify the build process to generate this new manifest and sign 
> it in a similar way to how we've been planning to sign our nightly builds.
OS: Windows 7 → All
Hardware: x86_64 → All
something to keep in mind is that infrasec is wanting to move to a HSM to manage our private keys, so any signing mechanism we choose would ideally be compatible with the HSM.
So, signing the plaintext signature file that catlee has suggested in comment 0 prevents us from using Authenticode, because it doesn't have the capability of signing non-PE file formats.

I have a solution which can potentially solve this problem.  Windows supports data DLLs, which are DLLs which only have resources but node code.  They are still valid PE file formats, so I was thinking that we may be able to create one such DLL, add a string resource which contains the signature as catlee has suggested, sign it using Authenticode, then use the Windows APIs to verify the signature on that file just like we do for updater.exe, and then verify the hashes of the rest of the files in the MAR against the hashes in the string data inside the DLL.

How does this sounds?
That sounds good to me and I've done setting icon resources in PE files in a previous life so I think it wouldn't be that hard.
this sounds good to me also - great idea ! bbondy has already written code to do the authenticode verification in the service.

it seems like it potentially wouldn't be that difficult to automate building this DLL as well (generating the list of hashes and then embedding it in the DLL).
just a note that of course, this only works for Windows but that's where the majority of users are currently and where we're shipping the new install helper service etc.
Though I like this I suspect that the security team will want a solution for other platforms as well. Since this will be done in the updater and not the service a single cross platform solution would be better but if that isn't expedient this method should be used for now. I am somewhat concerned in regards to this method and a possible future method that works across platforms in that there will likely need to be backwards compatibility on Windows to support both an authenticode solution and a cross platform solution.
Firstly, I managed to find the correct term for data DLLs: they're called resource-only DLLs. <http://msdn.microsoft.com/en-us/library/24b2tcy0%28v=vs.71%29.aspx>

(In reply to Ian Melven :imelven from comment #4)
> it seems like it potentially wouldn't be that difficult to automate building
> this DLL as well (generating the list of hashes and then embedding it in the
> DLL).

Well, that's a bit tricky since in order to build the DLL you still need to have the resource compiler and linker available, and I don't know if we have them on our signing machine.  But you can build the DLL using an empty string resource, and then use the UpdateResource API and friends to write a small application which is capable of changing the string resource in the DLL, and run that on the signing machine to update the DLL to get the new string resource into it.
> ...then use the UpdateResource API and friends...

Right if we were going the DLL method that's what I would have done.
I spoke with rs and we think that we should do it right and do it once so the solution is cross platform.
This will avoid us having to revisit the issue later.

Initially looking at the MAR file it seems that we have an opening that will keep the MAR file format the same.
The MAR file format looks like this:

4 bytes : "MAR1"
4 bytes : offset to INDEX in bytes (big endian) relative to start of file
[***currently no data but an opportunity to put extra data***]
[index which is a 4 byte index-entry length]
[index-entries]

So the MAR reader will skip to the index after reading the first 8 bytes to do the rest of the processing.  
That means we can use the space after the first 8 bytes to put any extra binary data we want (the signed hash file).

The one question I have though is what is the benefit of having a per file hash instead of a whole archive hash.  The whole archive hash would start at the location of the index and continue to the end of the file.  The code would be much simpler and cleaner to just do a whole archive hash and I think would be equally as secure.
The one possible benefit is that each file would be verified after it is either added or patched. The value of this I personally this is extremely small since there have been no reported issues where this has been a problem.
(In reply to Brian R. Bondy [:bbondy] from comment #0)
> Chris AtLee mentioned:
> > Create a new file inside the MAR called e.g. update.manifest.sums
> > This will contain a list SHA-256/512 checksums of all the files mentioned by update.manifest and updatev2.manifest, 
> > as well as the two manifest files themselves.

It would be better to create a single hash over the entire contents of the MAR file than the signature. That way, you ensure that you are covering all the metadata in the MAR file as well.

> The whole archive hash would start at the location of the index
> and continue to the end of the file.  The code would be much simpler
> and cleaner to just do a whole archive hash and I think would be
> equally as secure.

I agree with you, except that the hash should include all the data other than the signature. That is, it should include the "MAR1" header word and the offset to INDEX word too.

The fixed key and fixed backup key approach sounds reasonable to me, especially considering the cross-platform requirement. Do you have NSS available in the updater process? Which APIs are you going to use to do the hashing and verification?
> it should include the "MAR1" header word and the offset to INDEX word too.

Great, agree that is better.

> Do you have NSS available in the updater process? 

I'm not sure yet, I will verify after I verify that my assumptions about the existing MAR file reader code is correct.

> Which APIs are you going to use to do the hashing and verification?

If you know of any off hand that would save me a lot of time.
(In reply to Brian R. Bondy [:bbondy] from comment #11)
> > Do you have NSS available in the updater process? 
> 
> I'm not sure yet, I will verify after I verify that my assumptions about the
> existing MAR file reader code is correct.

rs can comment more precisely about this, but currently the updater doesn't link against any of our other libraries, NSS included.  Which sort of makes sense, if we wanted to ship an update which fixes a crash that happens as soon as the NSS shared library is loaded, for example.
I was a bit off on the file format, but the same idea will work.


Corrected file format of MARs:
4 bytes : "MAR1"
4 bytes : offset to INDEX in bytes (big endian) relative to start of file
[file 1 data]
[file 2 data]
...
[file N data]
[INDEX - 4 byte number of index entries]
[index-entry 1]
[index-entry 2]
...
[index-entry N]

We could stick the constant sized hash in anywhere but to keep things simple we can just put it before [file 1 data] right after the first 8 bytes as originally planned.
(In reply to Brian R. Bondy [:bbondy] from comment #13)
> 
> We could stick the constant sized hash in anywhere but to keep things simple
> we can just put it before [file 1 data] right after the first 8 bytes as
> originally planned.

I agree that we should be able to do this with the existing MAR format. I would suggest that we include an indicator of what type of hash is used. e.g.

"SHA1" + 20 byte hash
"SHA512" + 64 byte hash

Then we can add as many or as few hashes as we like before the file data begins.
(In reply to Chris AtLee [:catlee] from comment #14)
> I agree that we should be able to do this with the existing MAR format. I
> would suggest that we include an indicator of what type of hash is used. e.g.
> 
> "SHA1" + 20 byte hash
> "SHA512" + 64 byte hash
> 
> Then we can add as many or as few hashes as we like before the file data
> begins.

I suggest that we just include a version number here, and infer the type of signature (*not* hash, by the way) from the version number.
Blocks: 701087
Whiteboard: [sg:critical] (with a privileged service)
This is just an intermediate version of the patch.  
It does NOT do sign/verify of the hash yet.

This intermediate version of the patch just creates a backwards compatible MAR file with an embedded hash of all bytes of the file.  Extracting still works without verifying the hash since it is backwards compatible.   Please ignore the Makefile changes in the patch, that will need to be fixed up before this is done.

Future versions of this patch:
- The next verison of the patch will also verify the hash of all bytes of the file before extracting (probably today). 
- The third version of the patch will sign / verify the hash that is being created / verified.

The hash is calculated by including all bytes of the MAR file excluding the hash itself, in a consistent and deterministic order:

> Hash = SHA512([Bytes 0-4 which is MAR1][Bytes 8-72 (72=8+512bits/8)][Bytes 4-8])

I think for versioning of the archive itself, we will use the 4th byte, but I will circle back on that once the rest of the work is done.
Attachment #574957 - Attachment description: Intermediate patch for MAR file signing. Details in → Intermediate patch for MAR file signing. Details in Comment 16
Sorry the hash line above is wrong since bytes 8-72 is the hash itself which is excluded. It should read:

> Hash = SHA512([Bytes 0-4 which is MAR1][Bytes 72-End of file (72=8+512bits/8)][Bytes 4-8])
Uses openssl cmdline tools for signing / verification.

example usage:
mar.py -c test.mar -k rsa.key update/
mar.py -t test.mar -k rsa.key.pub

rsa.key should be a 4096 bit key.
- Added code for extracting the stored hash from an existing MAR file
- Added code for re-calculating the stored hash from an existing MAR file
- Added a new -v command line to the mar binary which verifies the embedded hash and returns 0 if the embedded hash is correct
- Added code to disallow MAR extracts with the mar binary via command line -x if the embedded hash is not valid 

Note: I don't do encrypting / decrypting yet of the hash, that is coming in a future patch.
Attachment #574957 - Attachment is obsolete: true
Brian, does this fall under a particular Silent Update feature or is it more general in terms of Updates? Also, what is the anticipated role for QA on this bug?
Anthony thanks for checking.  Once ready, this will need to be tested on all platforms (unlike bug 481815 even linux / mac), but not all UAC levels on Windows like bug 481815 needs.   Incremental and full updates should also be tested. RelEng might have more ideas for testing.
You should not use FreeBL directly. The FreeBL API is not intended for applications to use. It is only intended for use by Softoken, and I think there are some assumptions based on that. For example, in Softoken, we don't (IIRC) call any FreeBL function directly; instead, we only call them through a table of pointers to the functions. You can see parts of this strangeness in freebl/ldvector.c and freebl/loader.c.

The PK11_* (pk11wrap) and SEC* APIs (libnss) are the NSS APIs supported for use by applications. I think rstrong said that we do not want to rely on NSS, because that would prevent us from updating in certain situations (e.g. crashing bugs in NSS initialization, of which there was one recently). I guess this may be the reason you are trying to use FreeBL directly, to minimize the dependency.

Based on the limited information I have now, I recommend that instead you should use the Windows CAPI (CryptoAPI) hash/signature functions on Windows. I can help you with this. And, if possible, punt MAR signature verification for other platforms into a separate bug with lower priority. After all, we are doing this as a P1/critical bug only to support the Windows service.

Presumably, we could use (different) OS-provided APIs on Mac too. (Hopefully in another bug.)

On Linux, we cannot rely on their being any system-level crypto package when we aren't using system NSS. (Do we even do MAR-style updates in configurations that use --use-system-nss?) That means probably using NSS to verify the signatures. In that case, we should (probably) use VFY_Verify*. Note that these functions usually do the hashing for you, so there is no reason to do any hashing manually. (In fact--and I have to verify this--depending on what signature algorithm you use, you don't even need to *store* the hash separately from the signature.)
s/That means probably using NSS to verify the signatures/That means using the copy of NSS that Firefox bundles to verify the signatures/
If we aren't going to use NSS on all platforms and instead use platform specific api's then I am very tempted to say we will not use NSS on Linux. Also, if we are going to use NSS on Linux then we should use it on all platforms.

Brian, if you are ok with using NSS on Linux why shouldn't it be used on all platforms? Would a unit test be enough to have assurance that there isn't a crasher bug for the updater's usage?
(In reply to Robert Strong [:rstrong] (do not email) from comment #24)
> Brian, if you are ok with using NSS on Linux why shouldn't it be used on all
> platforms?

There is a lot of code that has to execute to initialize NSS (NSS_Init*) as a prerequisite to calling any of the signature verification functions. IIRC, either you or someone else pointed out that there is a real risk that some of this code could be buggy and we could get into a situation where NSS itself could cause us to fail to update. Also, eventually on non-Linux systems we are supposed to fold all of NSS into libxul. That would mean that updater.exe would have to link against libxul too. I think that is a real possibility.

> Would a unit test be enough to have assurance that there isn't a
> crasher bug for the updater's usage?

There are no other real choices on Linux, AFAICT, so we have to use NSS. If Linux had a built-in signature verification API, I would recommend to use it.
bsmith, Thanks for the feedback.

I thought of CryptoAPI as well since I use it already in the service, but we want to do a cross platform solution to avoid future work and refactoring. I don't think rstrong is completely against using NSS, but at least partially yes :).

I'm mostly concerned initially with just getting it working then we can swap in whatever library we need to.  Can we use OpenSSL? I would imagine you can do SHA512 hashes and RSA cyrptography there as well?

If we can't agree on anything I'll do something like make a function to check if archive signing should be done, and only return true from within Windows compiled builds.  Then make some stubs for the functions I need and implement them only with Windows CrytpoAPI code for now. That will at least minimize future work and keep all future logic the same as current logic.

> After all, we are doing this as a P1/critical bug only to support the Windows service.

Right but I think it is sitll a pretty big security bug even without the service, and probably with other OS.
On a side note the 'mar' program is compiled with HOST_ prefixes so for cross compiles it needs to generate a host binary and not a target machine binary.  I'm not sure if this will cause a lot of build problems or not w/ NSS. I'm guessing yes.
(In reply to Brian Smith (:bsmith) from comment #25)
> I think that is a real possibility.

I mean I think that bugs (crashing bugs, even) in NSS_Init* are a real possibility. We have fixed at least a couple in the last year and one in the last NSS release. (I think that crash was caused by Adobe Flash though, so less relevant here.)

The other concern is mismatched DLLs (e.g. NSS DLLs getting replaced for whatever reason with a version not compatible with updater.exe, or the same thing happening to libxul when we switch to libxul). I think it would be best to avoid as much as possible dependencies on non-system DLLs in the updater.

Also, I know on Windows there are problems with replacing/deleting DLLs immediately after having loaded/executed them. (e.g. anti-virus software, backup software.) I guess we must have figured out how to avoid these problems when updating Firefox. But--again I barely know anything about what I am talking about here--this may require use to dynamically load the NSS DLLs into the updater.exe process and dynamically unload them before replacing them, without killing updater.exe. This is quite different than how we replace the NSS DLLs after Firefox uses them (because the Firefox process exits completely). This issue on its own seems like unnecessary risk to me.

> > Would a unit test be enough to have assurance that there isn't a
> > crasher bug for the updater's usage?

In NSS, we have a whole test suite that calls NSS_Init* over 2500 times and we still have crashes in NSS_Init*. Definitely we should have tests for this code, but IMO having tests doesn't reduce the risk enough.
What about using a utility instead of an API that exists on the platform or that we can ship?
(In reply to Brian Smith (:bsmith) from comment #28)
>...
> Also, I know on Windows there are problems with replacing/deleting DLLs
> immediately after having loaded/executed them. (e.g. anti-virus software,
> backup software.) I guess we must have figured out how to avoid these
> problems when updating Firefox. But--again I barely know anything about what
> I am talking about here--this may require use to dynamically load the NSS
> DLLs into the updater.exe process and dynamically unload them before
> replacing them, without killing updater.exe. This is quite different than
> how we replace the NSS DLLs after Firefox uses them (because the Firefox
> process exits completely). This issue on its own seems like unnecessary risk
> to me.
It would be a copy located alongside the updater outside of the install dir. This doesn't mitigate the remaining issues but this specific issue is a non-issue.
Another option is only using the service for the case where a user is already able to update since this is already an issue for these users which will give us more time to come up with a solution.

Brian, could you run this by some people on the security team? Thanks
Brian Smith that is
would making copies of only what I need from NSS be a better approach?
(In reply to Brian R. Bondy [:bbondy] from comment #26)
> bsmith, Thanks for the feedback.
> 
> I'm mostly concerned initially with just getting it working then we can swap
> in whatever library we need to.  Can we use OpenSSL? I would imagine you can
> do SHA512 hashes and RSA cyrptography there as well?

1. It is probably better to use SHA-384 instead of SHA-512. I will explain this outside the bug.

2. OpenSSL is a huge dependency (in terms on complexity, if not size). I would rather not have to deal with it. If we *really* need to statically link a RSA+SHA verification function into updater.exe, then I can work with the NSS team to do that, and that would be much less complexity long-term.

> If we can't agree on anything I'll do something like make a function to
> check if archive signing should be done, and only return true from within
> Windows compiled builds.  Then make some stubs for the functions I need and
> implement them only with Windows CrytpoAPI code for now. That will at least
> minimize future work and keep all future logic the same as current logic.

We do not need a runtime API to determine what platform we are running on; #ifdef and friends will do this. If the code blocks are not too long, I would rather keep all the signature verification code together in one file with platform #ifdefs so that we can more easily make sure that changes get made to all implementations together. This would be easier for me to review, at least.

> Right but I think it is sitll a pretty big security bug even without the
> service, and probably with other OS.

I agree. But, I think we should do things optimally on Windows, and then take the time to do them as good as we can on other platforms later, rather than making the Windows version higher risk just so it can be the same as the other platforms ASAP.
> Another option is only using the service for the case where a user 
> is already able to update since this is already an issue for these 
> users which will give us more time to come up with a solution.

That might be a good approach for the short term.  I like it but not sure about security :)

I think most of the security team invovled are CC'ed in this bug so I'll ask here.

If we use the service only from admin accounts initially and not limited user accounts, is this bug still a blocker for the service to land on things other than Nightly? I think it would make the service and non service way equally as risky security bugs.

> We do not need a runtime API to determine what platform we are running 
> on; #ifdef and friends will do this

That's what I was suggesting, I probably didn't explain myself clear enough.
(In reply to Brian R. Bondy [:bbondy] from comment #35)
> If we use the service only from admin accounts initially and not limited
> user accounts, is this bug still a blocker for the service to land on things
> other than Nightly?

I have not been keeping up. Are you saying that the service would not run under LocalSystem or an admin account now?

By the way, Windows CryptoAPI might not be able to do SHA-2 signatures on Windows XP SP2 or lower. I think that we will (a) have to require XP SP3 or later for the service, and (b) skip the signature verification on SP2 or earlier in the non-service use of updater.exe.
(In reply to Brian Smith (:bsmith) from comment #36)
> (In reply to Brian R. Bondy [:bbondy] from comment #35)
> > If we use the service only from admin accounts initially and not limited
> > user accounts, is this bug still a blocker for the service to land on things
> > other than Nightly?
> 
> I have not been keeping up. Are you saying that the service would not run
> under LocalSystem or an admin account now?
The service would only be used by users running as admin since they already have this issue.

> By the way, Windows CryptoAPI might not be able to do SHA-2 signatures on
> Windows XP SP2 or lower. I think that we will (a) have to require XP SP3 or
> later for the service, and (b) skip the signature verification on SP2 or
> earlier in the non-service use of updater.exe.
I had a mid-air I didn't notice saying the same thing

iirc SHA-2 support doesn't exist in XP SP2 and below and this appears to confirm it.
http://en.wikipedia.org/wiki/SHA-2
We could include both a SHA-1 and a SHA-2 signature
So it sounds like we are leaning towards the CryptoAPI for this task.
Would an MD5 work for all versions and is it long enough to be secure?
(In reply to Chris AtLee [:catlee] from comment #38)
> We could include both a SHA-1 and a SHA-2 signature

(In reply to Brian R. Bondy [:bbondy] from comment #39)
> Would an MD5 work for all versions and is it long enough to be secure?

SHA1 is probably acceptable; After all, isn't the Authenticode certificate signed by the CA using RSA-2048+SHA1? If the attacker can break SHA1, it is already game-over, right? (MD5 is basically nearly completely broken now.)
(In reply to Brian Smith (:bsmith) from comment #40)
> (In reply to Chris AtLee [:catlee] from comment #38)
> > We could include both a SHA-1 and a SHA-2 signature
> 
> (In reply to Brian R. Bondy [:bbondy] from comment #39)
> > Would an MD5 work for all versions and is it long enough to be secure?
> 
> SHA1 is probably acceptable; After all, isn't the Authenticode certificate
> signed by the CA using RSA-2048+SHA1? If the attacker can break SHA1, it is
> already game-over, right? (MD5 is basically nearly completely broken now.)

Sure, but why not future proof ourselves? We include both signatures, and use the strongest one the platform supports.
(In reply to Chris AtLee [:catlee] from comment #41)
> Sure, but why not future proof ourselves? We include both signatures, and
> use the strongest one the platform supports.

If we have time to do so, then that is reasonable. But, it's more code to write tests for. It seems like we want to get this done ASAP. We are going to be better off with more automated testing of less code than with stronger signatures.
(In reply to Brian Smith (:bsmith) from comment #34)
> (In reply to Brian R. Bondy [:bbondy] from comment #26)
> > Right but I think it is sitll a pretty big security bug even without the
> > service, and probably with other OS.
> 
> I agree. But, I think we should do things optimally on Windows, and then
> take the time to do them as good as we can on other platforms later, rather
> than making the Windows version higher risk just so it can be the same as
> the other platforms ASAP.

+1. We need to stay focused on the immediate goal of delivering the Windows UAC service. We need a good solution but the initial focus is a fix for Windows.
(In reply to Lawrence Mandel [:lmandel] from comment #43)
> +1. We need to stay focused on the immediate goal of delivering the Windows
> UAC service. We need a good solution but the initial focus is a fix for
> Windows.

That's not to say that we should shortchange the other platforms. We can deal with them afterwards if that simplifies things at this point.
ok if there are no objections here is my plan:
- Use CryptoAPI for the hash and only calculate it on Windows.
- Use SHA1 for the hash
- Use CryptoAPI to sign the hash 
- Post a follow up bug to do signing on other plantforms which is a lower priority.

I should have most of this done by end of day tomorrow.
(In reply to Brian R. Bondy [:bbondy] from comment #45)
> ok if there are no objections here is my plan:
> - Use CryptoAPI for the hash and only calculate it on Windows.
> - Use SHA1 for the hash
> - Use CryptoAPI to sign the hash 
> - Post a follow up bug to do signing on other plantforms which is a lower
> priority.
> 
> I should have most of this done by end of day tomorrow.

Signing is going to be done on a separate linux signing machine, not the windows build machine. We don't have access to CryptoAPI there obviously.
(In reply to Chris AtLee [:catlee] from comment #46)
> (In reply to Brian R. Bondy [:bbondy] from comment #45)
> > - Use CryptoAPI to sign the hash 
> 
> Signing is going to be done on a separate linux signing machine, not the
> windows build machine. We don't have access to CryptoAPI there obviously.

s/sign/verify/. I am sure that is what he meant.
> Signing is going to be done on a separate linux signing machine, not the windows > build machine. We don't have access to CryptoAPI there obviously.

The 2 step process wouldn't be a problem but I think there will be a problem with CryptoAPI using a key it doesn't generate w/ CryptGenKey.  You can export keys with CryptExportKey to a file but it's stored in a strange looking file format.  I think that CryptImportKey only accepts keys that are generated with CryptGenKey.

Chris it seems that you want the mar program to use NSS and the updater to use CryptoAPI?

> Signing is going to be done on a separate linux signing machine

Can it be done on a separate Windows signing machine?
(In reply to Brian R. Bondy [:bbondy] from comment #48)
> > Signing is going to be done on a separate linux signing machine, not the windows > build machine. We don't have access to CryptoAPI there obviously.
> 
> The 2 step process wouldn't be a problem but I think there will be a problem
> with CryptoAPI using a key it doesn't generate w/ CryptGenKey.  You can
> export keys with CryptExportKey to a file but it's stored in a strange
> looking file format.  I think that CryptImportKey only accepts keys that are
> generated with CryptGenKey.

I've found a few references on how to get keys in and out of CryptoAPI that may be relevant.

http://pumka.net/2009/12/19/reading-writing-and-converting-rsa-keys-in-pem-der-publickeyblob-and-privatekeyblob-formats/

http://etutorials.org/Programming/secure+programming/Chapter+5.+Symmetric+Encryption/5.26+Creating+a+CryptoAPI+Key+Object+from+Raw+Key+Data/

http://etutorials.org/Programming/secure+programming/Chapter+5.+Symmetric+Encryption/5.27+Extracting+Raw+Key+Data+from+a+CryptoAPI+Key+Object/

> Chris it seems that you want the mar program to use NSS and the updater to
> use CryptoAPI?

I have no preference about implementation other than I want to be able to generate the signature for a mar file on linux. If we statically link NSS into the mar program, usage is pretty straightforward from our point of view. Signing of mars doesn't necessarily require the mar command, especially since the assembly of mar happens on a different machine than the signing does.

My plan was to have the build machines generate the mar file as usual, and then send it to the signing server. The signing server could generate and return just the signature, and the build machine would then update the mar file; or the signing server would return the entire signed mar file. Adding an option to the mar program to sign an existing mar file would be very handy.

> > Signing is going to be done on a separate linux signing machine
> 
> Can it be done on a separate Windows signing machine?

Possible, but we've been working under the assumption we would be doing signing on linux and so all of our code and testing so far has been based on that.
Maybe you can use your python program you attached earlier to do the signing? I'll still provide the ability to sign/verify as well but on the linux machine you could use the python program?
Thanks for the links by the way I'll look into them tomorrow.
(In reply to Brian R. Bondy [:bbondy] from comment #48)
> The 2 step process wouldn't be a problem but I think there will be a problem
> with CryptoAPI using a key it doesn't generate w/ CryptGenKey.  You can
> export keys with CryptExportKey to a file but it's stored in a strange
> looking file format.  I think that CryptImportKey only accepts keys that are
> generated with CryptGenKey.

This is because the standard packaging format of a public key is an X.509 certificate. Typically, an application asks CryptoAPI to get the public key out of a certificate and then operates on it, like this (from [1]):

        // Get the public key from the certificate
	CryptImportPublicKeyInfo(
		hCryptProv, 
		MY_TYPE,
		&pSignerCert->pCertInfo->SubjectPublicKeyInfo,
		&hPubKey
	);
	CheckError(bResult, L"CryptImportPublicKeyInfo............ ");

	// Verify the signature
	bResult = CryptVerifySignature(
		hHash, 
		pbBinary, 
		cbBinary, 
		hPubKey,
		NULL, 
		0
	);

[1] http://blogs.msdn.com/b/alejacma/archive/2008/01/23/how-to-sign-and-verify-with-cryptoapi-and-a-user-certificate.aspx

By far, the easiest way to accomplish your task (assuming we are not currently using a hardware security module for signing):
0. mkdir foo && cd foo
1. certutil -d . -N
2. certutil -S -d . -s "CN=My Cert" -n mycert -x -t ",,u" -g 2048
3. certutil certutil -L -d . -n mycert -r > mycert.der

(Note that this doesn't set all the cert options like the validity period correctly. It is just a demo.)

Then, take mycert.der and embed it into update.exe. Then use the CryptoAPI function parses a stream into a certificate object, and use the code above.

To sign your MAR file, you do basically the same thing, but using NSS functions. It would be a few dozen lines of code, at most; most of it can be copy/pasted from one of the examples in security/nss/cmd.

If you need to use a non-certificate format for the public key, then you will have to write twice as much code, or more.
Note also, by the way, that the certutil commands above will work with almost any HSM that has Linux support, when it comes time for that.
(In reply to Brian R. Bondy [:bbondy] from comment #21)
> Anthony thanks for checking.  Once ready, this will need to be tested on all
> platforms (unlike bug 481815 even linux / mac), but not all UAC levels on
> Windows like bug 481815 needs.   Incremental and full updates should also be
> tested. RelEng might have more ideas for testing.

Thanks for your response Brian. Does this work fall under a particular Silent Update feature?
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #54)
> (In reply to Brian R. Bondy [:bbondy] from comment #21)
> > Anthony thanks for checking.  Once ready, this will need to be tested on all
> > platforms (unlike bug 481815 even linux / mac), but not all UAC levels on
> > Windows like bug 481815 needs.   Incremental and full updates should also be
> > tested. RelEng might have more ideas for testing.
> 
> Thanks for your response Brian. Does this work fall under a particular
> Silent Update feature?

This work is all related to Removal of OS security dialog for Windows https://wiki.mozilla.org/Silent_Update_OS_Dialogs
(In reply to Lawrence Mandel [:lmandel] from comment #55)
> This work is all related to Removal of OS security dialog for Windows
> https://wiki.mozilla.org/Silent_Update_OS_Dialogs

QA work for this feature is assigned to a contractor who does not have security bug access, so they won't be able to see this bug in particular. Brian, can you reach out to your QA contact on Removal of OS Security Dialogs to make sure they are informed enough about this bug to be able to test it?
Given that public bug 544442 comment 12 already states that MAR files are not signed, and this is pretty much well-known to everybody that would care, I don't really see why this is in security-group anyway.
(In reply to Chris AtLee [:catlee] from comment #49)
> If we statically link NSS into the mar program,

This is not going to be possible anytime soon. Dynamic linking is the only way to link NSS in the foreseeable (as far as this bug goes) future.

> My plan was to have the build machines generate the mar file as usual, and
> then send it to the signing server. The signing server could generate and
> return just the signature, and the build machine would then update the mar
> file; or the signing server would return the entire signed mar file. Adding
> an option to the mar program to sign an existing mar file would be very
> handy.

How do you authenticate access to the signing server? That is, how do you prevent just anybody from signing anything they want with our key?
(In reply to Brian Smith (:bsmith) from comment #58)
> (In reply to Chris AtLee [:catlee] from comment #49)
> > If we statically link NSS into the mar program,
> 
> This is not going to be possible anytime soon. Dynamic linking is the only
> way to link NSS in the foreseeable (as far as this bug goes) future.
> 
> > My plan was to have the build machines generate the mar file as usual, and
> > then send it to the signing server. The signing server could generate and
> > return just the signature, and the build machine would then update the mar
> > file; or the signing server would return the entire signed mar file. Adding
> > an option to the mar program to sign an existing mar file would be very
> > handy.
> 
> How do you authenticate access to the signing server? That is, how do you
> prevent just anybody from signing anything they want with our key?

That's been discussed with infrasec elsewhere. Basically we're restricting access to this server to machines in the build network, and requiring that the machine authenticate itself either with client side SSL certs, or by attaching a signature of the file it's requesting be signed.
Re bsmith:
> Given that public bug 544442 comment 12 already states that MAR files 
> are not signed, and this is pretty much well-known to everybody that 
> would care, I don't really see why this is in security-group anyway.

I marked it as security because in the original comments I posted step by step instructions for an attack targeted at updates.  It is public knowledge that the MAR files are not signed, but I'm not sure if it is public knowledge that they need to be signed.
Thanks for the info in Comment 52 by the way.

> Currently the plan is to generate a MAR file that looks like this:
> <MAR header> <signature> <rest of the MAR file>
> IF, instead, you did things so that the file looked like this:
> <wrapper> <MAR header> <rest of MAR file> </wrapper>

Making the MAR files backwards compatible is important for at least 2 reasons:

1. The old updater.exe currently on people's machines needs to know how to read MAR files.  If we change how MAR files look, then old updater.exe would fail to be able to read MAR files.  One would think you could first add support for this in updater.exe, push that out as an update, and then future updates would work.  That is not correct though because people might not get that update and instead skip past it to the update after that.  Doing it this way would require us to always have people upgrading to a specific version, and then having to upgrade again after that to whatever the latest actually is.  

2. New MAR support won't be pushed out to other channels and releases, and so we'd have to maintain 2 different types of MAR files for updates. I don't think that would be a show stopper but it does complicate things more for releases.

--

Re: ashughes
> Thanks for your response Brian. Does this work fall under a particular 
> Silent Update feature?

Not exactly, but it is being done so that bug 481815 can land past Nightly into Aurora and beyond.

>  Brian, can you reach out to your QA contact on Removal of OS Security Dialogs 
> to make sure they are informed enough about this bug to be able to test it?

Yup once it is ready for testing I'll do that.
This patch does the same as the old patch but shows how we can swap in different crypto libraries to accomplish the same thing. 

The final code will work with whichever you compile NSS and CyrptoAPI and generate the same archives both ways. The plan is to use NSS for the 'mar' program and compile with CryptoAPI for the updater.exe.
Attachment #574990 - Attachment is obsolete: true
By the way in the final version whether you use CyrptoAPI or NSS #defines it'll generate the same hash and same signature.
Attached patch Intermediate patch (obsolete) — Splinter Review
This patch adds code for updater.exe verifying the signature before applying it. 

The next intermediate patch will have CyrptoAPI support for RSA signing, and verifying.
Attachment #575218 - Attachment is obsolete: true
Brian, for the signer, you should use the following functions, from cryptohi.h, in sequence:

	PK11_SignatureLen (to check that they key length is >= 2048 bits)
	SGN_NewContext (use SEC_OID_ISO_SHA1_WITH_RSA_SIGNATURE)
	SGN_Begin
	SGN_Update (2X: once for header, and then again for the rest of the marfile)
	SGN_End
	SGN_DestroyContext
(In reply to Brian R. Bondy [:bbondy] from comment #60)
> Re bsmith:
> > Given that public bug 544442 comment 12 already states that MAR files 
> > are not signed, and this is pretty much well-known to everybody that 
> > would care, I don't really see why this is in security-group anyway.
> 
> I marked it as security because in the original comments I posted step by
> step instructions for an attack targeted at updates.  It is public knowledge
> that the MAR files are not signed, but I'm not sure if it is public
> knowledge that they need to be signed.

just a note that i discussed this with dveditz earlier today and we agreed that this bug should stay hidden for now for the above reason - it outlines step by step instructions for an attack.
I've been fighting trying to get the NSS generated signed hash to verify with the CryptoAPI, and I just got it.  So that was the part I was most worried about. The problem was that the signed buffer is stored in reverse order from NSS to CyrptoAPI.

So the following fixed the problem for CryptVerifySignature to actually verify the signed hash successsfully:

  char signedBufferReversed[256];
  for (size_t i = 0; i < 256; i++) {
    signedBufferReversed[i] = signedBuffer[255 - i]; 
  }
(In reply to Brian R. Bondy [:bbondy] from comment #66)
> I've been fighting trying to get the NSS generated signed hash to verify
> with the CryptoAPI, and I just got it.  So that was the part I was most
> worried about. The problem was that the signed buffer is stored in reverse
> order from NSS to CyrptoAPI.

Oops! I remember that happening to me before too. It turns out that CyrptoAPI's output is backwards relative to NSS and OpenSSL here. This is why some OpenSSL commands even have a "-rev" option to accept CryptoAPI-generated content.

You should put your reversal code in the CryptoAPI version of the verification code, not in the signing code (I.e. the signature in the file should be in NSS/OpenSSL order, not CryptoAPI order), since CryptoAPI is the odd one out here.
This patch is mostly complete and actually signs and verifies the hash now.
Updater.exe has no depency on NSS, it uses CyrptoAPI to verify the signed hash.
The mar program uses NSS to do the signing.

Left to do (ETA today):
- Improve commands lines (remove hard coded certificate path)
- Some code cleanup
- Make the signing a 2 step process
- prefix "sha1-rsa:" before the hash
Attachment #575236 - Attachment is obsolete: true
I don't think the -rsa provides any value here. Also, we already use sha1, sha256, sha512, etc. to identify the algorithm used for the extension manager and app update.
> You should put your reversal code in the CryptoAPI version of the 
> verification code, not in the signing code 

Oops didn't see this until now, but I'm used to MS doing things a little backwards, so I happened to do what you suggested.

> I don't think the -rsa provides any value here.

OK cool, I guess I tend to want to put it because it could be signed/encrypted in different ways, but I'm fine with leaving it off.  Will do.
The other reason was that someone reading the spec might be confused and wonder why it's a 256byte hash instead of a 20 byte hash.  But this message is just explanatory, I'm doing as you suggested :)
I'm fine with leaving it with -rsa if you think it provides value.
catlee suggested  sha1-rsa2048, which I agree with the most.
I think it provides value because later we can add multiple signatures depending on what each platform supports natively, and by knowing the exact signing the platform can pick which one to verify.  I think it's possible we would one day want to do sha1-somethingelse so I'll do what catlee suggested unless there are major objections.
(In reply to Brian R. Bondy [:bbondy] from comment #68)
> - prefix "sha1-rsa:" before the hash

I suggest prefixing the hash with the single byte 0x01 to indicate "version 1 of the signature scheme." No reason to make it more complicated than that.

Also, you need to include this prefix in the signature (i.e. call SGN_Update on it).
bsmith, instead of having the updater support only one type of signature scheme we are going to support multiple in the future since a) we will need older clients able to update to newer versions b) other consumers of the updater may choose to use a different scheme than the one Firefox uses. Also, the additional complexity for the initial implementation is small.
(In reply to Brian R. Bondy [:bbondy] from comment #71)
> The other reason was that someone reading the spec might be confused and
> wonder why it's a 256byte hash instead of a 20 byte hash.  But this message
> is just explanatory, I'm doing as you suggested :)

In the spec, refer to the signature as a signature, not a hash. Then it will be less confusing. :) (It isn't a hash; it is a signature of a hash.)
(In reply to Robert Strong [:rstrong] (do not email) from comment #75)
> bsmith, instead of having the updater support only one type of signature
> scheme we are going to support multiple in the future since a) we will need
> older clients able to update to newer versions b) other consumers of the
> updater may choose to use a different scheme than the one Firefox uses.
> Also, the additional complexity for the initial implementation is small.

I do not understand what the intended file format is to be then. Is the draft spec available somewhere?
No spec as of yet since this was a last minute addition. One will be available prior to the security review.
After thinking about it more I think Brian Smith was right with a number to prefix the signed hash.

Each number should be defined in the MAR file format document and should describe:
1. The hash type
2. The signing algorithm and key length
3. The certificate/public key that should be used to verify the signature 

A value of 1 will mean the data is hashed with SHA1 and signed with an rsa 2048bit key.
It also means you need to use the first shipped certificate/public key to verify the signature. 

A value of 2 in the future could mean the same hash type and singing algorithm, but a different private key that signed it.

This allows us to have any number of certificates and future schemes in a single MAR file. 
If we ever need a new hashing algorithm, signing algorithm, or key to sign, we can include the old signature and and a new one.
Only old updater.exe would verify the old signatures.

I'll document all of this in the near future in the MAR file format wiki which I linked up.


> 4 bytes : "MAR1"
> 4 bytes : offset to INDEX in bytes (big endian) relative to start of file
> 4 bytes : number of hashes

Repeat :number of hashes" time:
> 4 bytes : signed hash ID.  THe ID is described in the MAR file format document
> X bytes : the signed hash

...

All bytes except for the actual hashes will be part of the hashes.
I documented the new MAR file format (which is backwards compatible) here:
https://wiki.mozilla.org/Software_Update:MAR

It differs slightly from the end of my Comment 79, so use the wiki page for the latest info.
Done: 
- Implemented NSS as a build config to be able to verify MAR signatures with, if we want to use it for OSX or Linux we could verify signatures on those platforms even today.
- CryptoAPI is used for Windows as before
- Implemented 2 phase signing process.  It works by creating a MAR file like before, then it repackages with signatures
- Put the relevent code into 2 files now mar_verify.c and mar_sign.c
- Implemented num_signatures field, signature_id field, and signature_length field
- Other cleanup

TODO before final patch for review:
- Some code cleanup
- Implement some command line params instead of the hard coded values I was using for testing
- Some build config fixups
- Update MAR file format documentation as per bsmith's emailed comments
- Update mar program command line wiki documentation

This should be ready for review as of next patch.
Attachment #575538 - Attachment is obsolete: true
Done: 
- Some code cleanup, refactoring, and testing
- Implement some command line params instead of the hard coded values I was using for testing
- Added num signatures field
- Added entire size of MAR field as per bsmith's suggestion
- Updated MAR file format documentation as per bsmith's emailed comments. The latest copy can be found here: https://wiki.mozilla.org/Software_Update:MAR

TODO before final patch for review:
- Fix makefiles in the current patch
- Add the old documentation as a patch, and the recent documentation changes as another patch
- Will need to include the v1 certificate and backup certificate in the installer once I get them from RelEng (See Bug Bug 704285 - Include certificates for MAR sign checks on Windows as part of updates and installers).
Attachment #575801 - Attachment is obsolete: true
I just read comment 0. Isn't this a generally elevation-of-privilege vulnerability? Don't we need a CVE? Don't we need some alternate mitigation strategy for earlier versions, including 3.6.x?
dveditz - could you answer comment #83? Thanks
> I just read comment 0. Isn't this a generally elevation-of-privilege vulnerability? Don't we need a CVE? 

Please explain more, I don't fully understand these questions.

> Don't we need some alternate mitigation strategy for earlier versions, including 3.6.x?

I can probably answer this once I know the first part.
(In reply to Brian R. Bondy [:bbondy] from comment #85)
> > I just read comment 0. Isn't this a generally elevation-of-privilege vulnerability? Don't we need a CVE? 
> 
> Please explain more, I don't fully understand these questions.

1. What you've described allows any program that can write to C:\Users\bbondy\AppData\Local\Mozilla\Firefox\Nightly\updates\0 to get something written to C:\Program Files\Firefox\<whatever>, that they wouldn't have privileges to write themselves. This is an elevation of privilege attack. I think this alone requires a CVE.

2. Any bug in the updater that could be exploited to cause data to be executed as content would potentially allow arbitrary content execution with elevated privilages. Hopefully DEP prevents this. But, I don't know if ROP or other attacks are possibilities with the updater. Have we ever looked into whether there are such vulnerabilities in the updater? Do we have ASLR and whatnot enabled for the updater? (This isn't really my domain; dveditz, Jesse, and others have experience here.)

The signature-less nature of the MAR files is a well-known issue, I think. The questions are really about if/how we must report this as a vulnerability in Firefox in our vulnerability disclosures, and if/how much we need to check previous releases for exploitability using maliciously-crafted MAR files. (e.g. a MAR file that has the file offsets within the file out of bounds or negative or whatnot.)
Blocks: 704285
(In reply to Brian Smith (:bsmith) from comment #83)
> I just read comment 0. Isn't this a generally elevation-of-privilege
> vulnerability?

Yes.

> Don't we need a CVE?

We don't use CVEs internally so we don't usually assign them until partners need a handle (e.g. when we announce fixes, sometimes earlier for cross-browser issues).

> Don't we need some alternate mitigation
> strategy for earlier versions, including 3.6.x?

By the time this ships 3.6.x may be EOL. If it's still supported then we can look into the feasibility of back-porting the work. I don't think there are any alternate approaches.

Since this is only a problem if you already have rogue software running on your machine product management may decide it is not worth it. Affected users should disinfect their machines and then (most of them) could upgrade to a version with the fix.
This is ready for review.

Summary:
libmar has support for verifying MARs with both NSS and CyrptoAPI.
The mar program can sign mars, but only with NSS.
The mar program is always using NSS to verify currently, but you can change a single define to use CryptoAPI instead.

usage:
mar [-C workingDir] {-c|-x|-t} archive.mar [files...]
mar [-C workingDir] -d NSSConfigDir -n certname -s archive.mar signed_archive.mar
mar [-C workingDir] -d NSSConfigDir -n certname -v signed_archive.mar

Examples:
Create a MAR:
mar -c c:\Users\bbondy\Desktop\test.mar c:\martest\1.txt c:\martest\2.txt c:\martest\0.txt

Sign a MAR:
mar -d c:\Users\bbondy\Desktop\foo2 -n mycert -s c:\Users\bbondy\Desktop\test.mar c:\Users\bbondy\Desktop\test_signed.mar

Verify a MAR:
mar -d c:\Users\bbondy\Desktop\foo2 -n mycert -v c:\Users\bbondy\Desktop\test_signed.mar

Extract a MAR: (Doesn't check the signature)
mar -x c:\Users\bbondy\Desktop\test_signed.mar

The verify mar code is used from the updater.exe code as well. 
That work is being tracked in bug 704285.
This bug is for libmar support and the mar program.

There is no module in the module owners wiki for modules/libmar so I think this fits mostly into the "Application Update" section. Let me know Rob S. if someone else can do this review so you can have a break from my code :).
Attachment #575965 - Attachment is obsolete: true
Attachment #576060 - Flags: review?(robert.bugzilla)
Attachment #576060 - Flags: review?(bsmith)
I should also mention that this document is also ready for re-review:
https://wiki.mozilla.org/Software_Update:MAR
Summary: Sign Mozilla archive files and verify them from updater.exe before applying updates → Add support for signing and verifying MAR files in libmar and the mar program
Attachment #576151 - Flags: review?(bsmith)
Attached patch New MAR file format wiki source (obsolete) — Splinter Review
Attachment #576153 - Flags: review?(bsmith)
Attachment #576151 - Attachment description: Original wiki file format → Original MAR file format wiki source
bsmith, I attached both documentation patches by the way in case you want it in source control instead.  I'm indifferent if we use the wiki or source control but I think it best to have the info only in one place and have the other reference the main information source.
Fixed documentation fail.  sizeOfEntireMAR field should have been marked as 8 bytes.
Attachment #576153 - Attachment is obsolete: true
Attachment #576153 - Flags: review?(bsmith)
Attachment #576156 - Flags: review?(bsmith)
Comment on attachment 576151 [details] [diff] [review]
Original MAR file format wiki source

bsmith, what did you want reviewed in the original version ? isn't this just superceded by attachment 576156 [details] [diff] [review] ?
Ian, yes you can ignore the original version.  I just included it in case bsmith wanted me to commit them to source so they would appear separately.
Attachment #576151 - Flags: review?(imelven)
Attachment #576151 - Flags: review?(bsmith)
Comment on attachment 576156 [details] [diff] [review]
New MAR file format wiki source. v2.

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

this looks good to me. one small nit : "The signatures must be composed of all bytes of the MAR file excluding the embedded signatures." could be reworded
as 'all bytes of the MAR file excluding the SIGNATURES block'.
Attachment #576156 - Flags: review?(imelven) → feedback+
Thanks for the feedback Ian. 

> 'all bytes of the MAR file excluding the SIGNATURES block'.

That would not be correct because the signature metadata is included in the signature calculation.
(In reply to Brian R. Bondy [:bbondy] from comment #97)
> Thanks for the feedback Ian. 
> 
> > 'all bytes of the MAR file excluding the SIGNATURES block'.
> 
> That would not be correct because the signature metadata is included in the
> signature calculation.

All the bytes of the MAR file except the SIGNATURES.SIGNATURE_ENTRY.Signature fields.
> All the bytes of the MAR file except the SIGNATURES.SIGNATURE_ENTRY.Signature fields.

that works, will update shortly.
(In reply to Brian R. Bondy [:bbondy] from comment #99)
> > All the bytes of the MAR file except the SIGNATURES.SIGNATURE_ENTRY.Signature fields.
> 
> that works, will update shortly.

Thanks Brian and Brian - that is much more explicit (which i think is good)

so when signing you need to know up front how many signatures will be on the MAR file in the end and their types - that seems alright, just an observation.
Comment on attachment 576156 [details] [diff] [review]
New MAR file format wiki source. v2.

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

::: modules/libmar/WikiFileFormat
@@ +3,5 @@
>  This document describes the Mozilla ARchive (MAR) format used by the update system to deliver update packages.  It is basically a series of files with an index tacked on to the end.
>  
>  == Details ==
>  
> +The file structure in a nut-shell is a header (HEADER), followed by a (SIGNATURES) block, followed by a list of variable length files, and finally ending with an index of the files (INDEX).  The index is a list of variable length entries (INDEX_ENTRY).  The signatures block is a list of variable length entries (SIGNATURE_ENTRY).

s/nut-shell/nutshell/

@@ +9,5 @@
>  
>  HEADER
> +  4 bytes : MARID - "MAR1"
> +  4 bytes : OffsetToIndex - offset to INDEX in bytes relative to start of file
> +  8 bytes : sizeOfEntireMAR - size in bytes of the entire MAR file

s/sizeOfEntireMAR/FileSize/

Use consistent capitalization.

@@ +33,3 @@
>    1 byte  : null terminator
>  
> +Some old MAR files will not contain the SIGNATURE block nor the sizeOfEntireMAR field.  Old parsers simply skip over it though because of the OffsetToIndex field.

"because they ignore everything between the MAR header and the offset given in the OffsetToIndex field."

@@ +42,4 @@
>  
> +== SignatureAlgorithmID  ==
> + 
> +  ID 1: The signature type is 2048-bit RSA-PKCS1-SHA1 of size 256 bytes 

Give each ID a name:

1: 2048-bit RSA-PKCS1-SHA1 (256 bytes).

IMO, this is overspecified. It should just be RSA-PKCS1-SHA1, between 2048 and 4096 or 8192 bits. (We need to check the RSA key size limit in NSS. I think it is 8K.)

@@ +46,2 @@
>  
> +The updater program will not use a MAR file if one of the known signatures does not verify. If the updater program encounters a SignatureAlgorithmID that it does not know about, it will not check the signature, and will proceed to the next signature.  This is why there exists a SignatureSize field, so that unknown signatures can be skipped.

Not correct. The updater will only accept the MAR file if at least one of the signatures verifies. (It *will* accept the file if some of the signatures do not verify, as long as one of them does.)

@@ +50,2 @@
>  
> +After Firefox 10 updater.exe on Windows will require a Signature ID of 1.

As of Firefox 11, only RSA-PKCS1-SHA1 signatures are accepted.

@@ +54,5 @@
> +== Byte Ordering ==
> +
> +All fields are in big-endian format.  
> +The signatures are in NSS / OpenSSL / big-endian order and not CryptoAPI order.
> +If CrytpoAPI is used to check a signature, the bytes of the signature should be reversed.

"must be reversed before verifying the signature using <name of CryptoAPI function used to verify signature>."

@@ +59,5 @@
> +
> +== Additional sections ==
> +
> +Additional MAR file sections may defined in the future.  It is recommended that these additional sections follow the SIGNATURES section.
> +If new sections are added, they must be included in the signature calculations.

There is no place to put new sections except after the SIGNATURES section.

Also, I think the last sentence can be removed. This is covered by the description of what must be signed.
Comment on attachment 576156 [details] [diff] [review]
New MAR file format wiki source. v2.

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

::: modules/libmar/WikiFileFormat
@@ +42,4 @@
>  
> +== SignatureAlgorithmID  ==
> + 
> +  ID 1: The signature type is 2048-bit RSA-PKCS1-SHA1 of size 256 bytes 

I meant:

1: RSA-PKCS1-SHA1 (2048 bits / 256 bytes).
> There is no place to put new sections except after the SIGNATURES section.

New sections could go in other locations.  For example in between any file content offset.
> so when signing you need to know up front how many signatures 
> will be on the MAR file in the end and their types - that seems 
> alright, just an observation.

In general the command lines allow for repackaging each time it signs, so this won't be a problem as each time we have all the info we need.  The first command line operation will add the first signature, the 2nd run will add the 2nd signature.  We're punting a lot of the multiple signature stuff until later though since it isn't needed today.
Ian you already r+ the last one, but this one has the recent feedback from you and bsmith in it, so I remarked it as r?.
Attachment #576156 - Attachment is obsolete: true
Attachment #576156 - Flags: review?(bsmith)
Attachment #576250 - Flags: review?(imelven)
Attachment #576250 - Flags: review?(bsmith)
Comment on attachment 576250 [details] [diff] [review]
New MAR file format wiki source. v3.

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

a couple of very small nits. 

also the secteam was discussing why a MAR file would have 0 or more signatures - the assumption is that this would be an older MAR file from before
we started signing, is that correct ?

::: modules/libmar/WikiFileFormat
@@ +38,3 @@
>  
> +Zero or more signatures can be specified.  
> +The signatures must be composed of all bytes of the MAR file excluding the Signature fields.

nit: i agree with bsmith that using SIGNATURE_ENTRY.Signature makes this a little clearer

@@ +46,3 @@
>  
> +The updater will only accept the MAR file if at least one of the signatures verifies.
> +Some versions of the updater may not apply a MAR file unless a specific SignatureAlgorithmID is inside of them.

does this mean 'unless a valid signature of a particular SignatureAlgorithmID is included in the MAR file' ?

@@ +53,5 @@
> +== Byte Ordering ==
> +
> +All fields are in big-endian format.  
> +The signatures are in NSS / OpenSSL / big-endian order and not CryptoAPI order.
> +If CrytpoAPI is used to check a signature, the bytes of the signature must be reversed before verifying the signature using CryptVerifySignature.

nit: typo - CryptoAPI
Attachment #576250 - Flags: review?(imelven) → feedback+
> nit: i agree with bsmith that using SIGNATURE_ENTRY.Signature 
> makes this a little clearer

I was about to do that, but then it would be inconsistent with how I refer to other fields throughout the rest of the document.  Should I update all field references to have the format CONTAINER.Field?
> also the secteam was discussing why a MAR file would have 0 or more 
> signatures - the assumption is that this would be an older MAR file 
> from before we started signing, is that correct ?

MAR files on Linux and OSX don't need to be signed since we don't do signature checks initially.  They can be signed though if RelEng wants them to be.  But no verification will happen on those platforms so they aren't required.
Whiteboard: [sg:critical] (with a privileged service) → [sg:critical] (with a privileged service) [rat:bsterne]
(In reply to Brian R. Bondy [:bbondy] from comment #107)
> > nit: i agree with bsmith that using SIGNATURE_ENTRY.Signature 
> > makes this a little clearer
> 
> I was about to do that, but then it would be inconsistent with how I refer
> to other fields throughout the rest of the document.  Should I update all
> field references to have the format CONTAINER.Field?

that would probably be the clearest thing to do :) thanks for the explanation on 0 signatures too - i was considering that possibility but then thought we were going to sign on those platforms and not verify - not signing there seems ok too.
Implemented Ian's latest review comments, and updated to CONTAINER.Field references.

> 0 signatures too - i was considering that possibility...

Ya I'm leaving it up to RelEng whether they want to sign them or not. Doesn't matter either way.
Attachment #576250 - Attachment is obsolete: true
Attachment #576250 - Flags: review?(bsmith)
Attachment #576338 - Flags: review?(imelven)
Attachment #576338 - Flags: review?(bsmith)
There was some confusion about `HEADER.Field` where one would think that the period was the end of the sentence and so the hash calculation was to exclude the whole section.  I fixed this by surrounding the field references with <tt>HEADER.Field</tt>.
Attachment #576151 - Attachment is obsolete: true
Attachment #576338 - Attachment is obsolete: true
Attachment #576338 - Flags: review?(imelven)
Attachment #576338 - Flags: review?(bsmith)
Attachment #576498 - Flags: review?(imelven)
Attachment #576498 - Flags: review?(bsmith)
Minor change if MAR_NSS is not defined and if not on Windows.
Attachment #576060 - Attachment is obsolete: true
Attachment #576060 - Flags: review?(robert.bugzilla)
Attachment #576060 - Flags: review?(imelven)
Attachment #576060 - Flags: review?(bsmith)
I need some help from anyone that knows how, or for bsmith when he gets back:
- The mar program is compiled as a host program and not a target program. So that means HOST_ prefix in makefiles. 
- This program uses NSS by default, but it can work to be CryptoAPI on Windows.
- This is causing problems for me when building with my current makefiles.
- If I remove all HOST_ prefix from \modules\libmar\tool\Makefile.in it works fine

I need some help or advise on how to get NSS compiled so it will work with a HOST compiled program.

Another option is that I can compile a target 'mar' program in addition to the HOST 'mar' program.  And only the target 'mar' program will link to NSS and be able to sign/verify bins. 

This is only a problem relating to the mar program by the way, not the work in updater.exe.
Note for RelEng:

You can compile a program currently for testing by using this latest patch on linux and removing all HOST_ prefixes from the file:
modules/libmar/tool/Makefile.in

It'll end up in your objdir/dist/bin
Attachment #576537 - Attachment is obsolete: true
Attachment #576545 - Flags: review?(imelven)
Attachment #576545 - Flags: review?(bsmith)
So, as currently written you can't do what you want. We build NSS for the target platform, not the host platform. (In most of our builds target == host, but for things like Android they're different.) Building NSS twice would be crazy, since it's one of the slower parts of our build (it doesn't build in parallel at all).

I don't think we actually generate MARs for Android builds, though, so it's possible we could change MAR to a target program (although that seems a bit odd, since it is intended to run on the host).  If we do that, we should condition building it behind a configure option (I think we already have --enable-update-packaging or something like that), and error if someone tries to use that during a cross-compile. The only thing that worries me there would be Mac builds, where we do an 32/64 bit build.
OK so here is the plan:

1. Move NSS building to happen before we build the mar program: 
[14:48:45] <ted> http://mxr.mozilla.org/mozilla-central/source/toolkit/toolkit-tiers.mk#84
[14:48:58] <ted> http://mxr.mozilla.org/mozilla-central/source/toolkit/toolkit-tiers.mk#213
[14:51:26] <ted> might have to fiddle there a little bit, make sure that updater doesn't get built before you build libmar

2. We will still build the mar program as a host program, that will remain unchanged.

3. The mar program will be built by default without the ability to sign / verify.  

4. A new configure option will be added so that you can force build the mar program to use the host library.  Something like --enable-mar-sign-verify

5. If that new configure option is used and host != target, then issue a friendly build error that this option is not available in cross compiles

6. libmar is currently built as both host and target.  
The host libmar is used by the mar program.  
The target libmar is used by updater on Windows.  
The target libmar will have the ability to verify always independent of that build option on Windows since it uses CryptoAPI.  
The target libmar on other platforms will not have the ability to verify currently.  Future support can be added for this in the context of another bug if we need it.

I will post this build work in another patch, so please continue to review the main logic patch of this bug.   A work around for RelEng was given in Comment 114 until the build config work is done (which is currently not my highest priority, so I'll come back to it next week).
minor correction:
4. A new configure option will be added so that you can force build the *host built* mar program to use the *target* library.  Something like --enable-mar-sign-verify
I'd call the option --enable-mar-signatures or something like that, and it should just disable building the host mar, and use the target mar instead, which could then be linked to the NSS libraries that we build.
I don't think we can ever disable building the host mar program because we use it for automatically creating MAR files for updates for channel releases.  For example each Nightly release gets new MAR files created by the host mar program.  

I think it would be enough to have -enable-target-mar which would built a target mar program that can sign/verify.  The host mar program would never be able to sign/verify, just create the MARs.  In that way we wouldn't need to issue any errors on cross compiles even.  The usage of the target mar program will be separate so it doesn't hurt cross compiles.
- Couple of minor fixes.  
- Now provide the ability in libmar to load the public key from memory instead of from disk. This is to avoid someone being able to replace the temporary file extracted from updater.exe on disk.

By the way I tested this with bug 704285 and bug 481815 and I have successfully done a full silent update from a signed mar that was first verified.
Attachment #576545 - Attachment is obsolete: true
Attachment #576545 - Flags: review?(imelven)
Attachment #576545 - Flags: review?(bsmith)
Attachment #576795 - Flags: review?(imelven)
Attachment #576795 - Flags: review?(bsmith)
Just an update on the Makefile stuff for host/ cross compiles.  I just need to test on other platforms but it is working now.  Patch coming later.

We don't need the new sign/verify inside the mar program that is built with host since we don't use that as part of the build process.  The host program as before will get built at objdir/dist/host/bin/mar.
It will be the same as before.  Created MAR files will have the needed fields to be able to sign it later though.

What I'm doing is having a second program now built from within the modules/libmar/tool/ which builds a target binary at objdir/dist/bin/signmar which is the same as the host program but also has the ability to sign and verify.

This works for normal compiles and cross compiles so there is no need for any extra flags or build configs.
Since this hasn't been reviewed yet I'll just throw the makefiles in this same patch. 

Makefiles are working now for building.
If you want to sign use the objdir/dist/bin/signmar program
Attachment #576795 - Attachment is obsolete: true
Attachment #576795 - Flags: review?(imelven)
Attachment #576795 - Flags: review?(bsmith)
Attachment #577654 - Flags: review?(robert.bugzilla)
Attachment #577654 - Flags: review?(bsmith)
Tested on both Windows and Linux btw.
Comment on attachment 576498 [details] [diff] [review]
New MAR file format wiki source. v5.

this looks good to me.
Attachment #576498 - Flags: review?(imelven) → feedback+
Comment on attachment 577654 [details] [diff] [review]
Support for signing and verifying MARs. v4.

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

a few comments and questions on this one below

a lot of them revolve around how many signatures are on the MAR file - in some places we restrict it to only one but in other
places there is code to handle more than one - in some places the code doesn't handle more than one correctly. i suggest
being consistent with this throughout this patch perhaps ? 

this patch should definitely be reviewed by bsmith also, _especially_ the bits that call into NSS :)

::: modules/libmar/sign/mar_sign.c
@@ +55,5 @@
> +#include "nss_secutil.h"
> +
> +int NSSInitCryptoContext()
> +{
> +  SECStatus status = NSS_Initialize("C:\\Users\\bbondy\\Desktop\\foo2", 

hardcoded path ?

@@ +70,5 @@
> +  return 0;
> +}
> +
> +/** 
> + * Obtains a singing context.

typo (i like singing context as a concept though)

@@ +175,5 @@
> +    goto failure;
> +
> +  /* Get the MAR length and adjust its size */
> +  if (fread(&sizeOfEntireMAR, sizeof(sizeOfEntireMAR), 1, fpSrc) != 1)
> +    goto failure;

this assumes the MAR file has the whole length in it, i guess we can rely on that ? (ie we won't sign
an old format file that doesn't have this field)

@@ +196,5 @@
> +  if (numSignatures == 0) {
> +    dstOffsetToIndex = ntohl(dstOffsetToIndex);
> +    dstOffsetToIndex += signatureSectionLength;
> +    dstOffsetToIndex = htonl(dstOffsetToIndex);
> +  }

what about the case when there was more than 1 signature in the file and we are blowing them 
away to re-sign with just 1 ?  the offsetToIndex we read is greater than the index in the new file

@@ +208,5 @@
> +  if (numSignatures == 0) {
> +    sizeOfEntireMAR = NETWORK_TO_HOST64(sizeOfEntireMAR);
> +    sizeOfEntireMAR += signatureSectionLength;
> +    sizeOfEntireMAR = HOST_TO_NETWORK64(sizeOfEntireMAR);
> +  }

same as above, if we are removing signatures, we need to re-calculate the total size

@@ +292,5 @@
> +  while (indexBufLoc != (indexBuf + indexLength)) {
> +    /* Adjust the offset */
> +    offsetToContent = (PRUint32 *)indexBufLoc; 
> +    *offsetToContent = ntohl(*offsetToContent);
> +    *offsetToContent += signatureSectionLength;

this seems fine since we're only allowing one signature - but not if there was multiple on the original file ?

since we only let you sign once, maybe this isn't an issue at the moment...  but there's code to handle more than one in
places as well.

::: modules/libmar/src/mar_create.c
@@ +153,5 @@
> +    goto failure;
> +  stack.last_offset += sizeof(sizeOfEntireMAR);
> +
> +  /* Write out the number of signatures, for now only at most 1 is supported */
> +  num_signatures = htonl(num_signatures);

this is always 0 above ?

::: modules/libmar/tool/mar.c
@@ +167,5 @@
> +    certBuffer = malloc(fileSize);
> +    if (!ReadFile(certFile, certBuffer, fileSize, &read, NULL) || 
> +        fileSize != read) {
> +      CloseHandle(certFile);
> +      return -1;

certBuffer isn't free'd here

::: modules/libmar/verify/cryptox.h
@@ +64,5 @@
> +                                   size_t signatureLen);
> +
> +#define CryptoX_LibraryHandle void*
> +#define CyrptoX_SignatureHandle VFYContext *
> +#define CyrptoX_PublicKey SECKEYPublicKey *

two typos of Crypto above

@@ +68,5 @@
> +#define CyrptoX_PublicKey SECKEYPublicKey *
> +#define CryptoX_Certificate CERTCertificate *
> +#define CyrptoX_InitCryptoLibrary(CryptoHandle, ConfigDir) \
> +  (*CryptoHandle = NULL), NSS_Init(ConfigDir)
> +#define CyrptoX_VerifyBegin(CryptoHandle, HashHandle, PublicKey) \

more typos

@@ +91,5 @@
> +
> +#include <windows.h>
> +#include <wincrypt.h>
> +
> +CryptoX_Result CyrptoAPI_InitCryptoContext(HCRYPTPROV *provider);

typo

@@ +105,5 @@
> +                                         char *signature, 
> +                                         DWORD signatureLen);
> +
> +#define CryptoX_LibraryHandle HCRYPTPROV
> +#define CyrptoX_SignatureHandle HCRYPTHASH

typo

@@ +106,5 @@
> +                                         DWORD signatureLen);
> +
> +#define CryptoX_LibraryHandle HCRYPTPROV
> +#define CyrptoX_SignatureHandle HCRYPTHASH
> +#define CyrptoX_PublicKey HCRYPTKEY

typo

@@ +110,5 @@
> +#define CyrptoX_PublicKey HCRYPTKEY
> +#define CryptoX_Certificate HCERTSTORE
> +#define CyrptoX_InitCryptoLibrary(CryptoHandle, ConfigDir) \
> +  CyrptoAPI_InitCryptoContext(CryptoHandle)
> +#define CyrptoX_VerifyBegin(CryptoHandle, HashHandle, PublicKey) \

more typos

@@ +134,5 @@
> +#define CyrptoX_InitCryptoLibrary(CryptoHandle, ConfigDir) \
> +  CryptoX_Error
> +#define CyrptoX_VerifyBegin(CryptoHandle, HashHandle, PublicKey) \
> +  CryptoX_Error
> +#define CryptoX_VerifyUpdate(HashHandle, buf, len) CryptoX_Error

typos...

::: modules/libmar/verify/mar_verify.c
@@ +124,5 @@
> +int mar_verify_signatureW(const PRUnichar *pathToMARFile, 
> +                          const char *certData,
> +                          size_t sizeOfCertData,
> +                          const char *configDir,
> +                          const char *certName) {

nit/observation : this could be refactored with the above since the only difference is the (w)fopen() call

@@ +205,5 @@
> +                                     signatureLen, 
> +                                     signatureAlgorithmID);
> +      free(extractedSignature);
> +      if (fseek(fp, curPos, SEEK_SET)) {
> +        return -3;

if the signature verification passes but the fseek fails, this will be treated as not verified ?

@@ +210,5 @@
> +      }
> +    }
> +  }
> +
> +  return 0;

this looks like it will return 0 if an unknown signature id is found - does this mean
an invalid signature id will end up looking to callers like the file was successfully verified ?

extractedSignature leaks with a signatureAlgorithm != 1 too

@@ +260,5 @@
> +    /* Get the signature algorithm ID */
> +    if (fread(&signatureAlgorithmID, sizeof(PRUint32), 1, fp) != 1)
> +        return -6;
> +
> +    /* Earlier signatures do not include the info of later signatures */

i was asking about this earlier during the spec/wiki review - this feels a little weird
since a signature's position affects what is actually covered by the signature.
Attachment #577654 - Flags: feedback+
Thanks for the review comments Ian.
I attached a new patch with the changes/fixes.

> this assumes the MAR file has the whole length in it, 
> i guess we can rely on that ? (ie we won't sign
> an old format file that doesn't have this field)

That is correct, you can only sign new mar files.
Old updater.exe can handle new mar files though which is the important part. 

> what about the case when there was more than 1 signature 
> in the file and we are blowing them 
> away to re-sign with just 1 ?  the offsetToIndex we read 
> is greater than the index in the new file
> ... other related comments.

We only support 1 sign currently via this program and it is always of the same size.
I tried to implement the verify code only though since we want this to be backwards compatible.
By backwards compatible I mean An older updater.exe should be able to verify newer MARs.


> mar_create.c
> +  num_signatures = htonl(num_signatures);
> this is always 0 above ?
That's correct it should always be 0.
I moved the initialization down to here and removed the htonl call since it's not needed :)



> typo, typo, typo,....

Apparently my right index finger is faster than my left :)


> mar_verify_signatureW
>nit/observation : this could be refactored with the above 
> since the only difference is the 
> (w)fopen() call

I thought of that too but I need them both to be defined for the mar program and for udpater.exe.
I can't have the char* convert to wchar_t and* since the wchar_t variant is only on Windows.

> this looks like it will return 0 if an unknown signature id is found

Oops, yes fixed.


> i was asking about this earlier during the spec/wiki review - 
> this feels a little weird
> since a signature's position affects what is actually covered by 
> the signature.

Currently we only support one signature, but the verify code tries to handle the case of future MARs that
will have more than one signature.
Attachment #577654 - Attachment is obsolete: true
Attachment #577654 - Flags: review?(robert.bugzilla)
Attachment #577654 - Flags: review?(bsmith)
Attachment #577836 - Flags: review?(robert.bugzilla)
Attachment #577836 - Flags: review?(bsmith)
Attachment #577836 - Flags: feedback?(imelven)
Fixed forward declaration missing a parameter which caused build problems on non-Windows platforms.
Attachment #577836 - Attachment is obsolete: true
Attachment #577836 - Flags: review?(robert.bugzilla)
Attachment #577836 - Flags: review?(bsmith)
Attachment #577836 - Flags: feedback?(imelven)
Attachment #577995 - Flags: review?(robert.bugzilla)
Attachment #577995 - Flags: review?(bsmith)
Attachment #577995 - Flags: feedback?(imelven)
Comment on attachment 577995 [details] [diff] [review]
Support for signing and verifying MARs. v5'.

>> i was asking about this earlier during the spec/wiki review - 
>> this feels a little weird
>> since a signature's position affects what is actually covered by 
>> the signature.

> Currently we only support one signature, but the verify code tries to handle 
> the case of future MARs that
> will have more than one signature.

right, this is more about how multiple signatures would work - it seems like each signature should contain the metadata for all of the signatures (which is a little more work to implement). i guess i need to think about the use case for multiple signatures - do we have one in mind ? 

also interested to hear what bsmith thinks about this part. everything else looks good.
Attachment #577995 - Flags: feedback?(imelven) → feedback+
Sounds good, a decision should be made about this soon so I can implement the verify to work that way. 

It'll also complicate the command lines that need to be used when we have multiple signatures.  If we use "only include metadata of what is before it" then it is much easier for command line usage because we can do 1 mar create call, followed by X mar sign calls.
(In reply to Ian Melven :imelven from comment #128)
> right, this is more about how multiple signatures would work - it seems like
> each signature should contain the metadata for all of the signatures.

Yes, Ian is right. Brian Bondy specifically documented this in the specification:

https://wiki.mozilla.org/Software_Update:MAR#SIGNATURE_blocks

"The signatures must be composed of all bytes of the MAR file excluding the SIGNATURE_ENTRY.Signature fields."
(In reply to Brian Smith (:bsmith) from comment #130)
> Yes, Ian is right. Brian Bondy specifically documented this in the
> specification:
> 
> https://wiki.mozilla.org/Software_Update:MAR#SIGNATURE_blocks
> 
> "The signatures must be composed of all bytes of the MAR file excluding the
> SIGNATURE_ENTRY.Signature fields."

Note that it is OK if the signing code only supports one signature, as long as the verification code correctly verifies according to the spec.
It doesn't matter for 1 signature, but I wanted to make it backwards compatible.
So currently since this is an open issue the wiki says all bytes.  The impelmentation though currently only does all SIGNATURE bytes before and including the said SignatureAlgorithmID.

Should I make it include all bytes? (Simple change) or should I update the wiki?
I agree about signing code by the way, it doesn't need to be implemented, it would be nice to verify that the verify code is working correctly for multiple signatures though.
(In reply to Brian R. Bondy [:bbondy] from comment #132)
> It doesn't matter for 1 signature, but I wanted to make it backwards
> compatible.
> So currently since this is an open issue the wiki says all bytes.  The
> impelmentation though currently only does all SIGNATURE bytes before and
> including the said SignatureAlgorithmID.
> 
> Should I make it include all bytes? (Simple change) or should I update the
> wiki?

that depends on if bsmith sees any actual issue with the way things are - if not, we can update the wiki to be consistent with the implementation.
Verify MAR now verifies all signature metadata, not just the ones before it.
Attachment #577995 - Attachment is obsolete: true
Attachment #577995 - Flags: review?(robert.bugzilla)
Attachment #577995 - Flags: review?(bsmith)
Attachment #578217 - Flags: review?(robert.bugzilla)
Attachment #578217 - Flags: review?(bsmith)
v7 has the ability to sign old MARs as well as new MARs.  New MARs have the extra 12 bytes after the index for num signatures (4 bytes) and size of mar (8 bytes).
RelEng needed this to land automatic signing of MARs.

Command lines for signing are the same, you can pass in either an old MAR or new MAR and it auto figures it out.
Attachment #578217 - Attachment is obsolete: true
Attachment #578217 - Flags: review?(robert.bugzilla)
Attachment #578217 - Flags: review?(bsmith)
Attachment #578737 - Flags: review?(robert.bugzilla)
Attachment #578737 - Flags: review?(bsmith)
v7 also adds error logging to every failure condition for sign and verify operations.
Fixed bug in mar_sign.c, uploaded a whole patch, but here is just the change:

 while (indexBufLoc != (indexBuf + indexLength)) {
     /* Adjust the offset */
     offsetToContent = (PRUint32 *)indexBufLoc; 
     *offsetToContent = ntohl(*offsetToContent);
     *offsetToContent += signatureSectionLength;
+    if (oldMar) {
+      *offsetToContent += sizeof(sizeOfEntireMAR) + sizeof(numSignatures);
+    }
     *offsetToContent = htonl(*offsetToContent);
     /* Skip past the offset, length, and flags */
     indexBufLoc += 3 * sizeof(PRUint32);
     indexBufLoc += strlen(indexBufLoc) + 1;
   }
Attachment #578737 - Attachment is obsolete: true
Attachment #578737 - Flags: review?(robert.bugzilla)
Attachment #578737 - Flags: review?(bsmith)
Attachment #578842 - Flags: review?(robert.bugzilla)
Attachment #578842 - Flags: review?(bsmith)
Comment on attachment 578842 [details] [diff] [review]
Support for signing and verifying MARs. v8.

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

So far, I have only reviewed mar_sign.c. It seems like either I don't understand how the re-signing code works, or that it doesn't work, or that it only works in special cases. If the re-signing code doesn't currently work, I strongly suggest removing it, to make the code easier to understand.

I will do more review of this code tomorrow.

::: modules/libmar/sign/mar_sign.c
@@ +68,5 @@
> +    fprintf(stderr, "ERROR: Could not initialize NSS\n");
> +    return 1;
> +  }
> +
> +  status = NSS_SetDomesticPolicy();

Do not call NSS_SetDomesticPolicy(). This is something that only needs to be done for programs that use libssl.

@@ +100,5 @@
> +  if (!*cert) {
> +    fprintf(stderr, "ERROR: Could not find cert from nickname\n");
> +    return -1;
> +  }
> + PK11_SetPasswordFunc(SECU_GetModulePassword);

nit: This should go immediately after the call to NSS_Initialize, not in this function.

@@ +117,5 @@
> +  }
> +
> +  *ctx = SGN_NewContext (SEC_OID_ISO_SHA1_WITH_RSA_SIGNATURE, *privKey);
> +  if (!*ctx) {
> +    fprintf(stderr, "ERROR: Could not create sign context\n");

nit: s/sign/signature/

@@ +122,5 @@
> +    return -1;
> +  }
> +  
> +  if (SGN_Begin(*ctx) != SECSuccess) {
> +    fprintf(stderr, "ERROR: Could not begin sign context\n");

nit: s/sign context/signature/

@@ +138,5 @@
> + * @param  fp     The MAR file handle
> + * @param  oldMar An out variable that stores 1 if the MAR is of old format.
> + * @return 0 on successful check.
> +*/
> +int IsOldMar(FILE *fp, int *oldMar) {

Why do we need to support old MAR files? If this support isn't necessary (anymore), then let's remove it to make the code easier to understand.

@@ +209,5 @@
> +  char buf[BLOCKSIZE];
> +  SECKEYPrivateKey *privKey = NULL;
> +  CERTCertificate *cert = NULL; 
> +  char *indexBuf = NULL, *indexBufLoc;
> +  size_t signatureSectionLength = sizeof(signatureAlgorithmID) + 

Why size_t here, and PRUint32 other places? I think this must be PRUint32 too.

Use MAR_ID_SIZE instead of sizeof(signatureAlgorithID).

@@ +210,5 @@
> +  SECKEYPrivateKey *privKey = NULL;
> +  CERTCertificate *cert = NULL; 
> +  char *indexBuf = NULL, *indexBufLoc;
> +  size_t signatureSectionLength = sizeof(signatureAlgorithmID) + 
> +                                  sizeof(signatureLength) +

Is sizeof(PRUint32) guaranteed to be 4, or is it just guaranteed to be at least 4?

I would rather avoid using sizeof for on-disk data structures, since sizeof(x) is used when the size of x is variable, but here any size other than 4 is wrong. (This applies to all the uses of sizeof in fread/fwrite.)

@@ +267,5 @@
> +    if (fread(&sizeOfEntireMAR, sizeof(sizeOfEntireMAR), 1, fpSrc) != 1) {
> +      fprintf(stderr, "ERROR: Could read mar size\n");
> +      goto failure;
> +    }
> +  

Here, verify that the input file is really sizeOfEntireMAR bytes long.

@@ +273,5 @@
> +    if (fread(&numSignatures, sizeof(numSignatures), 1, fpSrc) != 1) {
> +      fprintf(stderr, "ERROR: Could read num signatures\n");
> +      goto failure;
> +    }
> +    numSignatures = ntohl(numSignatures);

Does this code for resigning a signed MAR actually work, especially when the previously-signed input MAR file has a different number of signatures and/or signatures of different sizes than the output file will have? I ask this because it seems to me like this code only works correctly if the output file will be the same size as the input file. (But, I might very well be missing something.)

IMO, considering our planned workflow, it seems much better/simpler to insist that the input file is always unsigned, and fail here if numSignatures != 0. This would also result in fewer tests needed.

@@ +283,5 @@
> +      if (fread(&signatureLength, sizeof(signatureLength), 1, fpSrc) != 1) {
> +        fprintf(stderr, "ERROR: Could not read signature length\n");
> +        goto failure;
> +      }
> +      if (fseek(fpSrc, signatureLength, SEEK_CUR))

Missing braces.

This code must be untested because otherwise it always fails since the goto failure case is always executed.

@@ +289,5 @@
> +        goto failure;
> +    }
> +  } else {
> +    oldPos = ftell(fpSrc);
> +     if (fseek(fpSrc, 0, SEEK_END)) {

Indention.

@@ +294,5 @@
> +       fprintf(stderr, "ERROR: Could not seek to end of file.\n");
> +       goto failure;
> +     }
> +    sizeOfEntireMAR = ftell(fpSrc);
> +    sizeOfEntireMAR = HOST_TO_NETWORK64(sizeOfEntireMAR);

HOST_TO_NETWORK64 returns a value that cannot be interpreted as a number, because it is an invalid value due to the potentially-wrong byte order. Because of this, it is a bad idea to store the result of HOST_TO_NETWORK64 in a variable with a meaningful name like this. Instead, it is better to do PRUint64 temp = HOST_TO_NETWORK64(sizeOfEntireMAR) at the time we write the value to the file, and then fwrite(..., temp,...).

Or in other words, instead of insisting that sizeOfEntireMAR is always in the wrong byte order, we should insist that sizeOfEntireMAR is in the correct (native) byte order.

@@ +305,5 @@
> +  /* Write out the new offset to the index */
> +  dstOffsetToIndex = offsetToIndex;
> +  if (numSignatures == 0) {
> +    dstOffsetToIndex = ntohl(dstOffsetToIndex);
> +    dstOffsetToIndex += signatureSectionLength;

Calculate signatureSectionLength here instead of above, to make the code easier to read.

@@ +320,5 @@
> +                 sizeof(dstOffsetToIndex)) != SECSuccess) {
> +    fprintf(stderr, "ERROR: Could not update sign context with index offset\n");
> +    goto failure;
> +  }
> +

Instead of duplicating this logic, fwrite-then-SGN_Update, over and over, create a function that you can call like this:

    writeMARBytes(fpDest, ctx, buf, bufLen, INCLUDED_IN_SIGNATURE);
or this:
    writeMARBytes(fpDest, ctx, buf, bufLen, NOT_INCLUDED_IN_SIGNATURE);

that encapsulates this pattern. This will make it easier to see what the signature covers and doesn't cover.

@@ +354,5 @@
> +    goto failure;
> +  }
> +
> +  /* Write out the signature ID, for now only an ID of 1 is supported */
> +  signatureAlgorithmID = htonl(signatureAlgorithmID);

initialize signatureAlgorithmID here, so we don't have to scroll up to find the value.

@@ +368,5 @@
> +    goto failure;
> +  }
> +
> +  /* Write out the signature, length */
> +  signatureLength = XP_HASH_LEN;

NSSSignBegin should return signatureLength as an output parameter, instead of hardcoding XP_HASH_LEN here.

Also, *HASH* is the wrong term here.

@@ +380,5 @@
> +    fprintf(stderr, "ERROR: Could not update sign context with signature length\n");
> +    goto failure;
> +  }
> +
> +  /* Write out a placeholder for the hash, we'll come back to this later */

Add a comment:

 *** THIS IS NOT SIGNED because it a placeholder that will be replaced below. ***

@@ +450,5 @@
> +  while (indexBufLoc != (indexBuf + indexLength)) {
> +    /* Adjust the offset */
> +    offsetToContent = (PRUint32 *)indexBufLoc; 
> +    *offsetToContent = ntohl(*offsetToContent);
> +    *offsetToContent += signatureSectionLength;

Isn't this assuming that there were no signatures in the input MAR file? (i.e. it seems, from inspection, that this doesn't work in the re-signing case.)

@@ +468,5 @@
> +    fprintf(stderr, "ERROR: Could not update sign context with index length\n");
> +    goto failure;
> +  }
> +
> +  /* Get the hash */

s/hash/signature/.

@@ +473,5 @@
> +  if (SGN_End(ctx, &secItem) != SECSuccess) {
> +    fprintf(stderr, "ERROR: Could not end sign context\n");
> +    goto failure;
> +  }
> +  if (XP_HASH_LEN != secItem.len) {

signatureLength != secItem.len would be clearer
(but, note that htonl confusing here.)

@@ +480,5 @@
> +  }
> +
> +  /* Finally write out the signature that we stored
> +     a placeholder in earlier. */
> +  if (fseek(fpDest, SIGNATURE_OFFSET, SEEK_SET)) {

Instead of hard-coding SIGNATURE_OFFSET here, you should just ftell() before writing the placeholder, and then fseek() to the result of that ftell.

@@ +484,5 @@
> +  if (fseek(fpDest, SIGNATURE_OFFSET, SEEK_SET)) {
> +    fprintf(stderr, "ERROR: Could not seek to signature offset\n");
> +    goto failure;
> +  }
> +  if (fwrite(secItem.data, secItem.len, 1, fpDest) != 1) {

Add a comment:

   *** THIS IS NOT SIGNED because signatures
       cannot sign themselves or each other ***

::: modules/libmar/sign/nss_secutil.h
@@ +43,5 @@
> +#include <plgetopt.h>
> +#include "pk11func.h"
> +#include "cryptohi.h"
> +#include "hasht.h"
> +#include "ssl.h"

Don't include ssl.h, because you aren't doing anything with SSL.
(In reply to Brian Smith (:bsmith) from comment #139)
> @@ +138,5 @@
> > + * @param  fp     The MAR file handle
> > + * @param  oldMar An out variable that stores 1 if the MAR is of old format.
> > + * @return 0 on successful check.
> > +*/
> > +int IsOldMar(FILE *fp, int *oldMar) {
> 
> Why do we need to support old MAR files? If this support isn't necessary
> (anymore), then let's remove it to make the code easier to understand.
Because otherwise releng would have to create two mar files and advertise one update for users on versions that don't support signing and a different one for versions that do support signing. I know this question has been brought up a couple of times previously and if you'd like to discuss it again please contact me.
So, if we require mar signing I think we are going to have to require admin privileges during installation. At the very least we are going to have to just notify (e.g. don't try to download / apply an update) the user that an update is available with the link to download it when the HKLM for the installation are not present.
> So, if we require mar signing I think we are going to have to require admin 
> privileges during installation. At the very least we are going to have to just 
> notify (e.g. don't try to download / apply an update) the user that an update is 
> available with the link to download it when the HKLM for the installation are not 
> present.

That shouldn't be needed.  The reg keys are only for authenticode checks on exes to make sure it is signed by us. For the authenticode checks on exes we also check it is signed by something that chains up to a trusted root.

For MAR signing we don't use authenticode, the registry, nor a trusted cert.  We rely on signing with a private key, and then verifying that signature with a public key embedded in the updater.exe.
> Is sizeof(PRUint32) guaranteed to be 4, or is it just 
> guaranteed to be at least 4?

> I would rather avoid using sizeof for on-disk data structures, since sizeof(x)
> is used when the size of x is variable, but here any size other than 4 is
> wrong. (This applies to all the uses of sizeof in fread/fwrite.)

sizeof(PRUint32) and sizeof(variableOfTypePRUint32) is guaranteed to be 4 on all platforms.

From the documentation here:
https://developer.mozilla.org/en/PRUint32

> Guaranteed to be an unsigned 32-bit integer on all platforms. 

I don't really see PRUint32 to be an on disk data structure, I just see it as 4 consecutive bytes in memory. It is not subject to alignment nor padding.  I know we have a lot of code throughout the whole tree that makes the assumption of 4 bytes and also that uses sizeof(PRUint32) and sizeof(variablesOfTypePRUint32).
I should also mention that I think using sizeof(variableOfSomeType) can be more clear in code when there is no harm to do so like in the case of it being a PRUint32.

Example in the patch:
 *offsetToContent += sizeof(sizeOfEntireMAR) + sizeof(numSignatures);
Implemented bmsith's review comments on mar sign.
Attachment #578842 - Attachment is obsolete: true
Attachment #578842 - Flags: review?(robert.bugzilla)
Attachment #578842 - Flags: review?(bsmith)
Attachment #580158 - Flags: review?(robert.bugzilla)
Attachment #580158 - Flags: review?(bsmith)
Depends on: 708854
No longer depends on: 708854
This is pushed to elm by the way.  I will update elm as review comments come in.

bsmith & rs, any update on this review?
No bug fixes just refactoring:
- Now check if the MAR is old and return verify failure if so since it has no signature
- Now check for malloc failure
- Added some missing error messages on verify
- Combined the read and verify update calls into a wrapper function to reduce the amount of code
Attachment #580158 - Attachment is obsolete: true
Attachment #580158 - Flags: review?(robert.bugzilla)
Attachment #580158 - Flags: review?(bsmith)
Attachment #580907 - Flags: review?(robert.bugzilla)
Attachment #580907 - Flags: review?(bsmith)
Comment on attachment 580907 [details] [diff] [review]
Support for signing and verifying MARs. v10.

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

When submitting patches for review, please use showfunc=true in your .hgrc so that the function names show up in the context parts of the diffs.

Make sure that the code compiles without warning (in particular, signed/unsigned conversion warnings and value truncation warnings) on all platforms, especially Win32. I suspect that, if I were to build this code, I would get such warnings, at least on Windows.

nss_secutil.[ch] is copied out of nss/cmd, right? I think we can avoid copying secutil.c out of NSS. Please explain how releng wants to communicate the password to the program, and I will help you with this.

I will finish the review (cryptox.[ch] and mar_verify.c) ASAP. In the other files, there are no things in this patch that I fould to be *definitely wrong*. Mostly, my comments are about nits, clarifications, and unlikely-to-occur overflows.

::: modules/libmar/sign/Makefile.in
@@ +65,5 @@
> +  -I$(topsrcdir)/security/nss/lib/certdb \
> +  -I$(topsrcdir)/security/nss/lib/pkcs7 \
> +  -I$(topsrcdir)/security/nss/lib/smime \
> +  -I$(topsrcdir)/security/nss/lib/freebl \
> +  -I$(topsrcdir)/security/nss/lib/freebl/ecl \

When NSS is built, it outputs all the public header files to a single directory (dist/include). Use that directory instead of these directories, so that you don't accidentally depend on any internal NSS features. (e.g. freebl/ecl is 100% internal, IIRC.)

::: modules/libmar/sign/mar_sign.c
@@ +49,5 @@
> +#include <winsock2.h>
> +#else
> +#include <netinet/in.h>
> +#include <unistd.h>
> +#endif

I think you are including these becaues you need ntohl, et al. You probably
know already that NSPR handles this:
PRUint16 PR_ntohs(PRUint16 n);
PRUint32 PR_ntohl(PRUint32 n);
PRUint16 PR_htons(PRUint16 n);
PRUint32 PR_htonl(PRUint32 n);
PRUint64 PR_ntohll(PRUint64 n);
My guess is that you are avoiding NSPR here because you want to avoid having the Windows updater.exe depend on nspr4.dll, and you want the verification and signing code to be consistent in which set of functions they use. If that is the case, the please just move the above #include logic to the header where you define NETWORK_TO_HOST64 along with a comment (similar to this review comment) explaining why we are doing this.

@@ +67,5 @@
> +  SECStatus status = NSS_Initialize(NSSConfigDir, 
> +                                    "", "", SECMOD_DB, NSS_INIT_READONLY);
> +  if (SECSuccess != status) {
> +    fprintf(stderr, "ERROR: Could not initialize NSS\n");
> +    return 1;

return -1; (to be consistent with the rest of the program).

@@ +87,5 @@
> +                 CERTCertificate **cert,
> +                 PRUint32 *signatureLength) 
> +{
> +  secuPWData pwdata = { PW_NONE, 0 };
> +  if (!privKey || !cert || !signatureLength) {

This set of checks is incomplete.

@@ +112,5 @@
> +  /* Check that the key length is >= 2048 bits (256 bytes) */
> +  if (PK11_SignatureLen(*privKey) < 256) {
> +    fprintf(stderr, "ERROR: Signature length must be >= 2048 bits\n");
> +    return -1;
> +  }

I believe this program only works correctly when
XP_SIGNATURE_LEN == PK11_SignatureLen(*privKey) and
XP_SIGNATURE_LEN == 256.
It would be best to remove the XP_SIGNATURE_LEN macro completely, and replace it with a XP_MIN_SIGNATURE_LEN_IN_BYTES constant that is 256.
However, if we don't have time to do that, then at least change the code above to:
*signatureLength = PK11_SignatureLen(*privKey);
if (*signatureLength != XP_SIGNATURE_LEN) {
// XXX: The key size must be at least XP_SIGNATURE_LEN
// but some code assumes the key size is exactly
// XP_SIGNATURE_LEN.
fprintf(...);
return -1;
}
This way, we will be reminded that we postponed this work if/when we try to use a different key size in the future.
It is a good idea to be explicit about lengths being in bits or bytes, especially in code that deals with crypto, because different APIs return different units. To avoid asking you to make many small renamings like that, let's operate under the assumption that lengths are in bytes by default.

@@ +152,5 @@
> +  if (fwrite(buffer, size, 1, fpDest) != 1) {
> +    fprintf(stderr, "ERROR: Could not write %s\n", err);
> +    return -2;
> +  }
> +  if (SGN_Update(ctx, (char*)buffer, size) != SECSuccess) {

(const unsigned char *)buffer

@@ +160,5 @@
> +  return 0;
> +}
> +
> +/**
> + * Writes the passed buffer to the file fp and updates the signature context.

This description should say something about the fact that the bytes are read from the src file

@@ +192,5 @@
> +
> +
> +/** 
> + * Writes out a copy of the MAR at src but with an embedded signature.
> + * If the MAR at src already contains signatures, they are stripped

The input MAR file must be unsigned as of this revision.

@@ +210,5 @@
> +  PRUint32 offsetToIndex, dstOffsetToIndex, indexLength, 
> +    numSignatures = 0, signatureLength,
> +    signatureAlgorithmID, *offsetToContent, i, oldMar, oldPos,
> +    signatureSectionLength, numChunks, leftOver, signaturePlaceholderOffset;
> +  PRUint64 sizeOfEntireMARField = 0, realSizeOfSrcMAR;

It is not a good idea to use PRUint64 or PRUint32 for (most of) these variables, because they are going to be constantly used as long, as that is what the which might be a 32-bit signed integer.

Instead, use the types that are appropriate based on the APIs that you are using (currently long), and verify that the values read from the input file are within range (not negative, not larger than LONG_MAX). It may be useful to have a "read 32-bit integer" function and a "read 64-bit integer" function that each do these checks and the network-to-byte conversion.

Rename sizeOfEntireMARField to sizeOfEntireMAR to be consistent with the other variables that you are using to hold fields from the MAR file (which aren't suffixed with "Field").

f we insist on using sizeof, at least do the following somewhere (preferably in a header that is included everywhere):
PR_STATIC_ASSERT(sizeof(PRUint32) == 4);
PR_STATIC_ASSERT(sizeof(PRUint64) == 8);

@@ +258,5 @@
> +  /* Offset to index */
> +  if (fread(&offsetToIndex, sizeof(offsetToIndex), 1, fpSrc) != 1) {
> +    fprintf(stderr, "ERROR: Could not read offset\n");
> +    goto failure;
> +  }

offsetToIndex = ntohl(offsetToIndex);

@@ +300,5 @@
> +    }
> +  } else {
> +    sizeOfEntireMARField = sizeOfEntireMARField;
> +  }
> +

Check that offsetToIndex <= sizeOfEntireMAR. This way, we can safely use "sizeOfEntireMAR - offsetToIndex" below.
Check that offsetToIndex == ftell(fpSrc).

@@ +306,5 @@
> +  signatureSectionLength = sizeof(signatureAlgorithmID) + 
> +                           sizeof(signatureLength) +
> +                           signatureLength;
> +  dstOffsetToIndex = offsetToIndex;
> +  dstOffsetToIndex = ntohl(dstOffsetToIndex);

This line is not necessary, and would be wrong, if you do offsetToIndex = ntohl(offsetToIndex) above like I suggest.

@@ +325,5 @@
> +  sizeOfEntireMARField += signatureSectionLength;
> +  if (oldMar) {
> +    sizeOfEntireMARField += sizeof(sizeOfEntireMARField) + sizeof(numSignatures);
> +  }
> +  sizeOfEntireMARField = HOST_TO_NETWORK64(sizeOfEntireMARField);

Instead, temp64 = HOST_TO_NETWORK64(sizeOfEntireMAR) and write temp64, because (a) sizeOfEntireMAR should not be PRUint64 (see comments above), and (b) we should keep it in the correct byte order so that we can read it while debugging.

@@ +351,5 @@
> +    goto failure;
> +  }
> +  signatureAlgorithmID = ntohl(signatureAlgorithmID);
> +
> +  /* Write out the signature, length */

remove the comma.

@@ +366,5 @@
> +     *** THIS IS NOT SIGNED because it is a placeholder that will be replaced
> +         below, plus it is going to be the signature itself. ***
> +  */
> +  signaturePlaceholderOffset = ftell(fpDest);
> +  if (fwrite(buf, signatureLength, 1, fpDest) != 1) {

Must check that the size of the buffer is large enough to hold the signature.
Also, to avoid using uninitialized memory, memset buf to zeros before using it. (This isn't a serious issue in the current program, but it could be real bug if we ever use this code in some other program. And, the uninitialized memory read will cause various static analysis / memory bug finding tools to complain.)

@@ +371,5 @@
> +    fprintf(stderr, "ERROR: Could not write signature length\n");
> +    goto failure;
> +  }
> +
> +  /* Write out the rest of the MAR excluding the index header and index */

AFAICT, in a valid input file, there should be nothing to copy here, right?
If we extend the MAR file format in the future, then it might not be correct to blindly copy these bytes. It would be safer to just insist that there be nothing here, using the offsetToIndex == ftell(fpSrc) check I suggest above, removing this copying completely.

@@ +408,5 @@
> +  while (indexBufLoc != (indexBuf + indexLength)) {
> +    /* Adjust the offset */
> +    offsetToContent = (PRUint32 *)indexBufLoc; 
> +    *offsetToContent = ntohl(*offsetToContent);
> +    *offsetToContent += signatureSectionLength;

Put this line after the if (oldMar) block, since sizeOfentireMAR and numSignatures appear before the signatures. (i.e. always do operations in an order analogous to the order the items appear in the file.)

::: modules/libmar/src/mar.h
@@ +149,5 @@
> + *         a negative number if there was an error
> + *         a positive number if the signature does not verify
> + */
> +#ifdef XP_WIN
> +int mar_verify_signatureW(const PRUnichar *pathToMAR, 

Can't we just remove the configDir and certName parameters in mar_verify_signatureW?

::: modules/libmar/src/mar_create.c
@@ +126,5 @@
>  }
>  
>  int mar_create(const char *dest, int num_files, char **files) {
>    struct MarItemStack stack;
> +  PRUint32 offset_to_index = 0, size_of_index, num_signatures;

Usually we don't use underscores here, but camelCase.

@@ +145,5 @@
>      goto failure;
>    if (fwrite(&offset_to_index, sizeof(PRUint32), 1, fp) != 1)
>      goto failure;
>  
>    stack.last_offset = MAR_ID_SIZE + sizeof(PRUint32);

stack.last_offset = MAR_ID_SIZE
                  + sizeof(numSignatures)
                  + sizeof(sizeOfentireMar);

@@ +150,5 @@
>  
> +  /* We will circle back on this at the end of the MAR creation to fill it */
> +  if (fwrite(&sizeOfEntireMAR, sizeof(sizeOfEntireMAR), 1, fp) != 1)
> +    goto failure;
> +  stack.last_offset += sizeof(sizeOfEntireMAR);

remove this, if you do what I suggested above.

@@ +186,5 @@
>      goto failure;
>    if (fwrite(&offset_to_index, sizeof(offset_to_index), 1, fp) != 1)
>      goto failure;
> +  
> +  sizeOfEntireMAR = stack.last_offset +

Cast each item being added to (PRUint64).

::: modules/libmar/src/mar_private.h
@@ +51,5 @@
>  #define MAR_ITEM_SIZE(namelen) (3*sizeof(PRUint32) + (namelen) + 1)
>  
> +#if defined WORDS_BIGENDIAN || defined IS_BIG_ENDIAN
> +#define NETWORK_TO_HOST64(x) (x)
> +#define HOST_TO_NETWORK64(x) (x)

remove this definition of HOST_TO_NETWORK64...

@@ +62,5 @@
> +  (((((PRUint64) x) >> 32) & 0xFF) << 24) | \
> +  (((((PRUint64) x) >> 40) & 0xFF) << 16) | \
> +  (((((PRUint64) x) >> 48) & 0xFF) << 8) | \
> +  (((PRUint64) x) >> 56)
> +#define NETWORK_TO_HOST64 HOST_TO_NETWORK64

...and move this definition outside of the #if...#else..#endif.

::: modules/libmar/src/mar_read.c
@@ +302,5 @@
> + * @return 0 on successful check.
> +*/
> +int IsOldMAR(FILE *fp, int *oldMar)
> +{
> +  PRUint32 offsetToIndex, offsetToContent, oldPos;

See the comments in mar_sign.c about types and range checking for these kinds of variables.

@@ +309,5 @@
> +  }
> +
> +  oldPos = ftell(fp);
> +
> +  /* Skip to the start of the signature block */

/* Skip to the offset of the index */, right?

@@ +320,5 @@
> +    return -1;
> +  offsetToIndex = ntohl(offsetToIndex);
> +
> +  /* Skip to the first index entry past the index size field */
> +  if (fseek(fp, offsetToIndex + sizeof(PRUint32), SEEK_SET)) {

offsetToindex + sizeof(PRUint32) could overflow, in theory. Perhaps just seek to offsetToIndex, and then seek past IndexSize separately.

::: modules/libmar/tool/Makefile.in
@@ +65,5 @@
> +LIBS = $(DEPTH)/modules/libmar/src/$(LIB_PREFIX)mar.$(LIB_SUFFIX) \
> +  $(DEPTH)/modules/libmar/sign/$(LIB_PREFIX)signmar.$(LIB_SUFFIX) \
> +  $(DEPTH)/modules/libmar/verify/$(LIB_PREFIX)verifymar.$(LIB_SUFFIX) \
> +  $(DIST)/lib/$(LIB_PREFIX)crmf.$(LIB_SUFFIX) \
> +  $(DIST)/lib/$(LIB_PREFIX)smime3.$(LIB_SUFFIX) \

I am surprised you need crmf and smime. What error do you get if you link without them?

@@ +66,5 @@
> +  $(DEPTH)/modules/libmar/sign/$(LIB_PREFIX)signmar.$(LIB_SUFFIX) \
> +  $(DEPTH)/modules/libmar/verify/$(LIB_PREFIX)verifymar.$(LIB_SUFFIX) \
> +  $(DIST)/lib/$(LIB_PREFIX)crmf.$(LIB_SUFFIX) \
> +  $(DIST)/lib/$(LIB_PREFIX)smime3.$(LIB_SUFFIX) \
> +  $(DIST)/lib/$(LIB_PREFIX)ssl3.$(LIB_SUFFIX) \

You do not need libssl.

::: modules/libmar/tool/mar.c
@@ +59,5 @@
> +#ifndef NO_SIGN_VERIFY
> +  printf("  mar [-C workingDir] -d NSSConfigDir -n certname -s "
> +         "archive.mar out_signed_archive.mar\n");
> +
> +#if defined(XP_WIN) && !defined(MAR_NSS)

Do not overload "-d" to mean different things in different configurations. Instead, use a different flag for DERFilePath, e.g. "-D".

Isn't it possible to make this program work such that you don't have to compile it twice, once with !defined(MAR_NSS) and once with defined(MAR_NSS)?

@@ +153,5 @@
> +    }
> +
> +#if defined(XP_WIN) && !defined(MAR_NSS)
> +    /* If the mar program was built using CryptoAPI, then read in the buffer
> +       containing the cert. */

Instead, make this a runtime condition based on whether -D was given on the command line instead of "-d". (Still has to be #if defined(XP_WIN)).

@@ +176,5 @@
> +    CloseHandle(certFile);
> +
> +    /* If we compiled with CryptoAPI load the cert from disk.
> +       The path to the cert was passed in the -d parameter */
> +    if (mar_verify_signature(argv[2], certBuffer, fileSize, 

mar_verify_signatureW

::: modules/libmar/verify/cryptox.c
@@ +51,5 @@
> +                          "", "", 
> +                          SECMOD_DB, NSS_INIT_READONLY);
> +  if (SECSuccess != status) {
> +    return status;
> +  }

Remove this if statement, as it is redundant with the return statement below.

@@ +61,5 @@
> +                                 SECKEYPublicKey **publicKey, 
> +                                 CERTCertificate **cert)
> +{
> +  secuPWData pwdata = { PW_NONE, 0 };
> +  if (!cert || !publicKey) {

|| !cert

@@ +79,5 @@
> +}
> +
> +CryptoX_Result NSS_VerifyBegin(VFYContext **ctx, SECKEYPublicKey **publicKey)
> +{
> +  SECStatus status;

Check (ctx && *ctx && publicKey && *publicKey)

Check public key length is large enough.
> When submitting patches for review, please use showfunc=true in your .hgrc so
> that the function names show up in the context parts of the diffs.

I have already always had this in all of my patches.  In the attached patch you can see the functions which show up already as part of the context on diffs.  Please see the splinter review which shows them.  Please let me know in which part of the patch you think it is not working correctly.

> Make sure that the code compiles without warning (in particular, signed/unsigned
> conversion warnings and value truncation warnings) on all platforms, 
> especially Win32. I suspect that, if I were to build this code, I would get 
> such warnings, at least on Windows.

The development was done on Windows and there are no such warnings, I'm using VS2010. 
I verified for warnings before submitting the patch, and re-verified just now.
I admit that I don't check all platforms / compilers for warnings on each patch though, but I don't think anyone does that as it would lead to significantly slower development.
 
I verified just now in Linux and I get a couple of warnings there which is:
> if (SGN_Update(ctx, (char*)buffer, size) != SECSuccess) {

Where the warning is that it should be (const unsigned char*).
I think that is what you are referring to so I will fix that in the next patch.
htonl function calls do not produce warnings on any platform with unsigned values.

> It is not a good idea to use PRUint64 or PRUint32 for (most of) these variables, 
> because they are going to be constantly used as long, as that is what the which might be a 32-bit signed integer.

For the PRUint32, this is consistent with the usage in mar_create and mar_read.  It was never documented on the MAR file format page but the implementation has always been to use unsigned values which leave no room for invalid values in the MAR file.

> Isn't it possible to make this program work such that you don't have to 
> compile it twice, once with !defined(MAR_NSS) and once with defined(MAR_NSS)?

The reason we need to compile it twice is because we can't use NSS currently in host built programs. 
So we build the host built program as mar which can do everything it used to.
And we build the target built program as signmar which can do everything mar can do plus sign and verify.
RelEng is fine with this since they use a pre-built signmar on a different machine which is not the build server.
Unless there are strong objections here, I'd like to keep this the same.

> nss_secutil.[ch] is copied out of nss/cmd, right? 
> I think we can avoid copying secutil.c out of NSS. 

Right from the comment in the Makefile: The Original Code is code copied from secutil and secpwd.

> Please explain how releng wants to communicate the password to the program, and I will help you with this.

They want to communicate in the same way it works and is in use already.  They want to pass to standard input what the password is.
Blocks: 711139
Comment on attachment 580907 [details] [diff] [review]
Support for signing and verifying MARs. v10.

I talked with bbondy today and he said he was going to submit a new patch so clearing reviews on this one.
Attachment #580907 - Flags: review?(robert.bugzilla)
Attachment #580907 - Flags: review?(bsmith)
Didn't get a chance to finish the review comments from bsmith.  Will get back on this tomorrow after doing some testing of what we will be pushing out to m-c.  This is not required for landing on m-c nor aurora, but should be done very soon after landing on aurora.
I used htohl and htonl by the way to be consistent with the rest of the code in this module, but I'll try to update all code in this module to use PR_htonl and friends.

> Check that offsetToIndex == ftell(fpSrc)

I can't check that because at this point fpSrc should be at the content which comes before the index.
Regarding ntohl it would cause a problem since we don't build NSPR as a host lib. I'm going to keep the ntohl instead.  This is consistent with the rest of the file.  If you would like that changed though please post a new bug with that so it can be changed module wide.
Implemented review comments.
Attachment #580907 - Attachment is obsolete: true
Attachment #582893 - Flags: review?(robert.bugzilla)
Attachment #582893 - Flags: review?(bsmith)
By the way, thank you bsmith for all of the review comments on this patch, I appreciate them.
bsmith do you think you will have a chance to do another pass on the patch in this bug before the new year?
(I ask because there are a lot of holidays and I'm not sure of your PTO schedule)
Yes, I will review the patch before Christmas.
Comment on attachment 582893 [details] [diff] [review]
Support for signing and verifying MARs. v11

Cancelling rs' review comments until the next round after bmsith's next review comments.
Attachment #582893 - Flags: review?(robert.bugzilla)
By the way if there is something that is not needed for this task please mention that it is not a security blocker and it can be done in a different task.  That will ensure faster landing of this work.
Comment on attachment 582893 [details] [diff] [review]
Support for signing and verifying MARs. v11

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

The most serious concerns I have are mentioned in the review of mar_verify.c. I recommend starting there first.

::: modules/libmar/sign/mar_sign.c
@@ +114,5 @@
> +    return -1;
> +  }
> +
> +  /* Check that the key length is large enough for our requirements */
> +  if (PK11_SignatureLen(*privKey) < XP_MIN_SIGNATURE_LEN_IN_BYTES) {

Use "if (*signatureLength < XP_MIN_SIGNATURE_LEN_IN_BYTES)"

@@ +320,5 @@
> +                           sizeof(signatureLength) +
> +                           signatureLength;
> +  dstOffsetToIndex = offsetToIndex;
> +  dstOffsetToIndex += signatureSectionLength;
> +  if (oldMar) {

Put this if statement above the "dstOffsetToIndex += signatureSectionLength;" statement, so that we're always adding the sizes of the components in the order they are added in the file.

@@ +385,5 @@
> +    goto failure;
> +  }
> +
> +  /* Write out the rest of the MAR excluding the index header and index */
> +  offsetToIndex -= ftell(fpSrc);

Here, offsetToIndex has changed meaning to "number of bytes to copy". This is very confusing. Instead of changing the meaning of offsetToIndex, please create a new variable "numBytesToCopy = offsetToIndex - ftell(fpSrc)" and use that new variable in the two lines below.

::: modules/libmar/sign/nss_secutil.c
@@ +33,5 @@
> + * the provisions above, a recipient may use your version of this file under
> + * the terms of any one of the MPL, the GPL or the LGPL.
> + *
> + * ***** END LICENSE BLOCK ***** */
> +

/* Copied from NSS's cmd/lib/secutil.h CVS revision <???> */

I did not review this file, on the assumption that it is a straight copy/paste of the NSS file, with some sections removed.

::: modules/libmar/sign/nss_secutil.h
@@ +33,5 @@
> + * the provisions above, a recipient may use your version of this file under
> + * the terms of any one of the MPL, the GPL or the LGPL.
> + *
> + * ***** END LICENSE BLOCK ***** */
> +

/* Copied from NSS's cmd/lib/secutil.h CVS revision <???> */

I did not review this file carefully, on the assumption that it is a straight copy/paste from the NSS file, with some sections removed.

**However**, why does this file define struct VFYContextStr? AFAICT, that shouldn't be necessary and it should be removed. We shouldn't be messing with the internal bits of the verify context and fields may differ between versions of NSS.

::: modules/libmar/src/mar_create.c
@@ +186,5 @@
>    if (fseek(fp, MAR_ID_SIZE, SEEK_SET))
>      goto failure;
>    if (fwrite(&offset_to_index, sizeof(offset_to_index), 1, fp) != 1)
>      goto failure;
> +  

Switch offset_to_index back to host byte order.

@@ +192,5 @@
> +                       stack.size_used +
> +                       sizeof(size_of_index);
> +  size_of_entire_MAR = HOST_TO_NETWORK64(size_of_entire_MAR);
> +  if (fwrite(&size_of_entire_MAR, sizeof(size_of_entire_MAR), 1, fp) != 1)
> +    goto failure;

Switch size_of_entire_MAR back to host byte order.

::: modules/libmar/src/mar_private.h
@@ +66,5 @@
> +  (((PRUint64) x) >> 56)
> +#define NETWORK_TO_HOST64 HOST_TO_NETWORK64
> +
> +/* The mar program is compiled as a host bin so we don't have
> +   access to NSPR since we don't build it that way.  For that

"...to NSPR at runtime..."

@@ +67,5 @@
> +#define NETWORK_TO_HOST64 HOST_TO_NETWORK64
> +
> +/* The mar program is compiled as a host bin so we don't have
> +   access to NSPR since we don't build it that way.  For that
> +   reason we use ntohl and htonl instead of the PR equialents. */

"...use ntohl and htonl and define HOST_TO_NETWORK64 instead of using the NSPR equivalents."

(Note the spelling correction.)

@@ +72,5 @@
> +#ifdef XP_WIN
> +#include <winsock2.h>
> +#else
> +#include <netinet/in.h>
> +#endif

Put this block (including the comment) ahead of the definition of HOST_TO_NETWORK64.

::: modules/libmar/tool/Makefile.in
@@ +66,5 @@
> +  $(DEPTH)/modules/libmar/sign/$(LIB_PREFIX)signmar.$(LIB_SUFFIX) \
> +  $(DEPTH)/modules/libmar/verify/$(LIB_PREFIX)verifymar.$(LIB_SUFFIX) \
> +  $(DIST)/lib/$(LIB_PREFIX)nss3.$(LIB_SUFFIX) \
> +  $(DIST)/lib/$(LIB_PREFIX)nssutil3.$(LIB_SUFFIX) \
> +  $(DIST)/lib/$(LIB_PREFIX)cryptohi.$(LIB_SUFFIX) \

You shouldn't need to link to cryptohi, since it is included in libnss3.so.

::: modules/libmar/verify/cryptox.c
@@ +54,5 @@
> +}
> +
> +CryptoX_Result NSS_LoadPublicKey(const char *certNickname, 
> +                                 SECKEYPublicKey **publicKey, 
> +                                 CERTCertificate **cert)

Will need to call CERT_DestroyCert and SECKEY_DestroyPublicKey when this returns SECSuccess.

@@ +57,5 @@
> +                                 SECKEYPublicKey **publicKey, 
> +                                 CERTCertificate **cert)
> +{
> +  secuPWData pwdata = { PW_NONE, 0 };
> +  if (!cert || !publicKey || !cert) {

We should decide whether these functions need to check for NULL parameters or not. This one does, but the next one doesn't. We should be consistent here.

@@ +67,5 @@
> +  if (!*cert) {
> +    return CryptoX_Error;
> +  }
> +  *publicKey = CERT_ExtractPublicKey(*cert);
> +  if (!*publicKey) {

CERT_DestroyCertificate(*cert);

@@ +73,5 @@
> +  }
> +  return CryptoX_Success;
> +}
> +
> +CryptoX_Result NSS_VerifyBegin(VFYContext **ctx, SECKEYPublicKey **publicKey)

VFYContext * const* ctx, SECKEYPublicKey * const * publicKey

@@ +78,5 @@
> +{
> +  SECStatus status;
> +
> +  /* Check that the key length is large enough for our requirements */
> +  if ((SECKEY_PublicKeyStrengthInBits(*publicKey) * 8) < 

SECKEY_PublicKeyStrength(*publicKey)

@@ +86,5 @@
> +    return CryptoX_Error;
> +  }
> +
> +  *ctx = VFY_CreateContext(*publicKey, NULL, 
> +                           SEC_OID_ISO_SHA1_WITH_RSA_SIGNATURE, NULL);

Check *ctx != NULL.

@@ +94,5 @@
> +}
> +
> +CryptoX_Result NSS_VerifySignature(VFYContext **ctx, 
> +                                   char *signature, 
> +                                   size_t signatureLen)

VFYContext * const * ctx,
const unsigned char * signature,
unsigned int signatureLen)

Note that NSS cannot take size_t as a parameter when sizeof(size_t)==sizeof(uint64_t) and sizeof(unsigned int) < sizeof(size_t) (i.e. on Win64), so we have to limit these parameters to unsigned int. (The same applies to CryptoAPI; the size parameters are DWORD, which is 32-bits even on Win64.)

Perhaps it is worth defining "typedef unsigned int CryptoX_Length"

@@ +99,5 @@
> +{
> +  SECItem signedItem;
> +  SECStatus status;
> +  signedItem.len = signatureLen;
> +  signedItem.data = (const unsigned char*)signature;

signedItem.data = (unsigned char*)signature;

(Unfortunately, the type of SECItem::data is not const.)

@@ +104,5 @@
> +  status = VFY_EndWithSignature(*ctx, &signedItem);
> +  return SECSuccess == status ? CryptoX_Success : CryptoX_Error;
> +}
> +
> +#elif defined(XP_WIN)

Define the CryptoAPI_* functions in the same order that the analogous NSS_* functions are defined, so that the two corresponding functions can more easily be compared side-by-side in the editor.

@@ +107,5 @@
> +
> +#elif defined(XP_WIN)
> +CryptoX_Result CyprtoAPI_VerifySignature(HCRYPTHASH *hash, 
> +                                         HCRYPTKEY *pubKey,
> +                                         char *signature, 

const unsigned char *signature,

@@ +111,5 @@
> +                                         char *signature, 
> +                                         DWORD signatureLen)
> +{
> +  size_t i;
> +  BOOL result;

/* Windows APIs expect the bytes in the signature to be
 * in little-endian order, but we right the signature in
 * big-endian order. (Other from other APIs like NSS and
 * OpenSSL expect the big-endian order.)
 */

@@ +112,5 @@
> +                                         DWORD signatureLen)
> +{
> +  size_t i;
> +  BOOL result;
> +  char *signatureReversed = malloc(signatureLen);

BYTE *signatureReversed = malloc(signatureLen);

Check that signatureReversed != NULL.

@@ +116,5 @@
> +  char *signatureReversed = malloc(signatureLen);
> +  for (i = 0; i < signatureLen; i++) {
> +    signatureReversed[i] = signature[signatureLen - 1 - i]; 
> +  }
> +  result = CryptVerifySignature(*hash, (BYTE*)signatureReversed,

No cast if you make the above change.

@@ +124,5 @@
> +}
> +
> +CryptoX_Result CryptoAPI_LoadPublicKey(HCRYPTPROV hProv, 
> +                                       const char *certData,
> +                                       size_t sizeOfCertData,

const BYTE *certData,
DWORD sizeOfCertData,

@@ +126,5 @@
> +CryptoX_Result CryptoAPI_LoadPublicKey(HCRYPTPROV hProv, 
> +                                       const char *certData,
> +                                       size_t sizeOfCertData,
> +                                       HCRYPTKEY *publicKey,
> +                                       HCERTSTORE *hCertStore)

"phCertStore" or "certStore".

(A variable with the "h" prefix should always be a handle, never a pointer to a handle.)

@@ +132,5 @@
> +  CRYPT_DATA_BLOB blob;
> +  CERT_CONTEXT *context;
> +
> +  blob.cbData = sizeOfCertData;
> +  blob.pbData = CryptMemAlloc(sizeOfCertData);

Doesn't it work to just set blob.pbData = certData? I think CryptMemAlloc is just needed for things that you pass to CryptoAPI and expect CryptoAPI to free.

@@ +159,5 @@
> +
> +CryptoX_Result CryptoAPI_InitCryptoContext(HCRYPTPROV *provider)
> +{
> +  BOOL result = TRUE;
> +  *provider = (HCRYPTPROV)NULL;

I think you can just remove this initialization.

Otherwise, should it be set to INVALID_HANDLE_VALUE (which is -1, not 0) instead?

@@ +160,5 @@
> +CryptoX_Result CryptoAPI_InitCryptoContext(HCRYPTPROV *provider)
> +{
> +  BOOL result = TRUE;
> +  *provider = (HCRYPTPROV)NULL;
> +  if (!CryptAcquireContext(provider, NULL, NULL, PROV_RSA_FULL, 0)) {

The last parameter should be set to CRYPT_VERIFYCONTEXT to indicate that we don't need access to the keystore.

It seems to me it would be better to try to explicitly request the enhanced provider, and then fall back to the default if acquiring the enhanced provider fails. That way, we become insensitive to the setting of the default provider whenever the enhanced provider is available, but if the enhanced provider is not available (for whatever reason) then we still have the potential of working.

@@ +163,5 @@
> +  *provider = (HCRYPTPROV)NULL;
> +  if (!CryptAcquireContext(provider, NULL, NULL, PROV_RSA_FULL, 0)) {
> +    if (NTE_BAD_KEYSET == GetLastError()) {
> +      /* Maybe it doesn't exist, try to create it. */
> +      result = CryptAcquireContext(provider, NULL, NULL, PROV_RSA_FULL,

AFAICT, when we use CRYPT_VERIFYCONTEXT, creating a key store shouldn't help us because the CSP should be able to operate without one. However, if you want to leave this in "just in case," I won't rage about it.

@@ +174,5 @@
> +CryptoX_Result CryptoAPI_VerifyBegin(HCRYPTPROV provider, HCRYPTHASH* hash) 
> +{
> +  *hash = (HCRYPTHASH)NULL;
> +  return CryptCreateHash(provider, CALG_SHA1,
> +                         0, 0, hash) ? CryptoX_Success : CryptoX_Error;

Use the style you used above, where the result is saved in a local variable, and then you return result ? CryptoX_Success : CryptoX_Error; this makes debugging easier.

@@ +180,5 @@
> +
> +CryptoX_Result CryptoAPI_VerifyUpdate(HCRYPTHASH* hash, char *buf, size_t len)
> +{
> +  return CryptHashData(*hash, (BYTE*)buf, len, 0) ? 
> +         CryptoX_Success : CryptoX_Error;

ditto.

::: modules/libmar/verify/cryptox.h
@@ +41,5 @@
> +#define XP_MIN_SIGNATURE_LEN_IN_BYTES 256
> +
> +#define CryptoX_Result int
> +#define CryptoX_Success 0
> +#define CryptoX_Error 1

-1, to be consistent with other parts of this patch.

@@ +42,5 @@
> +
> +#define CryptoX_Result int
> +#define CryptoX_Success 0
> +#define CryptoX_Error 1
> +#define CryptoX_Succeeded(X) (X == CryptoX_Success)

(X) instead of X.

@@ +43,5 @@
> +#define CryptoX_Result int
> +#define CryptoX_Success 0
> +#define CryptoX_Error 1
> +#define CryptoX_Succeeded(X) (X == CryptoX_Success)
> +#define CryptoX_Failed(X) (X != CryptoX_Success)

(X) instead of X.

@@ +47,5 @@
> +#define CryptoX_Failed(X) (X != CryptoX_Success)
> +
> +#if defined(MAR_NSS)
> +
> +#include "nss_secutil.h"

#include "cryptoht.h" instead.

Below, change the signatures as noted in the review comments for the cryptox.c

@@ +67,5 @@
> +#define CryptoX_SignatureHandle VFYContext *
> +#define CryptoX_PublicKey SECKEYPublicKey *
> +#define CryptoX_Certificate CERTCertificate *
> +#define CryptoX_InitCryptoLibrary(CryptoHandle, ConfigDir) \
> +  (*CryptoHandle = NULL), NSS_Init(ConfigDir)

Don't use the comma operator here. Instead, you can just initialize the handle to the invalid value (NULL for NSS, INVALID_HANDLE_VALUE for CryptoAPI) when it is declared. (Perhaps, define a new "CryptoX_InvalidHandle" for that value.)

@@ +85,5 @@
> +
> +#elif defined(XP_WIN) 
> +
> +#ifndef WIN32_LEAN_AND_MEAN
> +#define WIN32_LEAN_AND_MEAN

IRRC, a header shouldn't define WIN32_LEAN_AND_MEAN, because it has global effects. Instead, it should be defined at the top of *.c files.

@@ +95,5 @@
> +CryptoX_Result CryptoAPI_InitCryptoContext(HCRYPTPROV *provider);
> +CryptoX_Result CryptoAPI_VerifyBegin(HCRYPTPROV provider, HCRYPTHASH* hash);
> +CryptoX_Result CryptoAPI_VerifyUpdate(HCRYPTHASH* hash, 
> +                                      char *buf, size_t len);
> +CryptoX_Result CryptoAPI_LoadPublicKey(HCRYPTPROV hProv, 

VerifyBegin/Update/Signature should appear together, without LoadPublicKey in the middle. (It's a good idea to order them that way in the *.c file too.)

@@ +123,5 @@
> +  CyprtoAPI_VerifySignature(hash, publicKey, signed, len)
> +#define CryptoX_FreePublicKey(key) \
> +  CryptDestroyKey(*(key))
> +#define CryptoX_FreeCertificate(cert) \
> +  CertCloseStore(*cert, CERT_CLOSE_STORE_FORCE_FLAG);

*(cert)

@@ +125,5 @@
> +  CryptDestroyKey(*(key))
> +#define CryptoX_FreeCertificate(cert) \
> +  CertCloseStore(*cert, CERT_CLOSE_STORE_FORCE_FLAG);
> +
> +#else

Do we need to be able to build this on platforms without NSS and without CryptoAPI? If not, then:

#error "No CryptoX implementation available for this platform"

and delete the #defines below.

Otherwise, document why this dummy implementation is necessary.

::: modules/libmar/verify/mar_verify.c
@@ +44,5 @@
> +#include <string.h>
> +#include "mar.h"
> +#include "mar_private.h"
> +#include "cryptox.h"
> +#ifdef XP_WIN

This is already handled in mar_private.h

@@ +86,5 @@
> +    return 0;
> +  }
> +
> +  if (fread(buffer, size, 1, fp) != 1) {
> +    fprintf(stderr, "ERROR: Could not read %s\n", err);

Do you really want to write to stderr in these functions?

(I don't know how error logging works in updater.exe, so maybe this is OK.)

@@ +90,5 @@
> +    fprintf(stderr, "ERROR: Could not read %s\n", err);
> +    return -1;
> +  }
> +
> +  if (CryptoX_Failed(CryptoX_VerifyUpdate(ctx, (char*)buffer, size))) {

make sure the cast is correct.

@@ +97,5 @@
> +  }
> +  return 0;
> +}
> +
> +

The functions below should take a handle to an already-open file and should not close the file. That way, it is less likely that users of these functions will do the following:

    if (0 == mar_verify_signature(marFile, ....)) {
       fd = open(marFile, ...);
       // process file
    }

The problem with doing that is that the contents of the file might change between the first line and the second line (i.e. it is a race condition.) The correct pattern to avoid such race conditions is:

   fd = open(file, ...);
   if (0 == mar_verify_signature(fd, ...)) {
       seek(fd, ...);
       // process file
   }

Also, the functions below shouldn't call CryptoX_InitCryptoLibrary and consequently don't need to be passed in the config dir. CryptoX_InitCryptoLibrary should only be called once per program. (It might be OK to call it more than once in a program, but calling it more than once with different parameters would have surprising results as only the first call would have an effect.)

@@ +111,5 @@
> + * @return 0 on success
> +*/
> +int mar_verify_signature(const char *pathToMARFile, 
> +                         const char *certData,
> +                         size_t sizeOfCertData,

PRUint32 sizeOfCertData,

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

The above check for an old MAR is unnecessary. The code below can just assume it is a signed MAR and it will fail when things don't make sense when interpreted as a signed MAR file. (It is important that the code below already work correctly when things don't make sense anyway.)

@@ +244,5 @@
> +  }
> +
> +  /* Skip to the start of the signature block */
> +  if (fseek(fp, SIGNATURE_BLOCK_OFFSET, SEEK_SET)) {
> +    fprintf(stderr, "ERROR: Could not seek past signature block.\n");

*to the signature block*, not past it.

@@ +253,5 @@
> +  if (fread(&signatureCount, sizeof(signatureCount), 1, fp) != 1) {
> +    fprintf(stderr, "ERROR: Could not read number of signatures.\n");
> +    return -1;
> +  }
> +  signatureCount = ntohl(signatureCount);

We should limit signatureCount to a reasonable number. e.g. 8. Otherwise, it will be trivial to waste the updater's time with a MAR file with many (invalid) signatures.

Similarly, we should limit the size of the input file to something that isn't ridiculously large. Definitely we should restrict the size of the MAR file to something that is smaller than PR_INT32_MAX for now, to guard against any PRInt32 <-> PRUint32 <-> size_t <-> PRUint64 bugs that we didn't find. (I would suggest that 250MB should be a good hard limit for the MAR file size.)

@@ +254,5 @@
> +    fprintf(stderr, "ERROR: Could not read number of signatures.\n");
> +    return -1;
> +  }
> +  signatureCount = ntohl(signatureCount);
> +  for (i = 0; i < signatureCount; i++) {

Make the condition "i < signatureCount && numVerified == 0". We don't want to verify more than one signature, because signature verification takes a significant amount of time.

@@ +268,5 @@
> +      return -1;
> +    }
> +    signatureLen = ntohl(signatureLen);
> +
> +    extractedSignature = malloc(signatureLen);

I suggest you limit signatureLen to some reasonable value. e.g. 2048. Otherwise, an invalid/malicious file could have to allocate a huge amount of memory here, and then cause you lots of trouble with crash handling and/or out-of-memory errors that occur in ultra-low-available-memory situations.

We should document this added restriction in the MAR file format doc.

@@ +326,5 @@
> +int mar_verify_signature_for_id_fp(FILE *fp, 
> +                                   CryptoX_LibraryHandle provider, 
> +                                   CryptoX_PublicKey key, 
> +                                   char *extractedSignature,
> +                                   size_t extractedSignatureLen,

Don't use size_t here or elsewhere. Use PRUint32.

@@ +346,5 @@
> +     Bytes 4-7: index offset 
> +     Bytes 8-15: size of entire MAR
> +   */
> +  if (ReadAndUpdateVerifyContext(fp, buf, SIGNATURE_BLOCK_OFFSET, 
> +                                 &signatureHandle, "signature block")) {

CryptoX_VerifyUpdate?

@@ +355,5 @@
> +  if (ReadAndUpdateVerifyContext(fp, &signatureCount, sizeof(PRUint32), 
> +                                 &signatureHandle, "signature count")) {
> +    return -1;
> +  }
> +  signatureCount = ntohl(signatureCount);

signatureCount should just be passed as a parameter to this function, and we should just make the previous ReadAndUpdateVerifyContext call's 3rd parameter be:

    SIGNATURE_BLOCK_OFFSET + sizeof(PRUint32) /* signatureCount */

@@ +364,5 @@
> +                                   &signatureHandle, 
> +                                   "signature algorithm ID")) {
> +        return -1;
> +    }
> +    signatureAlgorithmID = ntohl(signatureAlgorithmID);

CryptoX_VerifyUpdate instead? (see the next comment).

@@ +372,5 @@
> +      return -1;
> +    }
> +    signatureLen = ntohl(signatureLen);
> +
> +    if (signatureAlgorithmIDToVerify == signatureAlgorithmID && 

This check isn't correct. There might be more than one signature with the same signature algorithm ID but with different keys. I think we should just remove this check and remove the signatureAlgorithmIDToVerify parameter.

@@ +415,5 @@
> +    return 0;
> +  } else {
> +    fprintf(stderr, "ERROR: Signature count is 0.\n");
> +    return -1;
> +  }

AFAICT, this function couldn't be called without at least one signature being present in the file because otherwise we wouldn't have a signature to verify above, so the above checks aren't necessary and should be replaced with just "return 0".
Attachment #582893 - Flags: review?(bsmith) → review-
(I am sorry I didn't review this patch last weekend. I had forgot to put it on my Christmas TODO list.)

One other general comment: I like the clarify that the CryptoX_Succeded(x)/CryptoX_Failed(x) macros provide. Especially in mar_verify, it got confusing to remember whether zero means failure or success. If you have time, please do something similar for functions defined in mar_verify.c.
No problem, thanks for the review!
 +CryptoX_Result NSS_LoadPublicKey(const char *certNickname, 
> +                                 SECKEYPublicKey **publicKey, 
> +                                 CERTCertificate **cert)
> Will need to call CERT_DestroyCert and SECKEY_DestroyPublicKey when this returns SECSuccess.


I do this already at the end of mar_verify_signature:

> if (key) {
    
>   CryptoX_FreePublicKey(&key);
  
> } 

  
> 
> if (cert) {
    
>   CryptoX_FreeCertificate(&cert);
  
> }
> Instead, you can just initialize the handle to the invalid 
> value (NULL for NSS, INVALID_HANDLE_VALUE for CryptoAPI) 

At least half of the Win32 APIs dealing with handles use NULL instead of INVALID_HANDLE_VALUE to indicate an invalid handle value.  I'm not sure what is more prevalent in CryptoAPI but in error conditions my code assumes NULL is the invalid handle value.
Also I make no assumption about invalid handle values returned from CryptoAPI so this should be acceptable.
> +  if (fread(buffer, size, 1, fp) != 1) {
> +    fprintf(stderr, "ERROR: Could not read %s\n", err);

> Do you really want to write to stderr in these functions?
>(I don't know how error logging works in updater.exe, so maybe this is OK.)

It will just be ignored on updater code. This error logging is more for the signmar program which we do want to write to stderr.
> +  if (oldMar) {
> +    fprintf(stderr, "ERROR: The MAR file is in the old format so has"
> +                    " no signature to verify.\n");
> +    return -1;
> +  }

> The above check for an old MAR is unnecessary. The code below can just assume it
> is a signed MAR and it will fail when things don't make sense when interpreted as 
> a signed MAR file. (It is important that the code below already work correctly 
> when things don't make sense anyway.)

I disagree with this comment so will not change it on this pass. If you have strong objections, please re-ask in the next review.  It is better to give the user of the signmar program an appropriate error message than to have some obscure error message.  Yes it will fail anyway, but it is better to have a logical error message that indicates what the error is.
I don't understand the following 3 comments, I think maybe you aren't clear on what ReadAndUpdateVerifyContext does.
It simply reads data from the file and updates the verify context using CryptoX_VerifyUpdate in one step.
We have a similar concept in the signmarfile with WriteAndUpdateSignature.

> > @@ +346,5 @@
> > +     Bytes 4-7: index offset 
> > +     Bytes 8-15: size of entire MAR
> > +   */
> > +  if (ReadAndUpdateVerifyContext(fp, buf, SIGNATURE_BLOCK_OFFSET, 
> > +                                 &signatureHandle, "signature block")) {

> CryptoX_VerifyUpdate?
> 
> @@ +355,5 @@
> > +  if (ReadAndUpdateVerifyContext(fp, &signatureCount, sizeof(PRUint32), 
> > +                                 &signatureHandle, "signature count")) {
> > +    return -1;
> > +  }
> > +  signatureCount = ntohl(signatureCount);
> 
> signatureCount should just be passed as a parameter to this function, and we 
> should just make the previous ReadAndUpdateVerifyContext call's 3rd parameter 
> be:
> 
>   SIGNATURE_BLOCK_OFFSET + sizeof(PRUint32) /* signatureCount */

> @@ +364,5 @@
> > +                                   &signatureHandle, 
> > +                                   "signature algorithm ID")) {
> > +        return -1;
> > +    }
> > +    signatureAlgorithmID = ntohl(signatureAlgorithmID);
> 
> CryptoX_VerifyUpdate instead? (see the next comment).
Question for bsmith:

Is it ok if I change SECU_GetPasswordString to use stdin instead of "/dev/tty" on XP_UNIX? 

catlee needs to automate passing a password in and cannot currently.  Alternately, if you know how to get this to work with a script as is then that would work too.
(In reply to Brian R. Bondy [:bbondy] from comment #170)
> Question for bsmith:
> 
> Is it ok if I change SECU_GetPasswordString to use stdin instead of
> "/dev/tty" on XP_UNIX? 
> 
> catlee needs to automate passing a password in and cannot currently. 
> Alternately, if you know how to get this to work with a script as is then
> that would work too.

I'm currently calling 'signmar' from within python using code like this:

proc = subprocess.Popen(['signmar', '-d', '.', ...], stdin=subprocess.PIPE)
proc.stdin.write(passphrase)
proc.stdin.close()
Review comments are implemented, just need to test more and check for warnings on different platforms.  I'll re-request a review once that is done.
(In reply to Brian R. Bondy [:bbondy] from comment #168)
> I disagree with this comment so will not change it on this pass. If you have
> strong objections, please re-ask in the next review.  It is better to give
> the user of the signmar program an appropriate error message than to have
> some obscure error message.  Yes it will fail anyway, but it is better to
> have a logical error message that indicates what the error is.

I do not understand. signmar already does a check to see if the input file is old or new, and handles each case differently. Maybe you mean that you want the command line utility that verifies signatures to be able to output a pretty error message? I think that makes sense. However, IMO that IsOldMAR check should be done in that program, instead of here in this library.

There are three reasons I think it is bad to put this check here: First, it is somewhat of a heuristic, and a complicated one. IMO, it is better to avoid heuristics whenever possible when dealing with security code, and instead do things that behave predictable. Second, if/when we (fuzz) test the verifier code, we may not be able to reach as many code paths with this check in place. Thirdly, the less code, the better.

IMO, a very good way for the MAR verifier to output its pretty error message is the following:
1. Attempt to verify the MAR file.
2. If verification succeeds, output "good" and stop.
3. Output "bad".
4. Call IsOldMAR on the input file
5. If IsOldMAR returned true, then additionally output "this appears to be a MAR file using the old file format."
(In reply to Brian R. Bondy [:bbondy] from comment #169)
> > > @@ +346,5 @@
> > > +     Bytes 4-7: index offset 
> > > +     Bytes 8-15: size of entire MAR
> > > +   */
> > > +  if (ReadAndUpdateVerifyContext(fp, buf, SIGNATURE_BLOCK_OFFSET, 
> > > +                                 &signatureHandle, "signature block")) {
> 
> > CryptoX_VerifyUpdate?

Ignore this one.

> > @@ +355,5 @@
> > > +  if (ReadAndUpdateVerifyContext(fp, &signatureCount, sizeof(PRUint32), 
> > > +                                 &signatureHandle, "signature count")) {
> > > +    return -1;
> > > +  }
> > > +  signatureCount = ntohl(signatureCount);
> > 
> > signatureCount should just be passed as a parameter to this function, and we 
> > should just make the previous ReadAndUpdateVerifyContext call's 3rd parameter 
> > be:
> > 
> >   SIGNATURE_BLOCK_OFFSET + sizeof(PRUint32) /* signatureCount */

We already "parsed" signatureCount in the caller. We should just pass that parsed value into this function. That way, we can check that signatureCount > 0 at the start of the function, which simplifies things as I noted in the review. The above review comment is saying that, if you pass signatureCount as a parameter, then you do not need to "parse" signatureCount into a variable again.

> 
> > @@ +364,5 @@
> > > +                                   &signatureHandle, 
> > > +                                   "signature algorithm ID")) {
> > > +        return -1;
> > > +    }
> > > +    signatureAlgorithmID = ntohl(signatureAlgorithmID);
> > 
> > CryptoX_VerifyUpdate instead? (see the next comment).

Similar to the previous comment: If you fix the problem noted in the "next comment", then you do not need the signatureAlgorithmID variable anymore; you can just read into an opaque (unsigned char) buffer of the appropriate size. Doing this makes the code clearer (with appropriate comments) because it becomes more obvious that we are not using the values of these fields for anything in this function.
(In reply to Brian R. Bondy [:bbondy] from comment #170)
> Question for bsmith:
> 
> Is it ok if I change SECU_GetPasswordString to use stdin instead of
> "/dev/tty" on XP_UNIX? 

SECU_GetPasswordString uses /dev/tty so it can prevent shoulder surfing. (The same reason browsers' password fields show passwords as "*********".) 

My suggestion is to exit with an error if stdin is a tty (to prevent shoulder surfing by preventing interactive input of the password), and then read the password from stdin, completely doing away with your copy of SECU_GetPasswordString.
> > I disagree with this comment so will not change it on this pass. If you have
> > strong objections, please re-ask in the next review.  It is better to give
> > the user of the signmar program an appropriate error message than to have
> > some obscure error message.  Yes it will fail anyway, but it is better to
> > have a logical error message that indicates what the error is.
> 
> I do not understand. signmar already does a check to see if the input file is
> old or new, and handles each case differently. Maybe you mean that you want the
> command line utility that verifies signatures to be able to output a pretty
> error message? I think that makes sense. However, IMO that IsOldMAR check
> should be done in that program, instead of here in this library.

signmar is the name of the program that can sign and verify depending on which command line parameters are passed in. So yes I meant verify code in particular and not signing.  

Yes I did also mean getting a pretty error message when verifying and an old mar is detected.  I am fine with putting that in the mar program instead of the library as you suggested and will do so.
- Implemented review comments.
- Compiles on Windows and Linux without warnings.
- Tested on Windows and Linux for signing / verifying.
Attachment #582893 - Attachment is obsolete: true
Attachment #586323 - Flags: review?(bsmith)
(In reply to Brian Smith (:bsmith) from comment #175)
> (In reply to Brian R. Bondy [:bbondy] from comment #170)
> > Question for bsmith:
> > 
> > Is it ok if I change SECU_GetPasswordString to use stdin instead of
> > "/dev/tty" on XP_UNIX? 
> 
> SECU_GetPasswordString uses /dev/tty so it can prevent shoulder surfing.
> (The same reason browsers' password fields show passwords as "*********".) 
> 
> My suggestion is to exit with an error if stdin is a tty (to prevent
> shoulder surfing by preventing interactive input of the password), and then
> read the password from stdin, completely doing away with your copy of
> SECU_GetPasswordString.

How about we read from the terminal by default to prevent shoulder surfing, and accept a command line switch to support reading from a file descriptor or stdin (similar to how gpg does it), e.g. --passphrase-fd 0.

Reading from the terminal is handy for local testing. Reading from stdin is useful for automation.
(In reply to Chris AtLee [:catlee] from comment #178)
> How about we read from the terminal by default to prevent shoulder surfing,
> and accept a command line switch to support reading from a file descriptor
> or stdin (similar to how gpg does it), e.g. --passphrase-fd 0.
> 
> Reading from the terminal is handy for local testing. Reading from stdin is
> useful for automation.

This is fine too. But, I don't think a command line parameter is necessary. The C++ standard _isatty or the POSIX standard isatty() can be used to determine if stdin is interactive, AFAICT.
I'm not sure I understand what to do if no command line parameter is needed.  For example if you are proposing no command line parameter why not just read from stdin always and if it's a terminal or not, it doesn't matter to us.  

I'm already using isatty in the current patch pending review by the way, which only allows input if stdin is not a terminal.  But this is what catlee wants changed.
bsmith: ping, please see Comment 180 for blocking question.  Also do you know when the next review will be?  Regardless of Comment 180 please go ahead with the review when you have time as Comment 180 will just affect a minor change in nss_secutil.c

catlee: any update on the different certs for each channel that I can embed?  I believe the patch you have now can be used with a small workaround of passing an extra newline when signing.
Comment on attachment 586323 [details] [diff] [review]
Support for signing and verifying MARs. v12.

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

The main concern I have is that ftell() might not return a valid result if the file is larger than the return type of ftell() can handle.

::: modules/libmar/sign/mar_sign.c
@@ +454,5 @@
> +    goto failure;
> +  }
> +
> +  /* To protect against invalid MAR files, we assumes that the MAR file 
> +   * size is less than or equal to MAX_SIZE_OF_FILE.

This comment is unclear. I think you are doing this check to ensure that you don't sign a file that is too large to be accepted by the verification function. It would be more clear to say something like that. Also, I would rename MAX_SIZE_OF_FILE to MAX_SIZE_OF_MAR_FILE.

::: modules/libmar/sign/nss_secutil.c
@@ +35,5 @@
> + *
> + * ***** END LICENSE BLOCK ***** */
> +
> +/* With the exception of GetPasswordString, this file was
> + * copied from NSS's cmd/lib/secutil.c hg revision 8f011395145e 

I did not review nss_secutil.* here. I will comment on the needed changes in the next comment.

::: modules/libmar/sign/nss_secutil.h
@@ +82,5 @@
> +char *
> +SECU_FilePasswd(PK11SlotInfo *slot, PRBool retry, void *arg);
> +char *
> +SECU_GetModulePassword(PK11SlotInfo *slot, PRBool retry, void *arg);
> +

We only need to declare the one function that we actually call from outside of nss_secutil.c here in this header.

::: modules/libmar/src/mar.h
@@ +61,5 @@
> +
> +struct MarFile_ {
> +  FILE *fp;
> +  MarItem *item_table[TABLESIZE];
> +};

Why do we pass a pointer to one of these to the verify function, instead of just making that function's parameter be a FILE *? I think it is better to make it FILE * unless we have to use items_table for something.

@@ +164,5 @@
> + *         a negative number if there was an error
> + *         a positive number if the signature does not verify
> + */
> +#ifdef XP_WIN
> +int mar_verify_signatureW(MarFile *mar, 

Why not just FILE *?

@@ +171,5 @@
> +#endif
> +/**
> + * Verifies the embedded signature of the specified file path.
> + * This is only used by the signmar program and only for the verify signature.
> + * Command line parameter.  This should not be used to verify a MAR that will

I don't understand "Command line parameter."

Also, document that this function prints the error message when verification fails (if it does); if it isn't supposed to print the error message, then it looks like the caller is missing an fprintf().

@@ +172,5 @@
> +/**
> + * Verifies the embedded signature of the specified file path.
> + * This is only used by the signmar program and only for the verify signature.
> + * Command line parameter.  This should not be used to verify a MAR that will
> + * be extracted in the same operation by updater code.

IMO, it is a good idea to move the declarations (if not the implementations) of is_old_mar and mar_verify_signature to a header like "mar_cmdline.h" or something, that should contain functions that are to be used only by the command line utilities and not to be included by the updater.

::: modules/libmar/src/mar_private.h
@@ +50,5 @@
>  #define ROUND_UP(n, incr) (((n) / (incr) + 1) * (incr))
>  
>  #define MAR_ID "MAR1"
>  #define MAR_ID_SIZE 4
> +#define SIGNATURE_BLOCK_OFFSET 16

Document how we got 16.

@@ +53,5 @@
>  #define MAR_ID_SIZE 4
> +#define SIGNATURE_BLOCK_OFFSET 16
> +
> +/* We have a MAX amount of signatures so that an invalid MAR will never
> + * waiste too much of either updater's or signmar's time.

waste

::: modules/libmar/src/mar_read.c
@@ +292,5 @@
> + * @param oldMar An out parameter specifying if the MAR file is new or old.
> + * @return A non-zero value if an error occurred and the information 
> +           cannot be determined.
> + */
> +int is_old_mar(const char *path, int *oldMar)

Need to compare this to previous implementation.

::: modules/libmar/tool/mar.c
@@ +166,5 @@
> +      print_usage();
> +      return -1;
> +    }
> +    /* If the mar program was built using CryptoAPI, then read in the buffer
> +       containing the cert. */

We should note somewhere that we do not verify the certificate in any way, for example:

* We do check anything to ensure that the certificate is for the correct product or channel.
* We do not check that the certificate was issued by any trusted authority. We assume it to be self-signed.
* We do not check whether the certificate is valid for this usage.

If/when we include product/channel information in the MAR file, we may be able to make at least some of these checks.

@@ +188,5 @@
> +    }
> +    CloseHandle(certFile);
> +
> +    /* If we compiled with CryptoAPI load the cert from disk.
> +       The path to the cert was passed in the -D parameter */

I believe this comment is supposed to be above CreateFileA.

::: modules/libmar/verify/cryptox.c
@@ +81,5 @@
> +NSS_VerifyBegin(VFYContext **ctx, 
> +                SECKEYPublicKey * const *publicKey)
> +{
> +  SECStatus status;
> +  if (!ctx || !publicKey) {

|| !*publicKey

@@ +118,5 @@
> +                    unsigned int signatureLen)
> +{
> +  SECItem signedItem;
> +  SECStatus status;
> +  if (!ctx || !signature) {

|| !*ctx

@@ +147,5 @@
> +{
> +  DWORD i;
> +  BOOL result;
> +/* Windows APIs expect the bytes in the signature to be
> + * in little-endian order, but we right the signature in

s/right/write/

@@ +148,5 @@
> +  DWORD i;
> +  BOOL result;
> +/* Windows APIs expect the bytes in the signature to be
> + * in little-endian order, but we right the signature in
> + * big-endian order. (Other from other APIs like NSS and

s/from other //

@@ +152,5 @@
> + * big-endian order. (Other from other APIs like NSS and
> + * OpenSSL expect the big-endian order.)
> + */
> +  BYTE *signatureReversed;
> +  if (!hash || !pubKey || !signature) {

|| signatureLen < 1

because you do "signatureLen - 1" below.

::: modules/libmar/verify/cryptox.h
@@ +41,5 @@
> +#define XP_MIN_SIGNATURE_LEN_IN_BYTES 256
> +
> +#define CryptoX_Result int
> +#define CryptoX_Success 0
> +#define CryptoX_Error -1

(-1)

@@ +47,5 @@
> +#define CryptoX_Failed(X) ((X) != CryptoX_Success)
> +
> +#if defined(MAR_NSS)
> +
> +/* Needed for SECKEYPublicKey, otherwise I'd use cryptoht.h */

SECKEYPublicKey is defined in NSS's "keythi.h".

(NSS types are usually defined in a header like XXXXt.h for the APIs defined in XXXX.h, or sometimes XXXXthi.h for APIs defined in XXXXhi.h.)

@@ +74,5 @@
> +#define CryptoX_LoadPublicKey(CryptoHandle, certData, dataSize, \
> +                              publicKey, certName, cert) \
> +  NSS_LoadPublicKey(certName, publicKey, cert)
> +#define CryptoX_VerifySignature(hash, publicKey, signed, len) \
> +  NSS_VerifySignature(hash, (const unsigned char *)signed, len)

s/signed/(signedData)/

It is confusing to use a C keyword like this. Also, are parentheses useful here? I can never remember the how the operator precedence of the casting operator might affect things if the parameter is an expression like "x + 5".

@@ +109,5 @@
> +  CryptoAPI_InitCryptoContext(CryptoHandle)
> +#define CryptoX_VerifyBegin(CryptoHandle, SignatureHandle, PublicKey) \
> +  CryptoAPI_VerifyBegin(CryptoHandle, SignatureHandle)
> +#define CryptoX_VerifyUpdate(SignatureHandle, buf, len) \
> +  CryptoAPI_VerifyUpdate(SignatureHandle, (BYTE *)buf, len)

(buf)

@@ +112,5 @@
> +#define CryptoX_VerifyUpdate(SignatureHandle, buf, len) \
> +  CryptoAPI_VerifyUpdate(SignatureHandle, (BYTE *)buf, len)
> +#define CryptoX_LoadPublicKey(CryptoHandle, certData, dataSize, \
> +                              publicKey, certName, cert) \
> +  CryptoAPI_LoadPublicKey(CryptoHandle, (BYTE*)certData, \

(certData)

::: modules/libmar/verify/mar_verify.c
@@ +108,5 @@
> + * @param  pathToMARFile The path of the MAR file who's signature 
> +                         should be calculated
> + * @param  certData      The certificate data
> + * @param sizeOfCertData The size of the data stored in certData
> + * @param configDir      The NSS config DIR if using NSS

Sync documentation with signature.

@@ +245,5 @@
> +  if (fseek(fp, 0, SEEK_END)) {
> +    fprintf(stderr, "ERROR: Could not seek to the end of the MAR file.\n");
> +    return CryptoX_Error;
> +  }
> +  if (ftell(fp) > MAX_SIZE_OF_FILE) {

What does ftell do if the file is so large that ftell cannot return the size accurately? (i.e. larger than 2GB on Windows.) It seems like we must use some kind of 64-bit function for this, or do something a little more clever.

Also, we should static assert that ((PRUint64) MAX_SIZE_OF_FILE) is less than ((PRUint64) LONG_MAX), or whatever constant represents the largest value that ftell() and friends can use/return.

@@ +264,5 @@
> +  }
> +  signatureCount = ntohl(signatureCount);
> +
> +  /* Check that we have less than the max amount of signatures so we don't
> +   * waiste too much of either updater's or signmar's time.

waste

@@ +385,5 @@
> +     Bytes 8-15: size of entire MAR
> +   */
> +  if (CryptoX_Failed(ReadAndUpdateVerifyContext(fp, buf, 
> +                                                SIGNATURE_BLOCK_OFFSET +
> +                                                sizeof(PRUint32),

static assert that SIGNATURE_BLOCK_OFFSET + sizeof(PRUint32) < sizeof buf.
(In reply to Brian R. Bondy [:bbondy] from comment #180)
> I'm not sure I understand what to do if no command line parameter is needed.
> For example if you are proposing no command line parameter why not just read
> from stdin always and if it's a terminal or not, it doesn't matter to us.  
> 
> I'm already using isatty in the current patch pending review by the way,
> which only allows input if stdin is not a terminal.  But this is what catlee
> wants changed.

Just do:

    if (!isatty(x)) {
        fgets(...);
        return
    }

before the code that opens the console is executed.

Also, isatty() is available on Windows in io.h.

(I am surprised that the NSS function doesn't already work this way.)
Regarding the large file support, see
http://coding.derkeiler.com/Archive/C_CPP/comp.lang.c/2006-12/msg03560.html

It seems pretty straightforward to change.
In Comment 183 bmsith said:
> Just do:
> if (!isatty(x)) {
>   fgets(...);
>   return;
> }

Currently in this patch I do:

> #ifndef _WINDOWS
> if (isatty(fileno(stdin))) {
>   fprintf(stderr, "Using console for input is not allowed.\n");
>   return NULL;
> }
> #else 
>   fprintf(stdout, "Please enter your password:\n");
>   fflush(stdout);
> #endif
> 
> QUIET_FGETS (phrase, sizeof(phrase), stdin);

Sorry but I still don't understand exactly what you want me to do.  
Are you saying I would no longer call isatty on stdin as I'm doing and instead call it on me manually opening a console?  
Also what file handle would I use in fgets in your suggestion.
If stdin is not a TTY, then just return fgets(stdin,...);
Otherwise, do what was being done before.

But, I am surprised because the existing code already seemed to do that. I will figure out what is going on.
> > +struct MarFile_ {
> > +  FILE *fp;
> > +  MarItem *item_table[TABLESIZE];
> > +};
> 
> Why do we pass a pointer to one of these to the verify function, instead of just > making that function's parameter be a FILE *? I think it is better to make it 
> FILE * unless we have to use items_table for something.

Because every function used in updater code takes in a MarFile and not a FILE*.  For consistency I believe it to be cleaner.
> > +int is_old_mar(const char *path, int *oldMar)
> Need to compare this to previous implementation.

You need me to or you want to? I don't think the implementation has changed.
> We should assert that: 
> ((PRUint64) MAX_SIZE_OF_FILE) is less than ((PRUint64) LONG_MAX)

I'm going to do this instead of trying to find a portable 64bit ftell for now.  This should cover the case of the new code, and all of the existing code is already not working with 64bit files.  With the other security checks in place we will never be touching a MAR file that is not signed by us anyway.  And I don't think we'll be ever signing a MAR file which is > LONG_MAX.  If you still want this though please consider a new bug which is not a P1.
(In reply to Brian R. Bondy [:bbondy] from comment #189)
> With the other security checks in place we will never be touching a MAR
> file that is not signed by us anyway.

In the verifier code, the size of the file is checked before any signature has been verified. So, we cannot rely on the fact that the file is signed by us at the time we are checking the size of the file. I do not think that ftell()/fread()/fseek()--especially fseek(...,SEEK_END)--have well-defined behavior on all platform in the case that the file is larger than 2GB. So, I think you must use fseeko/ftello or _fseeki64/_ftelli64 exclusively, at least up to the point that you verify the length of the file is less than the maximum.
> If stdin is not a TTY, then just return fgets(stdin,...);
> Otherwise, do what was being done before.
> 
> But, I am surprised because the existing code already seemed to do that. 
> I will figure out what is going on.

The existing code doesn't do that.
But I have this fixed already in the patch coming soon.

Existing code which doesn't do what you suggested:

> char *
> SECU_GetPasswordString(void *arg, char *prompt) 
> {
> #ifndef _WINDOWS
> char *p = NULL;
> FILE *input, *output;
>  
> /* open terminal */
> input = fopen(consoleName, "r");
> if (input == NULL) {
> fprintf(stderr, "Error opening input terminal for read\n");
> return NULL;
> }
Implemented review comments.
Attachment #586323 - Attachment is obsolete: true
Attachment #586323 - Flags: review?(bsmith)
Attachment #587710 - Flags: review?(bsmith)
Whiteboard: [sg:critical] (with a privileged service) [rat:bsterne] → [sg:critical] (with a privileged service) [rat:dveditz]
Had 2 sizeof(num_signatures), one of them should have been sizeof(offset_to_index).  No logic change, just looked wrong.
Attachment #587710 - Attachment is obsolete: true
Attachment #587710 - Flags: review?(bsmith)
Attachment #588511 - Flags: review?(bsmith)
bsmith do you have a time estimation on the next review? It's been a week since the review comments were implemented and I want to make sure we keep on top of this task since it also has to go through rstrong once you are done the review.  Thanks in advance.
Blocks: 720688
bsmith, do you feel comfortable enough with the security / crypto aspects of the last patch enough to pass the rest of the review onto rstrong? rstrong needs to do a review on this patch anyway.
Comment on attachment 588511 [details] [diff] [review]
Support for signing and verifying MARs. v14.

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

LGTM. Two comments below. Please also have somebody from your team superreview this.

::: modules/libmar/sign/nss_secutil.c
@@ +131,5 @@
> +#endif
> +
> +  /* Strip off the newlines if present */
> +  if (phrase[PORT_Strlen(phrase)-1] == '\n' || 
> +      phrase[PORT_Strlen(phrase)-1] == '\r') {

What if the EOL is \r\n? You would have to strip off two characters, not just one. I don't know if this could happen though.

::: modules/libmar/src/mar_private.h
@@ +65,5 @@
> +   invalid MAR files. */
> +#define MAX_SIZE_OF_MAR_FILE ((PRInt64)524288000)
> +
> +/* We use ftell throughout the code, so until we upgrade to a 64-bit variation
> +   we make this assertion instead. */

This comment is no longer accurate, right?
Attachment #588511 - Flags: review?(bsmith) → review+
> What if the EOL is \r\n? You would have to strip off two characters, 
> not just one. I don't know if this could happen though.

QUIET_FGETS has the following check:

> if (!c || c == '\n' || c == '\r')
>   break;

Inside a loop with getch, so it can never be ended with both \r\n.


> This comment is no longer accurate, right?

Right I'll update that and request an sr from rstrong.
The previous comment was accurate since we still do make assumptions in the MAR code about 32-bit, just not in the newer MAR signing / verifying code.

Updated the previous comment to be more accurate.
Attachment #588511 - Attachment is obsolete: true
Attachment #591438 - Flags: superreview?(robert.bugzilla)
Attachment #591438 - Flags: review+
Blocks: 725180
Comment on attachment 576498 [details] [diff] [review]
New MAR file format wiki source. v5.

This seems reasonable to me. Somebody else from the updater team should also do a review. (Sorry this one slipped through the cracks.)
Attachment #576498 - Flags: review?(bsmith) → review+
> This seems reasonable to me. Somebody else from the updater team should 
> also do a review.

Emailed rs to go over the overall documentation at:
https://wiki.mozilla.org/Software_Update:MAR
Depends on: 728301
Depends on: 728935
Whiteboard: [sg:critical] (with a privileged service) [rat:dveditz] → [sg:critical] (with a privileged service) [secr:dveditz]
Attachment #591438 - Flags: superreview?(robert.bugzilla)
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
Depends on: 730618
Target Milestone: mozilla13 → mozilla12
Whiteboard: [sg:critical] (with a privileged service) [secr:dveditz] → [sg:critical][qa-] (with a privileged service) [secr:dveditz]
Whiteboard: [sg:critical][qa-] (with a privileged service) [secr:dveditz] → [sg:critical][qa-] (with a privileged service) [sec-assigned:dveditz]
Could I be added to bug 750462? I think it is probably just a security review bug for this task? I think it would be good to include the same CC list if it is marked as security sensitive.
(In reply to Brian R. Bondy [:bbondy] from comment #203)
> Could I be added to bug 750462? I think it is probably just a security
> review bug for this task? I think it would be good to include the same CC
> list if it is marked as security sensitive.

(done)
Group: core-security
Flags: sec-review?(dveditz)
Flags: sec-review?(dveditz)
You need to log in before you can comment on or make changes to this bug.