Opened 8 years ago

Closed 6 years ago

#79 closed defect (fixed)

Precertificate signature must be over something other than just the TBSCertificate

Reported by: rob.stradling@… Owned by: rob.stradling@…
Priority: blocker Milestone: review
Component: rfc6962-bis Version:
Severity: - Keywords:
Cc:

Description

If I understand the CMS spec correctly, then we're currently defining a Precertificate to be a CMS structure that contains a TBSCertificate and a signature over just that TBSCertificate.
That means that the components of a Precertificate can be trivially rearranged into an X.509 certificate with a valid signature!

To fix this, we need to either...

Require the SignedData?.encapContentInfo.eContent field to contain "something
TBSCertificate" or "TBSCertificate something".

or

Require a signed attribute to be present in SignedData?.signerInfos[0].signedAttrs. This is essentially equivalent to "TBSCertificate
something" in terms of what gets signed.
I think 2 is the cleaner solution, unless there's a cryptographic reason to prefer "something
TBSCertificate" (e.g. to protect against chosen prefix collisions?)

Change History (15)

comment:1 Changed 8 years ago by rob.stradling@…

  • Owner changed from draft-ietf-trans-rfc6962-bis@… to rob.stradling@…
  • Status changed from new to assigned

comment:2 follow-up: Changed 8 years ago by benl@…

It is unclear to me whether this is really a problem.

RFC 5652 says: "The input to the signature generation process includes the result of

the message digest calculation process and the signer's private key.
The details of the signature generation depend on the signature
algorithm employed."

It then defers to RFC 5280 for the actual definition of the algorithms.

AFAIK, RFC 5280 only specifies algorithms that are intended to work on bulk data (i.e. TBSCertificates), and hence they always digest the data first.

This would imply that the signature would be over a digest of a digest. However, its not the clearest RFC I ever read. If that interpretation is correct, then there is no problem.

If there is a problem, I agree that option 2 is better.

comment:3 in reply to: ↑ 2 Changed 8 years ago by rob.stradling@…

Replying to benl@…:

It is unclear to me whether this is really a problem.

OK, further investigation required.

<snip>

If there is a problem, I agree that option 2 is better.

Assuming for now that there is a problem...

Erwann suggests:
"Specify that the EncapsulatedContentInfo MUST be some specific value (allocate an OID from IETF/IANA/whatever).
RFC5652 states that if the EncapsulatedContentInfo is not id-data, then the signedAttrs MUST be present (whence solution 2 will apply)."

RFC5652 says that when signedAttrs is present...

"it MUST contain, at a minimum, the following two attributes:

A content-type attribute having as its value the content type
of the EncapsulatedContentInfo value being signed. Section
11.1 defines the content-type attribute. However, the
content-type attribute MUST NOT be used as part of a
countersignature unsigned attribute as defined in Section 11.4.

A message-digest attribute, having as its value the message
digest of the content. Section 11.2 defines the message-digest
attribute."

Putting just those two required attributes in signedAttrs seems sufficient.

comment:4 Changed 8 years ago by benl@…

I don't think any action is required. The I-D already says:

"A precertificate is a CMS signed-data object that contains a TBSCertificate in its SignedData?.encapContentInfo.eContent field,
identified by the OID <TBD> in the SignedData?.encapContentInfo.eContentType field."

In other words: option 2 is what the I-D already propose.

comment:5 Changed 8 years ago by rob.stradling@…

OK, I agree that option 2 is what the I-D already proposes. However, in the interest of helping implementers to avoid missing this requirement, I suggest we add a note to -bis along the lines of...

"Note that RFC5652 requires a content-type attribute and a message-digest attribute to be included in signedAttrs."

comment:6 Changed 8 years ago by benl@…

  • Milestone set to review

After discussion with Rob, core point is that the CMS signature is not an X509 signature.

Fixed at https://github.com/google/certificate-transparency-rfcs/commit/546e6e9451186e96ddd7b54ca02f17c8d86f951e.

comment:7 Changed 8 years ago by melinda.shore@…

  • Resolution set to fixed
  • Status changed from assigned to closed

comment:8 Changed 8 years ago by eranm@…

  • Resolution fixed deleted
  • Status changed from closed to reopened

Ben, I note that the github commit you link to only fix spelling of precertificates, not adopting Rob's proposal, is that intentional?

comment:9 Changed 8 years ago by rob.stradling@…

Ben wrote "Fixed at", not "Fixed by". He merged 3 commits to master to address this ticket. To review those 3 commits, see here:

https://github.com/google/certificate-transparency-rfcs/pull/48

comment:10 Changed 8 years ago by eranm@…

My bad, thanks for pointing out.
I still don't see your suggested text there though?

comment:11 Changed 8 years ago by rob.stradling@…

I had misunderstood the CMS spec when I wrote this ticket's Description. It turned out that the issue didn't actually exist. Ben and I agreed that we should just add a quick note to that effect.

comment:12 Changed 8 years ago by rob.stradling@…

  • Resolution set to fixed
  • Status changed from reopened to closed

Resolving fixed, since this was reviewed previously.

comment:13 Changed 6 years ago by rob.stradling@…

  • Milestone review deleted
  • Resolution fixed deleted
  • Status changed from closed to reopened

I recently took another look at how we're using CMS for precertificates in 6962-bis, and I found myself repeating the same misunderstandings that are documented earlier in this ticket. Therefore, I thought it would be wise to explain the requirements in more detail, drawing particular attention to the RFC5652 requirement that the signedAttrs field MUST be included:

https://github.com/google/certificate-transparency-rfcs/pull/264

comment:15 Changed 6 years ago by melinda.shore@…

  • Resolution set to fixed
  • Status changed from reopened to closed
Note: See TracTickets for help on using tickets.