792452 - MAR changes to embed multiple signatures (includes only libmar work not updater B2G specific work)

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect, P1)

Description

[Marshall Culpepper [:marshall_law]]

We will need to allow OEMs and Carriers to sign update MARs with their signatures along with the standard Mozilla signature. The MAR format currently supports up to 8 signatures in the embedded signature block, but there are some minor changes necessary to get the "signmar" tool to support this.

For details, See Brian Bondy's brain dump in b2g-fota-updates:
https://bugzilla.mozilla.org/show_bug.cgi?id=778084#c6

Comment 1

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

Brian Bondy made the suggestion that we should change the MAR spec so that signature X convers signature X-1. Whether or not this is good depends on the semantics we want to have for multi-party signatures. Do we want to say "Carrier X signed this, and THEN Mozilla signed it, and THEN operator Y signed it"? Or, do we just want to say "Mozilla signed it, carrier X signed it, and operator Y signed it, in some order that doesn't matter?" I think we only want the latter, but the idea that later signatures cover earlier signatures implies the former.

One thing I don't like about the later signatures covering the earlier signatures is that the earlier signatures are "weaker" in some sense, since they cover less of the file. I think having symmetry in what each signature signs makes the design easier to understand, and I don't think it is significantly more tedious to implement it that way.

Comment 2

[Brian R. Bondy [:bbondy]]

Everyone needs to know about everyone else for the more secure signing as you mentioned. I think that complicates both command line usage and also release semantics across different organizations that would want to sign.  My personal preference is to "change the MAR spec so that signature X convers signature X-1".

I think that:
"Carrier X signed this, and THEN Mozilla signed it, and THEN operator Y signed it"
Is more simple, and still is secure, and that's all we need to claim about it.

I think maybe it seems more scary because the signature block doesn't appear at the very start of the file, but it's the same thing.

Comment 3

[Brian R. Bondy [:bbondy]]

>  The MAR format currently supports up to 8 signatures

Note that this is just an artificial limit, put in place because everything should be capped in some way.  You can make this number larger if you'd like.

Comment 4

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

The multiple signature support in the MAR file was originally designed for a single organization to sign the MAR using multiple keys at the same time; that is, the signatures have "OR" semantics, not "AND" semantics. IIRC, the currently-deployed (in Firefox) MAR verification code will fail to verify anything except the first signature if we use the "change the MAR spec so that signature X convers signature X-1" strategy. And, AFAICT, the only way to sign a MAR with multiple signatures is to add all of those signatures at the same time, not incrementally in multiple executions of signmar.

I agree with Brian Bondy that it is OK to change the signing semantics to what he suggests. But, we should then consider this version 2 of the file format, and change the file type identifier in the header to "MAR2", and update the spec. And, we need to make sure we do not send MAR2 files to Firefox versions (as part of Firefox update) that only support MAR1. (The safest way to do this is to have the signmar program output MAR1 files for the first signature, and then MAR2 files for the second signature.)

An alternate way of doing this would be to simply nest a MAR inside a MAR inside a MAR, where each level of MAR is signed by a different organization. I don't know if this is a *better* alternative. It would be approximately the same amount of overhead, and it would avoid the need to changing any of the MAR tools, and it would allow each organization to provide backup keys like the original MAR specification did. Brian Bondy probably understands the downsides to this idea better than I do.

Comment 5

[Brian R. Bondy [:bbondy]]

