Closed Bug 795921 Opened 7 years ago Closed 7 years ago

Change MAR verification to use AND semantics for multiple signatures (not B2G specific)

Categories

(Toolkit :: Application Update, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19
blocking-basecamp +
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: bbondy, Assigned: bbondy)

References

Details

Attachments

(1 file, 2 obsolete files)

+++ 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.
Attached patch Patch v1. (obsolete) — Splinter Review
Attachment #666569 - Flags: review?(bsmith)
I will update the documentation once I land the patch by the way.
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.
(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.)
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?
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.
(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.
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.
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.
(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.
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.
(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?
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.
.. because each of the 3 certs on the device will be used exactly one time.
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.
No longer blocks: 793709
Summary: Change MAR verification to use AND semantics for multiple signatures → Change MAR verification to use AND semantics for multiple signatures (not B2G specific)
No longer blocks: b2g-fota-updates
Blocks: 793709
Do we know that we're going to do this, Brian (B.)?
Flags: needinfo?(netzen)
Yes I think this will be landing no matter what happens with B2G updates.
Flags: needinfo?(netzen)
Is this still needed after the other changes I already reviewed? I thought the other changes already incorporated this?
Yes it is still needed. This is a dependency to the main libmar work bug, and so should land before that lands.
It'll land at the same time, but the libmar work won't apply without this.
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?
Attachment #666569 - Flags: review?(bsmith) → review-
(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.
Attached patch Patch v2 (obsolete) — Splinter Review
Fixed comment and log message.
Attachment #666569 - Attachment is obsolete: true
Attachment #671688 - Flags: review?(bsmith)
I think you forgot to qfold the patch.
Attached patch Patch v2'Splinter Review
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.
Attachment #671688 - Attachment is obsolete: true
Attachment #671688 - Flags: review?(bsmith)
Attachment #671697 - Flags: review?(bsmith)
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...
Attachment #671697 - Flags: review?(bsmith) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/29f170efc8b7
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.