Brian R. Bondy [:bbondy] Assignee
	Description • 12 years ago

+++ This bug was initially created as a clone of Bug #792452 +++

For B2G we're changing the semantics of how libmar verification works.  We'll only verify a MAR file if all signatures match.  

We only have MAR files in the wild today that have a single signature, and only on Windows, so this doesn't matter.  If we ever do have to add multiple MAR signatures in Windows, we are conceding that the first update to such a MAR from a really old version will only check to see if a single signature is valid.

	Brian R. Bondy [:bbondy] Assignee
	Comment 2 • 12 years ago

I will update the documentation once I land the patch by the way.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 3 • 12 years ago

Brian, for B2G it isn't enough to verify that all the signatures are valid. We must verify that there is one signature from each party. That means that the verification logic must take N certs (or N sets of certs), one for each party, and then at the end verify that all three parties have signed it.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 4 • 12 years ago

(Consider the case where the carrier signed the MAR twice and the OEM signed it once, but there is no signature from Mozilla. Then, all three signatures would verify but we still don't want to consider the MAR to be valid.)

	Brian R. Bondy [:bbondy] Assignee
	Comment 5 • 12 years ago

So what I suggest for this is to have a new "Additional sections" block which asserts certain signature requirements on the MAR.  That will be done in its own bug and this bug will remain as is.  Does that sound good?

	Brian R. Bondy [:bbondy] Assignee
	Comment 6 • 12 years ago

In case this was not clear, updater code in b2g will only apply MAR files that have this additional block.  Whether or not signatures verify.  And the MAR will only verify if the conditions in that block are met.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 7 • 12 years ago

(In reply to Brian R. Bondy [:bbondy] from comment #5)
> So what I suggest for this is to have a new "Additional sections" block
> which asserts certain signature requirements on the MAR.  That will be done
> in its own bug and this bug will remain as is.  Does that sound good?

The requirements for what signatures are required for the MAR cannot be read from the MAR file itself; those requirements have to be hard-wired into the product verifying the signature.

Consider a MAR file that is signed by Mozilla (only) that doesn't include that extra signature requirements section. We do not want to accept that MAR file on B2G. Conversely, consider a MAR file that is signed by all three parties, and which doesn't include that extra signature requirements section. There's no reason to reject the MAR file. So, the extra section isn't needed.

Ideally it should work something like this:
1. The application creates N sets of certs and passes them to the MAR verifier.
2. The MAR verifier returns success if there is at least one signature from each set, and returns failure otherwise.
3. Firefox Desktop would pass one set of certs to the verifier (containing the two Desktop Firefox MAR signing certs).
4. B2G would pass three sets: one for carrier, one for OEM, one for Mozilla. Each set would have one or more certs in it.

That is, the semantics would be OR within a set, and AND across sets. This would subsume the current Desktop Firefox semantics as well as the required B2G semantics.

However, if it is too much work to allow more than one certificate per set, then we can fix the number of certs per set at one per set. Then, the array of certs passed to the verifier could stay the same, but the meaning would be different. Still, in that case, the patch above isn't good, because it accepts multiple signatures from the same cert.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 8 • 12 years ago

Here's another way to think about this: We need to change the semantics from OR to AND for the *certificates* (Cert X must have signed the mar AND cert Y must have signed the MAR). But, your patch changes the semantics from OR to AND for *signatures*, which isn't the same thing.

	Brian R. Bondy [:bbondy] Assignee
	Comment 9 • 12 years ago

I understand your point, but you don't currently understand my suggestion which solves your point.

(In reply to Brian Smith (:bsmith) from comment #7)
> (In reply to Brian R. Bondy [:bbondy] from comment #5)
> > So what I suggest for this is to have a new "Additional sections" block
> > which asserts certain signature requirements on the MAR.  That will be done
> > in its own bug and this bug will remain as is.  Does that sound good?
> 
> The requirements for what signatures are required for the MAR cannot be read
> from the MAR file itself; those requirements have to be hard-wired into the
> product verifying the signature.

Those requirements CAN be written in the MAR file.
However those requirements MUST be enforced from within B2G updater code.

> However, if it is too much work to allow more than one certificate per set,
> then we can fix the number of certs per set at one per set. Then, the array
> of certs passed to the verifier could stay the same, but the meaning would
> be different. 

Yes let's fix it as 1 cert per set. And let's forget about the lingo of a set :)

> Still, in that case, the patch above isn't good, because it
> accepts multiple signatures from the same cert.

The patch above is good and is a step into the right direction. It is not meant to satisfy every requirement that we have, I would rather split up the work into different bugs.  It makes reviews and discussions more targeted.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 10 • 12 years ago

(In reply to Brian R. Bondy [:bbondy] from comment #9)
> I understand your point, but you don't currently understand my suggestion
> which solves your point.

OK, you are right that I don't understand.

> (In reply to Brian Smith (:bsmith) from comment #7)
> > The requirements for what signatures are required for the MAR cannot be read
> > from the MAR file itself; those requirements have to be hard-wired into the
> > product verifying the signature.
> 
> Those requirements CAN be written in the MAR file.
> However those requirements MUST be enforced from within B2G updater code.

I still don't see how putting this information into the MAR file helps. B2G still will require three distinct signatures, whether or not this new section is present, and Desktop Firefox will still require one signature, whether or not this section is present. Definitely, B2G could be set up to reject MARs without this section; but, it is going to reject MARs without three distinct signatures anyway. That's why I don't think this new section is useful.

> Yes let's fix it as 1 cert per set. And let's forget about the lingo of a
> set :)

OK :)

> > Still, in that case, the patch above isn't good, because it
> > accepts multiple signatures from the same cert.
> 
> The patch above is good and is a step into the right direction. It is not
> meant to satisfy every requirement that we have, I would rather split up the
> work into different bugs.  It makes reviews and discussions more targeted.

OK. Are you planning to verify that all the signatures are valid, and then later verify that all signatures came from different certificates? If so, I'd like to see the patch that does the second part, to better understand why this first part is useful.

	Brian R. Bondy [:bbondy] Assignee
	Comment 11 • 12 years ago

the additional sections block that I'm talking about would have a specific " 4 bytes: BlockIdentifier" which would identify it as a MinimumCertRequirement block.  It would have a value of 3 in our case.

Then in our updater code, we would include 3 certs/public keys that can verify the embedded 3 signatures.  

The way you ensure that Mozilla doesn't sign 3 times, is because the builds we will produce for B2G will have 1 cert per party, and not 3 certs by Mozilla.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 12 • 12 years ago

(In reply to Brian R. Bondy [:bbondy] from comment #11)
> the additional sections block that I'm talking about would have a specific "
> 4 bytes: BlockIdentifier" which would identify it as a
> MinimumCertRequirement block.  It would have a value of 3 in our case.
> 
> Then in our updater code, we would include 3 certs/public keys that can
> verify the embedded 3 signatures.  
> 
> The way you ensure that Mozilla doesn't sign 3 times, is because the builds
> we will produce for B2G will have 1 cert per party, and not 3 certs by
> Mozilla.

But, what happens when the carrier creates a MAR with MinimumCertRequirement == 1, it with MinimumCertRequirement == 3 and then signs the MAR three times?

	Brian R. Bondy [:bbondy] Assignee
	Comment 13 • 12 years ago

The device should already have 3 certs on it that can verify.  
However that device was setup originally is how the verifying will work.

If a provider tries to sign 3 times, then the 3 certs on the device won't be able to verify it.

	Brian R. Bondy [:bbondy] Assignee
	Comment 14 • 12 years ago

.. because each of the 3 certs on the device will be used exactly one time.

	Brian R. Bondy [:bbondy] Assignee
	Comment 15 • 12 years ago

by the way, the reason I originally suggested an optional block was because I originally thought there may be other assertions we'd like to make as well, other than minimum signatures. In which case we could have all that handling internal to libmar and then simply check for the optional block id existance from within b2g updater code. 

But since you mentioned we'll only be needing number of certs validation (on top of making sure each cert is used once) I'll just do the work in another bug outside of the optional block.

	Andrew Overholt [:overholt]
	Comment 16 • 12 years ago

Do we know that we're going to do this, Brian (B.)?

	Brian R. Bondy [:bbondy] Assignee
	Comment 17 • 12 years ago

Yes I think this will be landing no matter what happens with B2G updates.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 18 • 12 years ago

Is this still needed after the other changes I already reviewed? I thought the other changes already incorporated this?

	Brian R. Bondy [:bbondy] Assignee
	Comment 19 • 12 years ago

Yes it is still needed. This is a dependency to the main libmar work bug, and so should land before that lands.

	Brian R. Bondy [:bbondy] Assignee
	Comment 20 • 12 years ago

It'll land at the same time, but the libmar work won't apply without this.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 21 • 12 years ago

Comment on attachment 666569 [details] [diff] [review]
Patch v1.

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

::: modules/libmar/verify/mar_verify.c
@@ +239,5 @@
>              MAX_SIGNATURES);
>      return CryptoX_Error;
>    }
>  
> +  for (i = 0; i < signatureCount && numVerified == i; i++) {

I agree we need to remove the numVerified == 0 condition.

For proper error messages, we cannot have the numVerified == i condition here, right? We need to keep verifying even after the first failed verification.

@@ +293,5 @@
>      }
>    }
>  
>    /* If we reached here and we verified at least one 
>       signature, return success. */

This comment is out of date.

@@ +299,3 @@
>      return CryptoX_Success;
>    } else {
>      fprintf(stderr, "ERROR: No signatures were verified.\n");

error message is out of date.

From my review of the other patches, I had thought that it would be impossible to reach this block. Are you sure this part of the patch is necessary in order for the tests to pass?

	Brian R. Bondy [:bbondy] Assignee
	Comment 22 • 12 years ago

(In reply to Brian Smith (:bsmith) from comment #21)
> Comment on attachment 666569 [details] [diff] [review]
> Patch v1.
> 
> Review of attachment 666569 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: modules/libmar/verify/mar_verify.c
> @@ +239,5 @@
> >              MAX_SIGNATURES);
> >      return CryptoX_Error;
> >    }
> >  
> > +  for (i = 0; i < signatureCount && numVerified == i; i++) {
> 
> I agree we need to remove the numVerified == 0 condition.
> 
> For proper error messages, we cannot have the numVerified == i condition
> here, right? We need to keep verifying even after the first failed
> verification.

That is correct.

> 
> @@ +293,5 @@
> >      }
> >    }
> >  
> >    /* If we reached here and we verified at least one 
> >       signature, return success. */
> 
> This comment is out of date.
> 
> @@ +299,3 @@
> >      return CryptoX_Success;
> >    } else {
> >      fprintf(stderr, "ERROR: No signatures were verified.\n");
> 
> error message is out of date.
> 
> From my review of the other patches, I had thought that it would be
> impossible to reach this block. Are you sure this part of the patch is
> necessary in order for the tests to pass?

That block was refactored in the next bug so you already seen this change.

I'll submit a patch with these differences shortly.  It doesn't matter since most of it was refactored. I could mark this as a dupe and qfold, but we might as well have an explicit bug for the OR to AND change.

	Brian R. Bondy [:bbondy] Assignee
	Comment 23 • 12 years ago

Fixed comment and log message.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 24 • 12 years ago

I think you forgot to qfold the patch.

	Brian R. Bondy [:bbondy] Assignee
	Comment 25 • 12 years ago

Sorry selected the old one from my read only boot camp partition.
I did this in the other bug too but corrected myself there already.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 26 • 12 years ago

Comment on attachment 671697 [details] [diff] [review]
Patch v2'

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

Not quite sure why I'm r+ing this since the code was rewritten anyway, but if it makes things easier...

	Brian R. Bondy [:bbondy] Assignee
	Comment 27 • 12 years ago

You are r+ing because the change makes sense on its own as written on 2012-10-01 08:42 PDT.  Follow up tasks that defined other objectives in Comment 0 refactored the code, but the goals in this bug were met in this patch.

	Brian R. Bondy [:bbondy] Assignee
	Comment 28 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/29f170efc8b7

	Ryan VanderMeulen [:RyanVM]
	Comment 29 • 12 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/691971c2fffb