(In reply to Brian Smith (:bsmith) from comment #4)
> that is, the signatures have "OR" semantics, not "AND" semantics. IIRC, the

Ya I called this out in my previous post.

> currently-deployed (in Firefox) MAR verification code will fail to verify
> anything except the first signature if we use the "change the MAR spec so
> that signature X convers signature X-1" strategy. 

We only have deployed MARs with 1 signature, since we don't even have a tool for doing 2 signatures. So IMHO it's ok to change it.

> And, AFAICT, the only way
> to sign a MAR with multiple signatures is to add all of those signatures at
> the same time, not incrementally in multiple executions of signmar.

Not true, when we sign a MAR we rebuild the whole file. So we could do the same.  We may need to know up front how many signatures the MAR should be prepared for or something like that to cover the file size field though.

> I agree with Brian Bondy that it is OK to change the signing semantics to
> what he suggests. But, we should then consider this version 2 of the file
> format, and change the file type identifier in the header to "MAR2", and
> update the spec. 

I didn't design the original version of this spec, but I do know that if you change that header value it will not be backwards compatible and no exiting product would be able to consume such a MAR :(  

Maybe this is fine for B2G, but we can't do this for desktop Firefox.

> And, we need to make sure we do not send MAR2 files to
> Firefox versions (as part of Firefox update) that only support MAR1. (The
> safest way to do this is to have the signmar program output MAR1 files for
> the first signature, and then MAR2 files for the second signature.)

Ah I see you knew that too, yup that sounds good.

> An alternate way of doing this would be to simply nest a MAR inside a MAR
> inside a MAR, where each level of MAR is signed by a different organization.
> I don't know if this is a *better* alternative. It would be approximately
> the same amount of overhead, and it would avoid the need to changing any of
> the MAR tools, and it would allow each organization to provide backup keys
> like the original MAR specification did. Brian Bondy probably understands
> the downsides to this idea better than I do.

That seems like we'd need to change core updater code in risky ways to accomplish it.  I prefer adding support for multiple signatures.

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Brian Smith (:bsmith) from comment #4)
> An alternate way of doing this would be to simply nest a MAR inside a MAR
> inside a MAR, where each level of MAR is signed by a different organization.

Not following along very closely, but this is less desirable for two reasons
 - makes it harder to manage disk space usage for intermediate unpackings
 - makes signature checking more expensive (IIUC)
 - means that the signing of organizations X, Y, Z become dependent on the previous parties to finish

Comment 7

[Marshall Culpepper [:marshall_law]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #6)
> (In reply to Brian Smith (:bsmith) from comment #4)
> > An alternate way of doing this would be to simply nest a MAR inside a MAR
> > inside a MAR, where each level of MAR is signed by a different organization.
> 
> Not following along very closely, but this is less desirable for two reasons
>  - makes it harder to manage disk space usage for intermediate unpackings
>  - makes signature checking more expensive (IIUC)
>  - means that the signing of organizations X, Y, Z become dependent on the
> previous parties to finish

Would it be easier to just require all signing certs at signing time? This still requires all org signatures up front, but gets rid of the "how do I add another signature" problem..

Comment 8

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

(In reply to Marshall Culpepper [:marshall_law] from comment #7)
> > Not following along very closely, but this is less desirable for two reasons
> >  - makes it harder to manage disk space usage for intermediate unpackings

Very true. Avoiding this would be the most complicated part.

> >  - makes signature checking more expensive (IIUC)

In theory. But in practice, with the APIs we have available for us to use for checking the signatures, we're not going to be able to do the optimization that avoids hashing the whole file three times, like I think you are thinking.

> >  - means that the signing of organizations X, Y, Z become dependent on the
> > previous parties to finish

How big of a problem would this be, in practice? This is also the problem with the "change the MAR spec so that signature X convers signature X-1" solution. This is solved by the "symmetric" solution where each sginature signs the data independently of the other signatures. But, bbondy is concerned about the "symmetric" solution (because the implementation is more complex?).

Comment 9

[Brian R. Bondy [:bbondy]]

I thought of another approach for discussion. 
We could leave the signature stuff exactly the same and only for Mozilla but define a new ADDITIONAL_SECTIONS entry.  
https://wiki.mozilla.org/Software_Update:MAR#Additional_sections

This would give us flexibility to do anything we want without changing existing semantics.

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Brian Smith (:bsmith) from comment #8)
> (In reply to Marshall Culpepper [:marshall_law] from comment #7)
> > >  - makes signature checking more expensive (IIUC)
> 
> In theory. But in practice, with the APIs we have available for us to use
> for checking the signatures, we're not going to be able to do the
> optimization that avoids hashing the whole file three times, like I think
> you are thinking.
> 

That's what I was thinking.  OK.

Comment 11

[Brian R. Bondy [:bbondy]]

A couple questions:
1. Will we know the number of signatures we want to include in the MAR file at build time?
2. Will we know the size of these signatures at build time?

I'm thinking that even if we generate MAR1 files, we'll still need RelEng to do some custom work.  They'll need to update the signmar program and also they'll need to somehow have the information above.   

Assuming we can get info for both 1&2 above, maybe we can commit this info to some file in m-c that RelEng can parse to determine how many MARs it will generate?

Comment 12

[Brian R. Bondy [:bbondy]]

In case someone overlooks Comment 11, these 2 questions are different than the 2 I sent to RelEng.

Comment 13

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

(In reply to Brian R. Bondy [:bbondy] from comment #11)
> A couple questions:
> 1. Will we know the number of signatures we want to include in the MAR file
> at build time?

Yes, if we are going to change the signature semantics from OR to AND. There won't be any unnecessary signatures, so the number of signatures in the MAR file will be the number that we require.

> 2. Will we know the size of these signatures at build time?

Yes, if every organization only has one key each, of it all of given organization's keys are the same size.

We will know the sizes of all the valid keys for each organization, because their certificates must be embedded in the tree. For example, if we update V1 -> V2, we just need to look in the V1 source code to determine what keys are even valid.

> I'm thinking that even if we generate MAR1 files, we'll still need RelEng to
> do some custom work.  They'll need to update the signmar program and also
> they'll need to somehow have the information above.   

Yes, because:

(1) We need to test our multi-signature verification code
(2) They need to take the three signatures from the three organizations, and add them to a single MAR file.
(3) We need some mechanism to say "key XXX is the OEM key, key YYY is the carrier key, and key ZZZ is Mozilla's key." Before, we didn't have to worry about which key belonged to whom. This requires changes to both how we validate signatures and how we generate them.

> Assuming we can get info for both 1&2 above, maybe we can commit this info
> to some file in m-c that RelEng can parse to determine how many MARs it will
> generate?

If our requirement is that a carrier, an OEM, and Mozilla must all sign each MAR, then we know the number of signatures is always going to be 3 (assuming one signature per organization). For our internal testing, we just need to create dummy OEM and carrier keys.

It seems like we'd need to do something like this:

(1) Change the MAR spec to say that the signatures have AND semantics, instead of OR semantics.
(2) Specify the relative positions of signatures in the MAR file: e.g. first signature is Mozilla, second signature is OEM, third signature is carrier.
(2) Change signmar to allow us to pass the IDs of THREE certs that will be used to sign the MAR, instead of one.
(3) Change signmar to output the signatures in a copy-pastable/emailable way, e.g. Base64.
(3) Create a tool that allows one or two signatures in a MAR file to be replaced with a different that are provided on the command line. (This tool should verify that the new signatures have the same metadata and are valid for the MAR file before it does the replacement.)

Each organization will have to create dummy certs for the two other organizations, and they would use their real cert + the two dummy certs when calling signmar to sign the MAR and output the signatures. Then each organization would have to take the output of the signmar program and send the (Base64) encoded value of their signature to the organization that creates the final MAR. The organization that creates the final mar would then use the signature replacement tool to replace the dummy signatures with the real signatures of the other two partners; the result will be the final MAR that can be deployed to the production phones.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #10)
> Brian Smith wrote:
> > In theory. But in practice, with the APIs we have available for us to use
> > for checking the signatures, we're not going to be able to do the
> > optimization that avoids hashing the whole file three times, like I think
> > you are thinking.
> 
> That's what I was thinking.  OK.

Now that we've agreed to make the signatures independent, we will be able to do this optimization.

Comment 14

[Brian R. Bondy [:bbondy]]

> >     2. We're thinking of introducing a new MAR type which is not backwards
> >    compatible with the existing MAR format, and only used for B2G.
> >    Would generating incompatible MAR files for B2G cause you any extra work?
> >    I think whether or not we make the new MAR files backwards compatible,
> >    we'll still need some new signing command lines that are used only for B2G.

catlee wrote:
> This shouldn't be a problem. It would be ideal if the same tools could be used to 
> generate/sign/unpack mar files of either format, controlled by a command line 
> switch or similar.

So I'm fine with doing the MAR2 format if you want controlled only by the command line switch.   Maybe we could remove the extra check that the file size actually matches the file-size-mar-field for MAR2 files as well at verification time since the extra signatures would be tacked on the end.

Comment 15

[Andrew Overholt [:overholt]]

Chris, should this be owned by you?  Or maybe Nick?

Comment 16

[Andrew Overholt [:overholt]]

Brian Bondy, will you be doing this work?

Comment 17

[Brian R. Bondy [:bbondy]]

If it's high priority then no I don't think so.

Comment 18

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

Brian Bondy, please provide feedback on Comment 13. I will ask you to review the patches for this bug. Will you have time to review them?

Comment 19

[Brian R. Bondy [:bbondy]]

> I will ask you to review the patches for this bug. 
> Will you have time to review them?

Yes I'll have time to review the patches for this bug.
I could work on them myself, but it won't be for a couple weeks, and if this is needed please pass it through rstrong first for prioritization with Windows 8 metro work and other general updater work.

> Brian Bondy, please provide feedback on Comment 13. 

Generally, I didn't comment because I agree with everything you said.
I would like someone who knows more to comment on how the release coordination will work, and if your statements are all correct.

I can comment on the rest though.

Other points to add relating to implementation:

- The signmar tool and libmar should work in the exact same way for verification and signing as it used to with the old command line parameters.
- New verification code should read MARX and parse out the X. If X is one it should verify using OR semantics.  If X is 2 it should do whatever the new rules are.

> (3) Change signmar to output the signatures in a copy-pastable/emailable 
> way, e.g. Base64.
>(3) Create a tool that allows one or two signatures in a MAR file to be replaced
> with a different that are provided on the command line. (This tool should verify 
> that the new signatures have the same metadata and are valid for the MAR file 
> before it does the replacement.)

We should add a new command for export and import (to be symmetric).  We can also optionally automatically do an export at the time you sign a mar.  Or allow the command lines to be combined. We should use some new file extension for the exports so they can be easily identified.  I think the export would contain a hash of the entire file excluding the signature block, as well as the signature.  The import would verify that the hash matches and import the signature if so.

bsmith: could you summarize your plan for MAR2, and how additional signatures would fit in (I think at the end of the file).  Also I have a question in Comment 14 about removing the filesize check that should be covered by that response.

Do you need extra feedback about implementation bits? If so please be explicit about what exactly you're looking for detail on.

Comment 20

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

(In reply to Brian R. Bondy [:bbondy] from comment #19)
> I would like someone who knows more to comment on how the release
> coordination will work, and if your statements are all correct.

We had a meeting about how release coordination will work last night, based on the preliminary design in comment 13.

> - The signmar tool and libmar should work in the exact same way for
> verification and signing as it used to with the old command line parameters.
> - New verification code should read MARX and parse out the X. If X is one it
> should verify using OR semantics.  If X is 2 it should do whatever the new
> rules are.

Basically, my comment 13 was trying to summarize my understanding of your proposal on #b2g the other day. In particular, I was planning on just redefining MAR1 to replace OR semantics with AND semantics like you'd recommended, avoiding the need for a MAR2 format at all. 

> We should add a new command for export and import (to be symmetric).  We can
> also optionally automatically do an export at the time you sign a mar.  Or
> allow the command lines to be combined. We should use some new file
> extension for the exports so they can be easily identified.  I think the
> export would contain a hash of the entire file excluding the signature
> block, as well as the signature.  The import would verify that the hash
> matches and import the signature if so.

The outputted signature will already included a hash of the file (excluding the other signatures), so we just need to verify the MAR with the given signatures before we replace the existing signatures with them.

> bsmith: could you summarize your plan for MAR2, and how additional
> signatures would fit in (I think at the end of the file).  Also I have a
> question in Comment 14 about removing the filesize check that should be
> covered by that response.

Let me clarify comment 13: I am proposing that every part will sign the MAR file with 3 signatures. In Mozilla's case, the signatures will be "Mozilla's Key," "Test Carrier's Key," and "Test OEM's key." We will make our "Test Carrier's Key" and "Test OEM's key" the same size as our partners' keys. So, for B2G, every MAR will have three signatures.

Later, when we have the actual carrier's signature and the actual OEM's signature, we will replace the test carrie and OEM signatures with the signatures from the real carrier and OEM, overwriting the test OEM and test carrier signature.

Other parties will do the same thing: the carrier will have a "Test Mozilla" cert and a "Test OEM" cert for their internal build testing; the OEM will have a "Test Mozilla" cert and a "Test Carrier" cert.

The consequence of doing it this way is that we don't need to change *anything* about the MAR file format, except that signatures will have AND semantics instead of OR semantics. There will never be any change to the file length or offsets because we require all the test signatures to be exactly the same size and algorithms as the production signatures.

Comment 21

[Brian R. Bondy [:bbondy]]

(In reply to Brian Smith (:bsmith) from comment #20)
> Let me clarify comment 13: I am proposing that every part will sign the MAR
> file with 3 signatures. In Mozilla's case, the signatures will be "Mozilla's
> Key," "Test Carrier's Key," and "Test OEM's key." We will make our "Test
> Carrier's Key" and "Test OEM's key" the same size as our partners' keys. So,
> for B2G, every MAR will have three signatures.
> 
> Later, when we have the actual carrier's signature and the actual OEM's
> signature, we will replace the test carrier and OEM signatures with the
> signatures from the real carrier and OEM, overwriting the test OEM and test
> carrier signature.
> Other parties will do the same thing: the carrier will have a "Test Mozilla"
> cert and a "Test OEM" cert for their internal build testing; the OEM will
> have a "Test Mozilla" cert and a "Test Carrier" cert.
> The consequence of doing it this way is that we don't need to change
> *anything* about the MAR file format, except that signatures will have AND
> semantics instead of OR semantics. There will never be any change to the
> file length or offsets because we require all the test signatures to be
> exactly the same size and algorithms as the production signatures.

Yes I already understood all of this, but I didn't understand that you were making these claims in support of keeping MAR1 instead of MAR2.  Thanks for clarifying. 

> Other parties will do the same thing: the carrier will have a "Test Mozilla"
> cert and a "Test OEM" cert for their internal build testing; the OEM will
> have a "Test Mozilla" cert and a "Test Carrier" cert.

Just out of curiosity, how do you envision them testing? And who owns the update server that distributes the MAR files in the end? (I understand that any party can with your design).  I assume that the test certs shouldn't be accepted as a valid cert when verifying, so how does one test such an update?

Comment 22

[Brian R. Bondy [:bbondy]]

And yes, I'm fine with changing OR-semantics to AND-semantics in the context of MAR1 given that we'll always have the number of signers beforehand and signing info.

Comment 23

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

(In reply to Brian R. Bondy [:bbondy] from comment #21)
> Just out of curiosity, how do you envision them testing? And who owns the
> update server that distributes the MAR files in the end? (I understand that
> any party can with your design).  I assume that the test certs shouldn't be
> accepted as a valid cert when verifying, so how does one test such an update?

One (or some combination of) the following:

(1) Everybody will have to do the bulk of their testing using builds that accept the test certs for the other party, and then do another (final) round of testing with new builds that only accept the production certs.

(2) All the builds will, by default accept only the production certs, but then the QA teams can use some debugging tool over USB to replace the production certs with test certs on the test devices (so that the test certs will be accepted instead of the production certs) before they run their tests.

(3) All three parties regularly share builds that we all sign so that we can all test the production configuration.

When I talked with RelEng yesterday, they were very skeptical (as I am) that each party could independently build B2G in such a way that the results are identical across all three builds. I think that near the end it will be important that we all make sure we're testing exactly the same builds, so I imagine that we'll do #2 (or #1) for a while, and then switch to #3 in the end-game.

Comment 24

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

(In reply to Brian R. Bondy [:bbondy] from comment #21)
> And who owns the update server that distributes the MAR files in the end? (I 
> understand that any party can with your design).

This is still up in the air. For the dogfooding stage, we will be distributing them.

Comment 25

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

(In reply to Brian Smith (:bsmith) from comment #23)
> (In reply to Brian R. Bondy [:bbondy] from comment #21)
> > Just out of curiosity, how do you envision them testing? And who owns the
> > update server that distributes the MAR files in the end? (I understand that
> > any party can with your design).  I assume that the test certs shouldn't be
> > accepted as a valid cert when verifying, so how does one test such an update?
> 
> One (or some combination of) the following:

How is it done for Firefox updates? Firefox updates have the same problem, and it seems like it would be nice to use a solution we already have experience with, if possible.

Comment 26

[Brian R. Bondy [:bbondy]]

There's a set of MAR unit tests but they don't run into this issue because it's just testing the MAR format itself.

For updater unit tests, Windows is the only platform that currently verifies MAR files. Tinderbox builds get generated with an xpcshell-labeled-cert embedded into udpater.exe instead of the real cert.

Comment 27

[Brian R. Bondy [:bbondy]]

there's also a define that you can remove support for the signing bits: MOZ_VERIFY_MAR_SIGNATURE

Comment 28

[Brian R. Bondy [:bbondy]]

I'll start to work on this Monday and get bsmith to review once ready

Comment 29

[Brian R. Bondy [:bbondy]]

Attachment: Patch v0.9

Here's a working patch for multiple MAR signatures support, as well as unit tests that covers the new functionality. 

I did development on OS X, I still need to test this on Windows (since there are some differences on that platform) and do some minor cleanup, so I'll wait to do that until I request review from bsmith.

Comment 30

[Brian R. Bondy [:bbondy]]

Attachment: Patch v0.95

Fixed Windows errors, tests pass on Windows.
Left to do: cleanup

Comment 31

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - Binary data files

Comment 32

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - Tests

Comment 33

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - Multiple signature sign/verify support

Comment 34

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - Existing updater code fixes

Comment 35

[Brian R. Bondy [:bbondy]]

Not sure if this is the correct keyword, but I think this should have a security review at some point.

Comment 36

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

Comment on attachment 668090 [details] [diff] [review]
Patch v1 - Tests

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

Brian, these patches look good to me. I will re-review them again tomorrow when my brain is less fried, but I don't expect to make any significant requests.

::: modules/libmar/tests/unit/test_sign_verify.js
@@ +193,3 @@
>      }, 
> +    // Test verifying a signed MAR file with too many certs fails
> +    test_verify_single_too_many_certs: function() {

The problem isn't too many certs; the problem is that the signature for mycert2 is missing.

@@ +209,5 @@
> +      verifyMAR(signedMAR, wantSuccess, ["mycert", "mycert2", "mycert3"]);
> +    },
> +    // Test verifying a signed MAR file with the same signature multiple
> +    // times fails.
> +    test_verify_multiple_same_cert: function() {

This is not what the test is testing. The test is testing the case where the 2nd signature doesn't verify with the second cert and the 3rd signature doesn't verify with the third cert. But, if it happened to be the case that the MAR was signed three times by mycert, then the verification would succeed when we pass mycert three times.

@@ +214,5 @@
> +      let signedMAR = do_get_file("data/multiple_signed_pib_mar.mar");
> +      verifyMAR(signedMAR, wantFailure, ["mycert", "mycert", "mycert"]);
> +    },
> +    // Test verifying a signed MAR file with the correct signatures but in
> +    // the wrong order fails

It is OK that we require the signatures to be in a given order, but this isn't strictly required. We could (relatively easily) make it OK for the signatures to be in any order. We need to document that we restrict the ordering of the signatures only as an optimization, not for security reasons. And, we need to document what the order is supposed to be.

If you have time, it would be better to remove the ordering requirement.

@@ +219,5 @@
> +    test_verify_multiple_wrong_order: function() {
> +      let signedMAR = do_get_file("data/multiple_signed_pib_mar.mar");
> +      verifyMAR(signedMAR, wantFailure, ["mycert3", "mycert2", "mycert"]);
> +    },
> +    // Test verifying only a subset of the signatures fails

This is the same test as test_verify_single_too_many_certs, except with 2/3 signatures instead of 1/2.

@@ +269,5 @@
> +      // Verify that the stripped MAR matches the original data MAR exactly
> +      let outMARData = getBinaryFileData(outMAR);
> +      let originalMARData = getBinaryFileData(originalMAR);
> +      compareBinaryData(outMARData, originalMARData);
> +    },

We should add a test of a MAR file with zero signatures, being verified with multiple signatures.

Comment 37

[Brian R. Bondy [:bbondy]]

Comment on attachment 668090 [details] [diff] [review]
Patch v1 - Tests

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

::: modules/libmar/tests/unit/test_sign_verify.js
@@ +193,3 @@
>      }, 
> +    // Test verifying a signed MAR file with too many certs fails
> +    test_verify_single_too_many_certs: function() {

Understood, I'll change this test to pass mycert twice.

@@ +209,5 @@
> +      verifyMAR(signedMAR, wantSuccess, ["mycert", "mycert2", "mycert3"]);
> +    },
> +    // Test verifying a signed MAR file with the same signature multiple
> +    // times fails.
> +    test_verify_multiple_same_cert: function() {

I think this is just a comment change, the comment should have said: 
// Test verifying a signed MAR file with the same certificate multiple
// times fails.  The input MAR has: mycert, mycert2, mycert3.
// We're verifying that position 2 and 3 are wrong.
What I'm trying to test here is someone knowing a MAR file has 3 signatures, so using the same cert 3 times to satisfy numVerified ==  3.
I'll change the comment depending on if we accept only specific ordering or not.

@@ +214,5 @@
> +      let signedMAR = do_get_file("data/multiple_signed_pib_mar.mar");
> +      verifyMAR(signedMAR, wantFailure, ["mycert", "mycert", "mycert"]);
> +    },
> +    // Test verifying a signed MAR file with the correct signatures but in
> +    // the wrong order fails

I'll likely remove that requirement and change this to a wantSuccess test.

@@ +219,5 @@
> +    test_verify_multiple_wrong_order: function() {
> +      let signedMAR = do_get_file("data/multiple_signed_pib_mar.mar");
> +      verifyMAR(signedMAR, wantFailure, ["mycert3", "mycert2", "mycert"]);
> +    },
> +    // Test verifying only a subset of the signatures fails

I don't think so. This is different because the input MAR here has 3 signagtures in it: mycert, mycert2, and mycert3 would verify it.  Above that MAR only has 1 signature in it: mycert.  This is testing that all passed certs verify, but we should still fail since the input mar wants: mecert, mycert2, mycert3.

@@ +269,5 @@
> +      // Verify that the stripped MAR matches the original data MAR exactly
> +      let outMARData = getBinaryFileData(outMAR);
> +      let originalMARData = getBinaryFileData(originalMAR);
> +      compareBinaryData(outMARData, originalMARData);
> +    },

agreed, sounds good.  Also we should test that trying to verify while passing 0 signatures doesn't verify (or it's also testing that it doesn't crash).  Also I'll add a couple tests for a missing source MAR file should fail, and missing cert in the cert db. And do that last sentence for strip, sign, verify.

Comment 38

[Brian R. Bondy [:bbondy]]

Attachment: Patch v2 - Tests

Implemented review comments, and added more tests, some other minor refactoring on the tests.

Comment 39

[Brian R. Bondy [:bbondy]]

Attachment: Patch v2 - Multiple signature sign/verify support

Refactored code so that it only reads the MAR file once instead of once per signature. 

I did not change it to accept arbitrary ordering for verification, but the change to allow it is trivial, but I ran into a problem with NSS that I couldn't figure out.  The way I implemented it was with VFY_EndWithSignature being called on each extracted signature.  The problem though is that once you pass a VFYContext with a wrong extracted signature and it fails, no future calls can succeed.  I tried to find an NSS call to clone a VFYContext but I couldn't find one.  

If you know how to do this please let me know and I'll make that change in a follow up bug, let's try to finish up on this one for the time being.

I do understand that we will want to assign ordering of who signs in which position, but this bug is not the place either. This bug is generic libmar work and not b2g specific.

Comment 40

[Brian R. Bondy [:bbondy]]

Do you have an ETA for reviewing this and bug 795921?

I'd like to get these 2 bugs landed so that RelEng can build a signmar tool and start signing B2G stuff.  I'm not sure how much time RelEng will need, so I think it is important that we give them what they need from us as early as possible.

I know you started to look at this last week, but I only seen a few comments on the tests since last week.

Comment 41

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

Comment on attachment 669555 [details] [diff] [review]
Patch v2 - Multiple signature sign/verify support

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

Looks pretty good.

::: modules/libmar/sign/mar_sign.c
@@ +35,5 @@
>  {
>    SECStatus status = NSS_Initialize(NSSConfigDir, 
>                                      "", "", SECMOD_DB, NSS_INIT_READONLY);
>    if (SECSuccess != status) {
> +    fprintf(stderr, "ERROR: Could not initialize NSS: %d\n", (int)status);

fprintf(stderr, "ERROR: Could not initialize NSS: %s\n", PR_ErrorToName(PR_GetError()));

It's magic.

@@ +138,5 @@
>      return -2;
>    }
> +
> +  for (k = 0; k < ctxCount; ++k) {
> +    if (SGN_Update(ctxs[k], (const unsigned char *)buffer, size) != SECSuccess) {

Nit: Use uint8_t instead of unsigned char for non-textual data.

@@ +482,5 @@
>   * The passed in MAR file must not already be signed or an error will 
>   * be returned.
>   *
> + * @param  NSSConfigDir  The NSS directory containing the private key for signing
> + * @param  certName      The nickname of the certificate to use for signing

s/certName/certNames/
s/nickname/nicknames/

Please use the plural form consistently for all arrays.

@@ +485,5 @@
> + * @param  NSSConfigDir  The NSS directory containing the private key for signing
> + * @param  certName      The nickname of the certificate to use for signing
> + * @param  certNameCount The number of certificate names contained in certNames
> + *                       This is also the same as the number of signatures you'd
> + *                       like to produce.

s/This is also the same as the number of signatures you'd like to produce/One signature will be produced for each certificate/

@@ +493,5 @@
>   *         -1 on error
>  */
>  int
>  mar_repackage_and_sign(const char *NSSConfigDir, 
> +                       const char **certNames,

const char * const *certNames,

In general in this patch, wherever you have const <X> **, you should replace that with const <X> * const *

@@ +508,5 @@
>      numChunks, i;
>    FILE *fpSrc = NULL, *fpDest = NULL;
>    int rv = -1, hasSignatureBlock;
> +  SGNContext *ctxs[MAX_SIGNATURES];
> +  SECItem secItem[MAX_SIGNATURES];

s/secItem/secItems/

@@ +513,2 @@
>    char buf[BLOCKSIZE];
> +  SECKEYPrivateKey *privKey[MAX_SIGNATURES];

s/privKey/privKeys/

@@ +513,3 @@
>    char buf[BLOCKSIZE];
> +  SECKEYPrivateKey *privKey[MAX_SIGNATURES];
> +  CERTCertificate *cert[MAX_SIGNATURES];

s/cert/certs/

@@ +666,5 @@
>      goto failure;
>    }
>    numSignatures = ntohl(numSignatures);
>  
> +  signaturePlaceholderOffset = ftello(fpDest);

Blank line here.

@@ +668,5 @@
>    numSignatures = ntohl(numSignatures);
>  
> +  signaturePlaceholderOffset = ftello(fpDest);
> +  for (k = 0; k < certNameCount; k++) {
> +    /* Write out the signature ID, for now only an ID of 1 is supported */

s/signature ID/signature algorithm ID/

@@ +760,5 @@
>    if (ftello(fpDest) > MAX_SIZE_OF_MAR_FILE) {
>      goto failure;
>    }
>  
> +  for (k = 0; k < certNameCount; k++) {

These loops that aren't doing anything with certNames would be clearer if certNameCount were renamed certCount, similar to the way you have a "certCount" parameter in functions below.

@@ +825,4 @@
>  
> +    if (privKey[k]) {
> +      SECKEY_DestroyPrivateKey(privKey[k]);
> +    }

Need to clean up secItems.

::: modules/libmar/src/mar.h
@@ +144,5 @@
>   *         a positive number if the signature does not verify
>   */
>  #ifdef XP_WIN
> +int mar_verify_signaturesW(MarFile *mar,
> +                           const char **certData,

const uint8_t * const *certData,

::: modules/libmar/src/mar_cmdline.h
@@ +60,5 @@
>   *         a negative number if there was an error
>   *         a positive number if the signature does not verify
>   */
> +int mar_verify_signatures(const char *pathToMAR,
> +                          const char **certData,

const uint8_t * const * certData,

@@ +61,5 @@
>   *         a positive number if the signature does not verify
>   */
> +int mar_verify_signatures(const char *pathToMAR,
> +                          const char **certData,
> +                          uint32_t *sizeOfCertData,

const uint32_t * certDataSizes,

@@ +62,5 @@
>   */
> +int mar_verify_signatures(const char *pathToMAR,
> +                          const char **certData,
> +                          uint32_t *sizeOfCertData,
> +                          const char **certNames,

const char * const * certNames,

::: modules/libmar/tool/mar.c
@@ +21,5 @@
>  int NSSInitCryptoContext(const char *NSSConfigDir);
>  #endif
>  
>  int mar_repackage_and_sign(const char *NSSConfigDir,
> +                           const char **certNames,

const char * const *certNames,

@@ +84,3 @@
>  #if defined(XP_WIN) && !defined(MAR_NSS) && !defined(NO_SIGN_VERIFY)
>    HANDLE certFile;
> +  DWORD fileSizes[MAX_SIGNATURES];

Why are we using DWORD here when other places were changed to use uint32_t. Also, why not uint64_t?

@@ +87,2 @@
>    DWORD read;
> +  char *certBuffers[MAX_SIGNATURES];

uint8_t *

@@ +92,5 @@
> +  memset(certNames, 0, sizeof(certNames));
> +#if defined(XP_WIN) && !defined(MAR_NSS) && !defined(NO_SIGN_VERIFY)
> +  memset(fileSizes, 0, sizeof(fileSizes));
> +  memset(certBuffers, 0, sizeof(certBuffers));
> +  memset(DERFilePaths, 0, sizeof(DERFilePaths));

It's important to document that these programs probably don't do Unicode properly, especially since our partners live in regions that use non-ASCII characters.

@@ +114,5 @@
>        argv += 2;
>        argc -= 2;
>      } 
>  #if defined(XP_WIN) && !defined(MAR_NSS) && !defined(NO_SIGN_VERIFY)
> +    /* -D DERFilePath, also matches -D[anything] DERFilePath */

It seems simple enough to just add a check that argv[1][2] == '0' + certCount.

BTW, NSS utilities usually use one of two syntaxes for multi-valued parameters:

Option 1) "-D" certname1(":"certname2)*. I.e. values are separated by colons and there's no need to number the args.

Option 2) (-D certname)+, i.e. the same syntax you use, but without the numbering, which seems to be what your parser actually parses.

I think we should specify the syntax as using the pattern in Option 2.

If there is an important reason for numbering the parameters, then we need to add consistency checking for the numbering, and we need to document that important reason. If we don't add consistency checking, then the user of the utility may get confused in what he expects "-D1 foo -D0 bar" to do.

@@ +130,5 @@
>      else if (argv[1][0] == '-' && argv[1][1] == 'd') {
>        NSSConfigDir = argv[2];
>        argv += 2;
>        argc -= 2;
> +     /* -n certName, also matches -n[anything] certName */

Ditto.

@@ +242,5 @@
> +      certBuffers[k] = malloc(fileSizes[k]);
> +      if (!ReadFile(certFile, certBuffers[k], fileSizes[k], &read, NULL) ||
> +          fileSizes[k] != read) {
> +        CloseHandle(certFile);
> +        free(certBuffers[k]);

Need to clean up certBuffers[0..(k-1)] all the time to avoid leaks.

@@ +254,4 @@
>        /* Determine if the source MAR file has the new fields for signing */
>        int hasSignatureBlock;
> +      for (k = 0; k < certCount; ++k) {
> +        free(certBuffers[k]);

better to move this to a "cleanup" section.

::: modules/libmar/verify/cryptox.c
@@ +80,5 @@
>   * @return CryptoX_Success on success, CryptoX_Error on error.
>  */
>  CryptoX_Result
> +//NSS_VerifySignature(VFYContext * const *ctx, 
> +NSS_VerifySignature(VFYContext **ctx, 

Why did we have to remove the const?

::: modules/libmar/verify/mar_verify.c
@@ +84,5 @@
>   * 
>   * @param pathToMAR  The path of the MAR file who's signature should be checked
> + * @param certData   Pointer to the first element in an array of certificate
> + *                   file data.
> + * @param sizeOfCertData Pointer to the first element in an array for size of the

certDataSizes

@@ +97,5 @@
>  int
> +mar_verify_signatures(const char *pathToMARFile,
> +                      const char **certData,
> +                      uint32_t *sizeOfCertData,
> +                      const char **certNames,

const uint8_t * const * certData,
const uint32_t *certDataSizes,
const char * const *certNames,

@@ +148,5 @@
> +
> +    if (certs[k]) {
> +      CryptoX_FreeCertificate(&certs[k]);
> +    }
> +  } 

Trailing whitespace.

@@ +154,5 @@
>  }
>  
>  #ifdef XP_WIN
>  /**
> + * Verifies a MAR file by making sure all signatures verify.

Verifies a MAR file by verifying each signature with the corresponding certificate. That is, the first signature will be verified using the first certificate given, the second signature will be verified using the second certificate given, etc. The signature count must exactly match the number of certificates given, and all signature verifications must succeed.

@@ +159,2 @@
>   * 
> + * @param  pathToMARFile  The path of the MAR file who's signature 

Seems like this parameter's documentation is out of date. Also, trailing whitespace.

@@ +160,5 @@
> + * @param  pathToMARFile  The path of the MAR file who's signature 
> + *                        should be calculated
> + * @param  certData       Pointer to the first element in an array of
> + *                        certificate data
> + * @param  sizeOfCertData Pointer to the first element in an array for size of

certDataSizes

@@ +168,5 @@
>  */
>  int
> +mar_verify_signaturesW(MarFile *mar,
> +                       const char **certData,
> +                       uint32_t *sizeOfCertData,

const uint8_t * const *certData,
const uint32_t * certDataSizes,

@@ +212,5 @@
> +
> +    if (certs[k]) {
> +      CryptoX_FreeCertificate(&certs[k]);
> +    }
> +  } 

trailing whitespace.

@@ +342,2 @@
>      return CryptoX_Success;
> +  } else if (numVerified == 0) {

Do not use else after return.

@@ +350,4 @@
>  }
>  
>  /**
>   * Verifies if a specific signature ID matches the extracted signature.

This documentation is incorrect. I think it needs to be similar to the documentation I suggested for the previous function.

@@ +356,5 @@
>   * @param  provider             A library provider
> + * @param  keys                 A pointer to the first element in an
> + *                              array of keys.
> + * @param  signatureCount       The number of signatures in the MAR file
> + * @param  extractedSignatures  Pointer to the first element in an array

The order of signatureCount and extractedSignatures should be swapped to be consistent with the rest of the file.

@@ +367,5 @@
> +mar_verify_signatures_for_fp(FILE *fp,
> +                             CryptoX_ProviderHandle provider,
> +                             CryptoX_PublicKey *keys,
> +                             uint32_t signatureCount,
> +                             char **extractedSignatures,

const uint8_t * const *extractedSignatures,

@@ +368,5 @@
> +                             CryptoX_ProviderHandle provider,
> +                             CryptoX_PublicKey *keys,
> +                             uint32_t signatureCount,
> +                             char **extractedSignatures,
> +                             uint32_t *numVerified)

It seems like the function either fails, or it returns *numVerified == signatureCount. So, why even have a numVerified output parameter?

@@ +395,5 @@
>      return CryptoX_Error;
>    }
>  
> +  for (i = 0; i < signatureCount; i++) {
> +    CryptoX_VerifyBegin(provider, &signatureHandles[i], &keys[i]);

You need to clean up signatureHandles for all the early returns.

Also, why aren't we checking the return value of CryptoX_VerifyBegin?

@@ +438,4 @@
>                                                    "signature length"))) {
>        return CryptoX_Error;
>      }
> +    signatureLengths[i] = ntohl(signatureLengths[i]);

Need to check signatureLengths[i] against max signature length.

@@ +468,5 @@
> +  /* Verify the signatures */
> +  for (i = 0; i < signatureCount; i++) {
> +    if (CryptoX_Failed(CryptoX_VerifySignature(&signatureHandles[i],
> +                                               &keys[i],
> +                                               extractedSignatures[i], 

trailing whitespace.

Comment 42

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

Also, I think that the same (or better) effects could have been achieved with many fewer changes:

For each cert given:
   verify the MAR file with that cert like we did before.
   If that verification fails then fail.

This would have avoided most of the changes where functions were changed to take N certs instead of one cert, and would have avoided the strict 1:1 ordering between certs and signatures, while still ensuring that every cert given signed the MAR at least once. It might even still be easier to write that patch than to address all the review comments and re-review all the code looking for leaks that were introduced in the 1 cert -> n cert conversion.

Also, in the future when we need to make significant changes to these programs, we should convert them to C++ so that we can use ScopedNSSTypes and other smart pointers to avoid needing to track resource allocation so carefully.

Comment 43

[Brian R. Bondy [:bbondy]]

(In reply to Brian Smith (:bsmith) from comment #42)
> Also, I think that the same (or better) effects could have been achieved
> with many fewer changes:
> 
> For each cert given:
>    verify the MAR file with that cert like we did before.
>    If that verification fails then fail.
> 
> This would have avoided most of the changes where functions were changed to
> take N certs instead of one cert, ...

That would also mean that you'd have to read the MAR file from disk 3 times which doesn't sound like a good idea.  Actually more than 3 times given that when we verify we extract the signature and then move the file pointer around to read the rest of the data to compute the signature from the MAR data.

Comment 44

[Brian R. Bondy [:bbondy]]

(In reply to Brian Smith (:bsmith) from comment #42)
> Also, in the future when we need to make significant changes to these
> programs, we should convert them to C++ so that we can use ScopedNSSTypes
> and other smart pointers to avoid needing to track resource allocation so
> carefully.

The choice of C here predates me, and changing to C++, along with changing all existing source doesn't sound like a good idea given that we were on a strict deadline here and there was a lot of existing code that would need to be updated.  RAII would have been great, but not worth the effort for this change.

Comment 45

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

(In reply to Brian R. Bondy [:bbondy] from comment #43)
> > This would have avoided most of the changes where functions were changed to
> > take N certs instead of one cert, ...
> 
> That would also mean that you'd have to read the MAR file from disk 3 times
> which doesn't sound like a good idea.  Actually more than 3 times given that
> when we verify we extract the signature and then move the file pointer
> around to read the rest of the data to compute the signature from the MAR
> data.

Understood.

(In reply to Brian R. Bondy [:bbondy] from comment #44)
> The choice of C here predates me, and changing to C++, along with changing
> all existing source doesn't sound like a good idea given that we were on a
> strict deadline here and there was a lot of existing code that would need to
> be updated.  RAII would have been great, but not worth the effort for this
> change.

Also understood. I'm definitely not suggesting that you convert this stuff to C++ now.

Comment 46

[Brian R. Bondy [:bbondy]]

Attachment: Patch v3 - Multiple signature sign/verify support

Implemented review comments.
Answers to some questions are inline in the code in comments.

Comment 47

[Brian R. Bondy [:bbondy]]

Attachment: Patch v4 - Multiple signature sign/verify support

Fixed a couple more const type declarations

Comment 48

[Brian R. Bondy [:bbondy]]

Attachment: Patch v3 - Tests

Updated tests patch

Comment 49

[Brian R. Bondy [:bbondy]]

Attachment: Patch v5 - Multiple signature sign/verify support

Windows fix

Comment 50

[Brian R. Bondy [:bbondy]]

Attachment: Patch v5' - Multiple signature sign/verify support

Comment 51

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

Comment on attachment 671470 [details] [diff] [review]
Patch v5' - Multiple signature sign/verify support

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

Brian, looks good. Thank you for adding the explicative comments into the source code.

Note that there seems to be one serious bug in this update. Search for "double-free".

::: modules/libmar/sign/mar_sign.c
@@ +827,5 @@
> +      SECKEY_DestroyPrivateKey(privKeys[k]);
> +    }
> +
> +    PORT_Free(secItems[k].data);
> +    secItems[k].data = NULL;

Nit: SECITEM_FreeItem(&secItems[k], false);

No need for secItems[k].data = NULL;

Generally, try to use the de-allocation function that corresponds to the allocation function.

::: modules/libmar/src/mar.h
@@ +140,5 @@
>   * @param mar            The already opened MAR file.
> + * @param certData       Pointer to the first element in an array of certificate
> + *                       file data.
> + * @param sizeOfCertData Pointer to the first element in an array for size of
> + *                       the cert data.

Change name of argument to match other renamings you did.

@@ +149,5 @@
>   */
>  #ifdef XP_WIN
> +int mar_verify_signaturesW(MarFile *mar,
> +                           const uint8_t * const *certData,
> +                           uint32_t *sizeOfCertData,

const uint32_t * certDataSizes,

::: modules/libmar/src/mar_cmdline.h
@@ +66,5 @@
>   *         a positive number if the signature does not verify
>   */
> +int mar_verify_signatures(const char *pathToMAR,
> +                          const uint8_t * const *certData,
> +                          uint32_t *certDataSizes,

const uint32_t *

::: modules/libmar/tool/mar.c
@@ +122,5 @@
>      } 
>  #if defined(XP_WIN) && !defined(MAR_NSS) && !defined(NO_SIGN_VERIFY)
> +    /* -D DERFilePath, also matches -D[index] DERFilePath
> +       The reason we require an index for verifying is to keep symmetric
> +       with the import and export command line arguments.  */

The comment doesn't match the code, because the comment says the indexes are required. I suggest we just remove the " || argv[1][2] == '\0'" to resolve the mismatch.

Also, static_assert that MAX_SIGNATURES <= 9 because this code requires that.

Nit: s/The reason we/We/ and s/is to keep/to be/

@@ +142,5 @@
>        argv += 2;
>        argc -= 2;
> +     /* -n certName, also matches -n[index] certName
> +        The reason we require an index for verifying is to keep symmetric
> +        with the import and export command line arguments.  */

Ditto.

@@ +256,5 @@
> +      if (!ReadFile(certFile, certBuffers[k], fileSizes[k], &read, NULL) ||
> +          fileSizes[k] != read) {
> +        CloseHandle(certFile);
> +        for (i = 0; i <= k; i++) {
> +          free(certBuffers[k]);

free(certBuffers[i]);

double-free and memory leak.

::: modules/libmar/verify/mar_verify.c
@@ +136,5 @@
> +    if (CryptoX_Failed(CryptoX_LoadPublicKey(provider, certData[k], certDataSizes[k],
> +                                             &keys[k], certNames[k], &certs[k]))) {
> +      fclose(fp);
> +      fprintf(stderr, "ERROR: Could not load public key.\n");
> +      return CryptoX_Error;

This will leak certs[0..(k-1)] and keys[0..(k-1)].

Perhaps "goto failure"

@@ +375,5 @@
> + * @param  signatureCount       The number of signatures in the MAR file
> + * @param numVerified           Out parameter which will be filled with
> + *                              the number of verified signatures.
> + *                              This information can be useful for printing
> + *                              error messages.

We should add "On success, *numVerified == signatureCount."

Comment 52

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

Comment on attachment 671455 [details] [diff] [review]
Patch v3 - Tests

I reviewed just the interdiff from v1.

Comment 53

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

Comment on attachment 668089 [details] [diff] [review]
Patch v1 - Binary data files

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

Do you have the command lines / scripts that were used to make these changes?

Comment 54

[Brian R. Bondy [:bbondy]]

I didn't save them but it is pretty straight forward and it is the same commands as before in the original signing bug with certutil. I'm confident it's safe to be r+.

Comment 55

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

Comment on attachment 668090 [details] [diff] [review]
Patch v1 - Tests

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

LGTM

Comment 56

[Brian R. Bondy [:bbondy]]

So for the binary patch I did this:
certutil -S -d . -s "CN=My Cert" -n mycert -x -t ",,u" -g 2048
certutil certutil -L -d . -n mycert -r > mycert.der
certutil -S -d . -s "CN=My Cert2" -n mycert2 -x -t ",,u" -g 2048
certutil certutil -L -d . -n mycert2 -r > mycert2.der
certutil -S -d . -s "CN=My Cert3" -n mycert3 -x -t ",,u" -g 2048
certutil certutil -L -d . -n mycert3 -r > mycert3.der

And each time it asked me to press keys on my keyboard, etc.

Comment 57

[Brian R. Bondy [:bbondy]]

> The comment doesn't match the code, because the comment says the indexes are 
> required. I suggest we just remove the " || argv[1][2] == '\0'" to resolve the 
> mismatch.

I'm going to just adjust the comment because I want to keep this backwards compatible so releng doesn't need to change existing scripts.

Comment 58

[Brian R. Bondy [:bbondy]]

Attachment: Patch v3 - Tests

Carrying forward r+.
Minor Windows fix.

Comment 59

[Brian R. Bondy [:bbondy]]

Attachment: Patch v3 - Tests

Comment 60

[Brian R. Bondy [:bbondy]]

Attachment: Patch v6 - Multiple signature sign/verify support

Implemented review comments.

Comment 61

[Brian R. Bondy [:bbondy]]

Attachment: Patch v6 - Multiple signature sign/verify support

sorry forgot to qfold the changes of the review comments.

Comment 62

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

Comment on attachment 668089 [details] [diff] [review]
Patch v1 - Binary data files

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

I didn't really review these binary files, but I did review the tests that use them and the commands that bbondy uesd to generate the DER files.

Comment 63

[Brian R. Bondy [:bbondy]]

Attachment: Patch v2 - Existing updater code fixes

Fix for updater patch after review comments (char * to uint_8 * compiling error on Windows)
Carrying forward r+.

Comment 64

[Brian R. Bondy [:bbondy]]

https://hg.mozilla.org/mozilla-central/rev/ae0eca054da9
https://hg.mozilla.org/mozilla-central/rev/d01cfea41ba2
https://hg.mozilla.org/mozilla-central/rev/b417ab9ec14d
https://hg.mozilla.org/mozilla-central/rev/cd20be13eaaf

Comment 65

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/829d05dcdec5
https://hg.mozilla.org/releases/mozilla-aurora/rev/f3b95f654815
https://hg.mozilla.org/releases/mozilla-aurora/rev/842138897e32
https://hg.mozilla.org/releases/mozilla-aurora/rev/43aad407af6a

Comment 66

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Removing secreview flag here since a) I'm not going to get to it any time soon, and b) I would probably just have to ask one of the Brians above to explain the code to me, so don't think I am going to add any value here.