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)
Toolkit
Application Update
Tracking
()
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.
Assignee | ||
Updated•13 years ago
|
OS: Windows 7 → All
Hardware: x86_64 → All
Comment 1•13 years ago
|
||
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.
Comment 2•13 years ago
|
||
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?
Assignee | ||
Comment 3•13 years ago
|
||
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.
Comment 4•13 years ago
|
||
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).
Comment 5•13 years ago
|
||
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.
Comment 6•13 years ago
|
||
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.
Comment 7•13 years ago
|
||
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.
Assignee | ||
Comment 8•13 years ago
|
||
> ...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.
Comment 9•13 years ago
|
||
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.
Comment 10•13 years ago
|
||
(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?
Assignee | ||
Comment 11•13 years ago
|
||
> 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.
Comment 12•13 years ago
|
||
(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.
Assignee | ||
Comment 13•13 years ago
|
||
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.
Comment 14•13 years ago
|
||
(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.
Comment 15•13 years ago
|
||
(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.
Updated•13 years ago
|
Whiteboard: [sg:critical] (with a privileged service)
Updated•13 years ago
|
status-firefox10:
--- → affected
status-firefox11:
--- → affected
status-firefox8:
--- → wontfix
status-firefox9:
--- → wontfix
tracking-firefox10:
--- → +
tracking-firefox11:
--- → +
tracking-firefox8:
--- → -
tracking-firefox9:
--- → -
Assignee | ||
Comment 16•13 years ago
|
||
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.
Assignee | ||
Updated•13 years ago
|
Attachment #574957 -
Attachment description: Intermediate patch for MAR file signing. Details in → Intermediate patch for MAR file signing. Details in Comment 16
Assignee | ||
Comment 17•13 years ago
|
||
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])
Comment 18•13 years ago
|
||
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.
Assignee | ||
Comment 19•13 years ago
|
||
- 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
Comment 20•13 years ago
|
||
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?
Updated•13 years ago
|
Keywords: sec-review-needed
Assignee | ||
Comment 21•13 years ago
|
||
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.
Comment 22•13 years ago
|
||
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.)
Comment 23•13 years ago
|
||
s/That means probably using NSS to verify the signatures/That means using the copy of NSS that Firefox bundles to verify the signatures/
Comment 24•13 years ago
|
||
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?
Comment 25•13 years ago
|
||
(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.
Assignee | ||
Comment 26•13 years ago
|
||
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.
Assignee | ||
Comment 27•13 years ago
|
||
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.
Comment 28•13 years ago
|
||
(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.
Assignee | ||
Comment 29•13 years ago
|
||
What about using a utility instead of an API that exists on the platform or that we can ship?
Comment 30•13 years ago
|
||
(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.
Comment 31•13 years ago
|
||
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
Comment 32•13 years ago
|
||
Brian Smith that is
Assignee | ||
Comment 33•13 years ago
|
||
would making copies of only what I need from NSS be a better approach?
Comment 34•13 years ago
|
||
(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.
Assignee | ||
Comment 35•13 years ago
|
||
> 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.
Comment 36•13 years ago
|
||
(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.
Comment 37•13 years ago
|
||
(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
Comment 38•13 years ago
|
||
We could include both a SHA-1 and a SHA-2 signature
Assignee | ||
Comment 39•13 years ago
|
||
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?
Comment 40•13 years ago
|
||
(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.)
Comment 41•13 years ago
|
||
(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.
Comment 42•13 years ago
|
||
(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.
Comment 43•13 years ago
|
||
(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.
Comment 44•13 years ago
|
||
(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.
Assignee | ||
Comment 45•13 years ago
|
||
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.
Comment 46•13 years ago
|
||
(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.
Comment 47•13 years ago
|
||
(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.
Assignee | ||
Comment 48•13 years ago
|
||
> 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?
Comment 49•13 years ago
|
||
(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.
Assignee | ||
Comment 50•13 years ago
|
||
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?
Assignee | ||
Comment 51•13 years ago
|
||
Thanks for the links by the way I'll look into them tomorrow.
Comment 52•13 years ago
|
||
(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.
Comment 53•13 years ago
|
||
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.
Comment 54•13 years ago
|
||
(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?
Comment 55•13 years ago
|
||
(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
Comment 56•13 years ago
|
||
(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?
Comment 57•13 years ago
|
||
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.
Comment 58•13 years ago
|
||
(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?
Comment 59•13 years ago
|
||
(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.
Assignee | ||
Comment 60•13 years ago
|
||
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.
Assignee | ||
Comment 61•13 years ago
|
||
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
Assignee | ||
Comment 62•13 years ago
|
||
By the way in the final version whether you use CyrptoAPI or NSS #defines it'll generate the same hash and same signature.
Assignee | ||
Comment 63•13 years ago
|
||
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
Comment 64•13 years ago
|
||
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
Comment 65•13 years ago
|
||
(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.
Assignee | ||
Comment 66•13 years ago
|
||
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]; }
Comment 67•13 years ago
|
||
(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.
Assignee | ||
Comment 68•13 years ago
|
||
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
Comment 69•13 years ago
|
||
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.
Assignee | ||
Comment 70•13 years ago
|
||
> 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.
Assignee | ||
Comment 71•13 years ago
|
||
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 :)
Comment 72•13 years ago
|
||
I'm fine with leaving it with -rsa if you think it provides value.
Assignee | ||
Comment 73•13 years ago
|
||
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.
Comment 74•13 years ago
|
||
(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).
Comment 75•13 years ago
|
||
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.
Comment 76•13 years ago
|
||
(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.)
Comment 77•13 years ago
|
||
(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?
Comment 78•13 years ago
|
||
No spec as of yet since this was a last minute addition. One will be available prior to the security review.
Assignee | ||
Updated•13 years ago
|
Assignee | ||
Comment 79•13 years ago
|
||
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.
Assignee | ||
Comment 80•13 years ago
|
||
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.
Assignee | ||
Comment 81•13 years ago
|
||
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
Assignee | ||
Comment 82•13 years ago
|
||
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
Comment 83•13 years ago
|
||
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?
Comment 84•13 years ago
|
||
dveditz - could you answer comment #83? Thanks
Assignee | ||
Comment 85•13 years ago
|
||
> 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.
Comment 86•13 years ago
|
||
(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.)
Comment 87•13 years ago
|
||
(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.
Assignee | ||
Comment 88•13 years ago
|
||
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)
Assignee | ||
Comment 89•13 years ago
|
||
I should also mention that this document is also ready for re-review: https://wiki.mozilla.org/Software_Update:MAR
Assignee | ||
Updated•13 years ago
|
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
Assignee | ||
Comment 90•13 years ago
|
||
Attachment #576151 -
Flags: review?(bsmith)
Assignee | ||
Comment 91•13 years ago
|
||
Attachment #576153 -
Flags: review?(bsmith)
Assignee | ||
Updated•13 years ago
|
Attachment #576151 -
Attachment description: Original wiki file format → Original MAR file format wiki source
Assignee | ||
Comment 92•13 years ago
|
||
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.
Assignee | ||
Comment 93•13 years ago
|
||
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)
Updated•13 years ago
|
Attachment #576060 -
Flags: review?(imelven)
Updated•13 years ago
|
Attachment #576151 -
Flags: review?(imelven)
Updated•13 years ago
|
Attachment #576156 -
Flags: review?(imelven)
Comment 94•13 years ago
|
||
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] ?
Assignee | ||
Comment 95•13 years ago
|
||
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.
Assignee | ||
Updated•13 years ago
|
Attachment #576151 -
Flags: review?(imelven)
Attachment #576151 -
Flags: review?(bsmith)
Comment 96•13 years ago
|
||
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+
Assignee | ||
Comment 97•13 years ago
|
||
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.
Comment 98•13 years ago
|
||
(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.
Assignee | ||
Comment 99•13 years ago
|
||
> All the bytes of the MAR file except the SIGNATURES.SIGNATURE_ENTRY.Signature fields.
that works, will update shortly.
Comment 100•13 years ago
|
||
(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 101•13 years ago
|
||
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 102•13 years ago
|
||
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).
Assignee | ||
Comment 103•13 years ago
|
||
> 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.
Assignee | ||
Comment 104•13 years ago
|
||
> 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.
Assignee | ||
Comment 105•13 years ago
|
||
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 106•13 years ago
|
||
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+
Assignee | ||
Comment 107•13 years ago
|
||
> 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?
Assignee | ||
Comment 108•13 years ago
|
||
> 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.
Updated•13 years ago
|
Whiteboard: [sg:critical] (with a privileged service) → [sg:critical] (with a privileged service) [rat:bsterne]
Comment 109•13 years ago
|
||
(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.
Assignee | ||
Comment 110•13 years ago
|
||
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)
Assignee | ||
Comment 111•13 years ago
|
||
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)
Assignee | ||
Comment 112•13 years ago
|
||
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)
Assignee | ||
Comment 113•13 years ago
|
||
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.
Assignee | ||
Comment 114•13 years ago
|
||
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)
Comment 115•13 years ago
|
||
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.
Assignee | ||
Comment 116•13 years ago
|
||
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).
Assignee | ||
Comment 117•13 years ago
|
||
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
Comment 118•13 years ago
|
||
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.
Assignee | ||
Comment 119•13 years ago
|
||
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.
Assignee | ||
Comment 120•13 years ago
|
||
- 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)
Assignee | ||
Comment 121•13 years ago
|
||
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.
Assignee | ||
Comment 122•13 years ago
|
||
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)
Assignee | ||
Comment 123•13 years ago
|
||
Tested on both Windows and Linux btw.
Comment 124•13 years ago
|
||
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 125•13 years ago
|
||
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+
Assignee | ||
Comment 126•13 years ago
|
||
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)
Assignee | ||
Comment 127•13 years ago
|
||
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 128•13 years ago
|
||
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+
Assignee | ||
Comment 129•13 years ago
|
||
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.
Comment 130•13 years ago
|
||
(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."
Comment 131•13 years ago
|
||
(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.
Assignee | ||
Comment 132•13 years ago
|
||
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?
Assignee | ||
Comment 133•13 years ago
|
||
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.
Comment 134•13 years ago
|
||
(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.
Assignee | ||
Comment 135•13 years ago
|
||
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)
Assignee | ||
Comment 136•13 years ago
|
||
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)
Assignee | ||
Comment 137•13 years ago
|
||
v7 also adds error logging to every failure condition for sign and verify operations.
Assignee | ||
Comment 138•13 years ago
|
||
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 139•13 years ago
|
||
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.
Comment 140•13 years ago
|
||
(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.
Comment 141•13 years ago
|
||
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.
Assignee | ||
Comment 142•13 years ago
|
||
> 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.
Assignee | ||
Comment 143•13 years ago
|
||
> 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).
Assignee | ||
Comment 144•13 years ago
|
||
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);
Assignee | ||
Comment 145•12 years ago
|
||
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)
Assignee | ||
Comment 146•12 years ago
|
||
This is pushed to elm by the way. I will update elm as review comments come in. bsmith & rs, any update on this review?
Assignee | ||
Comment 147•12 years ago
|
||
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 148•12 years ago
|
||
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.
Assignee | ||
Comment 149•12 years ago
|
||
> 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.
Comment 150•12 years ago
|
||
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)
Assignee | ||
Comment 151•12 years ago
|
||
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.
Assignee | ||
Comment 152•12 years ago
|
||
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.
Assignee | ||
Comment 153•12 years ago
|
||
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.
Assignee | ||
Comment 154•12 years ago
|
||
Implemented review comments.
Attachment #580907 -
Attachment is obsolete: true
Attachment #582893 -
Flags: review?(robert.bugzilla)
Attachment #582893 -
Flags: review?(bsmith)
Assignee | ||
Comment 155•12 years ago
|
||
By the way, thank you bsmith for all of the review comments on this patch, I appreciate them.
Assignee | ||
Comment 156•12 years ago
|
||
bsmith do you think you will have a chance to do another pass on the patch in this bug before the new year?
Assignee | ||
Comment 157•12 years ago
|
||
(I ask because there are a lot of holidays and I'm not sure of your PTO schedule)
Comment 158•12 years ago
|
||
Yes, I will review the patch before Christmas.
Assignee | ||
Comment 159•12 years ago
|
||
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)
Assignee | ||
Comment 160•12 years ago
|
||
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 161•12 years ago
|
||
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-
Comment 162•12 years ago
|
||
(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.
Assignee | ||
Comment 163•12 years ago
|
||
No problem, thanks for the review!
Assignee | ||
Comment 164•12 years ago
|
||
+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); > }
Assignee | ||
Comment 165•12 years ago
|
||
> 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.
Assignee | ||
Comment 166•12 years ago
|
||
Also I make no assumption about invalid handle values returned from CryptoAPI so this should be acceptable.
Assignee | ||
Comment 167•12 years ago
|
||
> + 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.
Assignee | ||
Comment 168•12 years ago
|
||
> + 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.
Assignee | ||
Comment 169•12 years ago
|
||
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).
Assignee | ||
Comment 170•12 years ago
|
||
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.
Comment 171•12 years ago
|
||
(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()
Assignee | ||
Comment 172•12 years ago
|
||
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.
Comment 173•12 years ago
|
||
(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."
Comment 174•12 years ago
|
||
(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.
Comment 175•12 years ago
|
||
(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.
Assignee | ||
Comment 176•12 years ago
|
||
> > 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.
Updated•12 years ago
|
status-firefox12:
--- → affected
tracking-firefox12:
--- → +
Assignee | ||
Comment 177•12 years ago
|
||
- 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)
Comment 178•12 years ago
|
||
(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.
Comment 179•12 years ago
|
||
(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.
Assignee | ||
Comment 180•12 years ago
|
||
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.
Assignee | ||
Comment 181•12 years ago
|
||
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 182•12 years ago
|
||
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.
Comment 183•12 years ago
|
||
(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.)
Comment 184•12 years ago
|
||
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.
Assignee | ||
Comment 185•12 years ago
|
||
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.
Comment 186•12 years ago
|
||
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.
Assignee | ||
Comment 187•12 years ago
|
||
> > +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.
Assignee | ||
Comment 188•12 years ago
|
||
> > +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.
Assignee | ||
Comment 189•12 years ago
|
||
> 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.
Comment 190•12 years ago
|
||
(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.
Assignee | ||
Comment 191•12 years ago
|
||
> 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; > }
Assignee | ||
Comment 192•12 years ago
|
||
Implemented review comments.
Attachment #586323 -
Attachment is obsolete: true
Attachment #586323 -
Flags: review?(bsmith)
Attachment #587710 -
Flags: review?(bsmith)
Updated•12 years ago
|
Whiteboard: [sg:critical] (with a privileged service) [rat:bsterne] → [sg:critical] (with a privileged service) [rat:dveditz]
Assignee | ||
Comment 193•12 years ago
|
||
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)
Assignee | ||
Comment 194•12 years ago
|
||
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.
Assignee | ||
Comment 195•12 years ago
|
||
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 196•12 years ago
|
||
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+
Assignee | ||
Comment 197•12 years ago
|
||
> 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.
Assignee | ||
Comment 198•12 years ago
|
||
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+
Comment 199•12 years ago
|
||
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+
Assignee | ||
Comment 200•12 years ago
|
||
> 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
Updated•12 years ago
|
Whiteboard: [sg:critical] (with a privileged service) [rat:dveditz] → [sg:critical] (with a privileged service) [secr:dveditz]
Assignee | ||
Updated•12 years ago
|
Attachment #591438 -
Flags: superreview?(robert.bugzilla)
Assignee | ||
Updated•12 years ago
|
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
Assignee | ||
Comment 201•12 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/033ca7c5bce8
Assignee | ||
Comment 202•12 years ago
|
||
http://hg.mozilla.org/releases/mozilla-aurora/rev/18840d884c97
Assignee | ||
Updated•12 years ago
|
Target Milestone: mozilla13 → mozilla12
Updated•12 years ago
|
status-firefox13:
--- → fixed
tracking-firefox13:
--- → +
Updated•12 years ago
|
status-firefox-esr10:
--- → wontfix
Whiteboard: [sg:critical] (with a privileged service) [secr:dveditz] → [sg:critical][qa-] (with a privileged service) [secr:dveditz]
Updated•12 years ago
|
Whiteboard: [sg:critical][qa-] (with a privileged service) [secr:dveditz] → [sg:critical][qa-] (with a privileged service) [sec-assigned:dveditz]
Assignee | ||
Comment 203•12 years ago
|
||
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.
Comment 204•12 years ago
|
||
(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)
Updated•12 years ago
|
Group: core-security
Updated•12 years ago
|
Flags: sec-review?(dveditz)
Updated•6 years ago
|
Flags: sec-review?(dveditz)
You need to log in
before you can comment on or make changes to this bug.
Description
•