June 18, 2020
Security Review of CFSSL Signer Code
Certificate signing is the most security-sensitive task performed by a certificate authority. The CA has to sign values, like DNS names, that are provided by untrusted sources. The CA must rigorously validate these values before signing them. If an attacker can bypass validation and get untrusted data included in a certificate, the results can be dire. For example, if an attacker can trick a CA into including an arbitrary SAN extension, they can get a certificate for domains they don't control.
Unfortunately, there is a history of CAs including unvalidated information in certificates. A common cause is CAs copying information directly from CSRs instead of from more constrained information sources. Since CSRs can contain both subject identity information and arbitrary certificate extensions, directly ingesting CSRs is extremely error-prone for CAs. For this reason, CAs would be well-advised to extract the public key from the CSR very early in the certificate enrollment process and discard everything else. In a perfect world, CAs would accept standalone public keys from subscribers instead of CSRs. (Before you say "proof-of-possession", see the appendix.)
I decided to review the signing code in CFSSL, the open source PKI toolkit used by Let's Encrypt's Boulder, to see how it stacks up against this advice. Unfortunately, I found that CFSSL copies subject identity information from CSRs by default, has features that are hard to use safely, and uses complicated logic that obfuscates what is included in certificate fields. I recommend that publicly-trusted CAs not use CFSSL.
Update: since publication of this post, Let's Encrypt has begun moving away from CFSSL!
Scope of review
I reviewed the CFSSL Git repository as of commit 6b49beae21ff90a09aea3901741ef02b1057ee65 (the HEAD of master at the time of my review). I reviewed the code in the signer and signer/local packages.
The signing operation
In CFSSL, you sign a certificate by invoking the Sign
function on the signer.Signer
interface, which has
this signature:
Sign(req SignRequest) (cert []byte, err error)
There is only one actual implementation of signer.Signer
:
local.Signer
. (The other implementations, remote.Signer
and
universal.Signer
, are ultimately wrappers around local.Signer
.)
Inputs to the signing operation
At a high level, inputs to the signing operation come from three to four places:
-
The
Signer
object, which contains:- The private key and certificate of the CA
- A list of named profiles plus a default profile
- A signature algorithm
I will refer to this object as
Signer
. -
The
SignRequest
argument, whose relevant fields are:Hosts []string Request string // The CSR Subject *Subject Profile string CRLOverride string Serial *big.Int Extensions []Extension NotBefore time.Time NotAfter time.Time
I will refer to this object as
SignRequest
. -
The
Signer
's default certificate profile, represented by an instance of the SigningProfile struct. I will refer to the default profile asdefaultProfile
. -
The effective certificate profile, represented by an instance of the SigningProfile struct. I will refer to the effective profile as
profile
. If the profile named bySignRequest.Profile
exists inSigner
, thenprofile
is that profile. If it doesn't exist, thenprofile
equalsdefaultProfile
.
The Sign
function takes values from these places and combines them to produce the input to x509.CreateCertificate
in Go's standard library.
There is overlap - for instance SANs can be specified in the CSR, SignRequest.Hosts
, or
SignRequest.Extensions
. How does Sign
decide which source to use when constructing the certificate?
Certificate construction logic
To understand how Sign
works, I looked at each certificate field and worked backwards to figure out
how Sign
decides to populate the field. Below are my findings.
Serial Number
- If
profile.ClientProvidesSerialNumbers
is true: useSignRequest.Serial
(error if not set). - Else: generate a random 20 byte serial number.
Not Before
- If
SignRequest.NotBefore
is non-zero: use it. - Else if
profile.NotBefore
is non-zero: use it. - Else if
profile.Backdate
is non-zero: usecurrent time - profile.Backdate
. - Else: use
current time - 5 minutes
.
Not After
- If
SignRequest.NotAfter
is non-zero: use it. - Else if
profile.NotAfter
is non-zero: use it. - Else if
profile.Expiry
is non-zero: usenot before + profile.Expiry
. - Else: use
not before + defaultProfile.Expiry
.
Signature Algorithm
- If
profile.CSRWhitelist
is nil orprofile.CSRWhitelist.SignatureAlgorithm
is true: UseSigner
's signature algorithm. - Else: CFSSL leaves the signature algorithm unspecified and the Go standard library picks a sensible algorithm.
Comments: it's weird how something named CSRWhitelist
is used to decide whether to use a value that comes not from the CSR, but from Signer
. This is probably because CFSSL's ParseCertificateRequest
function gets this field from Signer
rather than from the CSR that it is parsing. This sort of indirection and misleading naming makes the code hard to understand.
Public Key
- If
profile.CSRWhitelist
is nil orprofile.CSRWhitelist.PublicKey
is is true: Use the CSR's public key. - Else: the certificate won't have a public key (this probably causes
x509.CreateCertificate
to return an error).
Comments: it's unclear why you'd ever want profile.CSRWhitelist.PublicKey
to be false. The public key is literally the only piece of information that should be taken from the CSR.
SANs
This one's a doozy...
- If
profile.CopyExtensions
is true andprofile.CSRWhitelist
is nil and the CSR contains a SAN extension andSignRequest.Extensions
contains a SAN extension, and the SAN OID is present inprofile.ExtensionWhitelist
: add two SAN extensions to the certificate, one from the CSR and one fromSignRequest.Extensions
. Note thatSignRequest.Hosts
is ignored andprofile.NameWhitelist
is bypassed. - Else if
profile.CopyExtensions
is true andprofile.CSRWhitelist
is nil and the CSR contains a SAN extension: use the SAN extension verbatim from the CSR. Note thatSignRequest.Hosts
is ignored andprofile.NameWhitelist
is bypassed. - Else if
SignRequest.Extensions
contains a SAN extension, and the SAN OID is present inprofile.ExtensionWhitelist
: use the SAN extension verbatim fromSignRequest.Extensions
. Note thatSignRequest.Hosts
is ignored andprofile.NameWhitelist
is bypassed. - Else if
profile.CAConstraint.IsCA
is true: the certificate will not contain a SAN extension. - Else if
SignRequest.Hosts
is non-nil:- Use each string in
SignRequest.Hosts
as follows:- If string parses as an IP address: make it an IP Address SAN.
- Else if string parses as an email address: make it an email address SAN.
- Else if string parses as a URI, make it a URI SAN.
- Else: make it a DNS SAN.
- If
profile.NameWhitelist
is non-nil: return an error unless the string representation of every DNS, email, and URI SAN matches theprofile.NameWhitelist
regex (IP address SANs are not checked).
- Use each string in
- Else if
profile.CSRWhitelist
is nil and the CSR contains a SAN extension:- Copy the DNS names, IP addresses, email addresses, and URIs from the CSR's SAN extension.
- If
profile.NameWhitelist
is non-nil: enforce whitelist as described above.
- Else if
profile.CSRWhitelist
is non-nil and the CSR contains a SAN extension:- If
profile.CSRWhitelist.DNSNames
is true: use DNS names from the CSR's SAN extension. - If
profile.CSRWhitelist.IPAddresses
is true: use IP addresses from the CSR's SAN extension. - If
profile.CSRWhitelist.EmailAddresses
is true: use email addresses from the CSR's SAN extension. - If
profile.CSRWhitelist.URIs
is true: use URIs from the CSR's SAN extension. - If
profile.NameWhitelist
is non-nil: enforce whitelist as described above.
- If
Subject
For each supported subject attribute (common name, country, province, locality, organization, organizational unit, serial number):
- If the attribute was specified in
SignRequest.Subject
: use it. - Else if
profile.CSRWhitelist
is nil orprofile.CSRWhitelist.Subject
is true: use the attribute from the CSR's subject, if present.
Common name only: if profile.NameWhitelist
is non-nil: return an error unless the common name matches the profile.NameWhitelist
regex.
Note: SignRequest.Hosts
does not override the common name.
Basic Constraints
- If
SignRequest.Extensions
contains a basic constraints extension, and the basic constraints OID is present inprofile.ExtensionWhitelist
: copy the basic constraints extension verbatim fromSignRequest.Extensions
. - Else: use the values from
profile.CAConstraint
.
Comments: given how security-sensitive this extension is, it's a relief that there's no way for the value to come from the CSR. Despite this, there is code earlier in the signing process that looks at the CSR's Basic Constraints extension. First it's extracted from the CSR in ParseCertificateRequest
and then it's validated in Sign
. This code ultimately has no effect, but it makes the logic harder to follow (and gave me a mild heart attack when I saw it).
Extensions besides SAN and Basic Constraints
For a given extension FOO
:
- If
profile.CopyExtensions
is true andprofile.CSRWhitelist
is nil and the CSR contains aFOO
extension andSignRequest.Extensions
contains aFOO
extension, andFOO
is present inprofile.ExtensionWhitelist
: add twoFOO
extensions to the certificate, one from the CSR and one fromSignRequest.Extensions
. Note that fields inSignRequest
(likeCRLOverride
) orprofile
(likeOCSP
,CRL
, etc.) that would normally control theFOO
extension are ignored. - Else if
profile.CopyExtensions
is true andprofile.CSRWhitelist
is nil and the CSR contains aFOO
extension: copy it verbatim from the CSR. Note that fields inSignRequest
(likeCRLOverride
) orprofile
(likeOCSP
,CRL
, etc.) that would normally control theFOO
extension are ignored. - Else if
SignRequest.Extensions
contains aFOO
extension, andFOO
is present inprofile.ExtensionWhitelist
: copy it verbatim to the certificate. Note that fields inSignRequest
(likeCRLOverride
) orprofile
(likeOCSP
,CRL
, etc.) that would normally control theFOO
extension are ignored. - Else: use fields from
SignRequest
(likeCRLOverride
) andprofile
(likeOCSP
,CRL
, etc.) to decide what value the extension should have, if any.
Other comments
By default, CSRWhitelist
is nil. This is a bad default, as it means SANs will be copied from the CSR unless SignRequest.Hosts
is set. Likewise, any subject attribute not specified in SignRequest.Subject
will be copied from the CSR. This is practically impossible to use safely: to avoid including unvalidated subject information you have to specify a value for every attribute in SignRequest.Subject
- and if you don't want the attribute included in the final certificate you're out of luck. If CFSSL ever adds support for a new attribute type, you had better update your code to specify a value for the attribute or unvalidated information might slip through. This is exactly the sort of logic that makes it so easy to accidentally issue certificates with "Some-State" in the subject.
If the profile specified by SignRequest.Profile
doesn't exist, the default profile is used. This could lead to an unexpected certificate profile being used if a CA deletes a profile from their configuration but there are still references to it elsewhere. Considering the trouble that CAs have with profile management (see the infamous TURKTRUST incident or the CA that discovered they had a whopping 85 buggy profiles), I think it would be much safer if a non-existent profile resulted in an error.
SignRequest.Hosts
is untyped - everything is a string and there is no distinction between IP addresses, email addresses, URIs, and DNS names. (Also, Hosts
is a misleading name because URIs and email addresses aren't hosts.) CFSSL decides what type of SAN to include based on what the string in Hosts
successfully parses as, and assumes it's a DNS name if it doesn't parse as anything else. This could lead to unexpected SAN types in the certificate. Determining if a string was intended to be a URI by trying to parse it is an especially bad idea considering how hellish URIs are to parse, and how much variation there is between different URI parsing implementations. If the user of CFSSL adds a string which they believe to be a valid URI to SignRequest.Hosts
, but Go's URI parser rejects it, the URI will end up in a DNS SAN instead.
Variable names are inconsistent and often unhelpful. In Sign
, req
is used for values from SignRequest
and safeTemplate
is used for values from the CSR. But in PopulateSubjectFromCSR
(which is called by Sign
), req
is used for values from the CSR, and s
is used for values from the SignRequest. This increases the likelihood of accidentally using data from the wrong source.
ParseCertificateRequest
blindly and unconditionally copies the extensions from the CSR to the Extensions
field of the x509.Certificate
template - even if profile.CopyExtensions
is false. Fortunately, this field is ignored by x509.CreateCertificate
so it's probably harmless. It just means that attacker-controlled input is propagated further through the program, increasing the opportunity for it to be misused.
CopyExtensions is a foot cannon
I am extremely concerned by the presence of the CopyExtensions
option. Enabling it practically guarantees misissuance because all extensions (except Basic Constraints) are copied verbatim from the CSR, overriding any value specified in the profile or the SignRequest
. In particular, SignRequest.Hosts
and profile.NameWhitelist
are ignored if the CSR contains a SAN extension. Also, profile.ExtensionWhitelist
only applies to extensions specified in SignRequest
- not those specified in the CSR. I think it's quite likely that users of CopyExtensions
will be surprised when neither of these whitelists are effective.
Lack of documentation
As I showed above, the logic for constructing a certificate is very complicated, and you have to use CFSSL in exactly the right way to avoid copying unvalidated information from CSRs. Unfortunately, documentation is practically non-existent and I could only figure out CFSSL's logic by reading the source code. Obviously, the lack of documentation makes it hard to use CFSSL safely. But the more fundamental problem is that documentation writing wasn't a core part of CFSSL's engineering process. Had documentation been written in tandem with the design and implementation of CFSSL, it would have been evident that incomprehensibility was spiraling out of control. This information could have been fed back into the engineering process and used to redesign or even reject features that made the system too hard to understand. I have personally saved myself many times from releasing overly-complicated software just by writing the documentation for it.
Final thoughts
CFSSL has some nice features, like its friendly command line interface
and its certificate bundler for building optimal certificate chains.
However, I struggle to see the value provided by its signer package.
Its truly useful functionality, like Certificate Transparency submission and pre-issuance
linting, could be extracted into standalone libraries. The rest of the signer
is just a complicated wrapper around Go's x509.CreateCertificate
that obscures what gets included in certificates and will include the wrong thing if you hold it wrong.
A long history of misissuance shows us why we need better.
If you're a CA, just call x509.CreateCertificate
directly - it will be much easier to ensure
you are only including validated information in your certificates.
Appendix: Proof-of-Possession and TLS
A common but unfounded objection to discarding everything in a CSR except the public key is that checking the CSR's signature is necessary because it ensures proof-of-possession of the private key. If a CA doesn't verify proof-of-possession, then someone could obtain a certificate for a key which belongs to someone else. (In fact, someone recently got a certificate containing Let's Encrypt's public key.) For TLS, this doesn't matter. (Other protocols, like S/MIME, may be different.) The TLS protocol ensures proof-of-possession every time the certificate is used.
For TLS 1.3, this is easy to see: the server or client has to send a Certificate Verify message which contains a signature from their private key over a transcript of the handshake. The handshake includes their certificate, which is a superset of the information in a CSR. Therefore, the Certificate Verify message proves at least as much as the CSR signature does. In fact it's better, since the proof is fresh and not reliant on a trusted third party doing its job correctly.
In earlier versions of TLS, client certificates are verified in the same way (signing a handshake transcript which includes the certificate). Server certificates are used differently, but ultimately the handshake transcript (which includes the server certificate) is authenticated by a shared secret that is known only to the client and the holder of the certificate private key (provided neither party deliberately sabotages their security). So as with TLS 1.3, private key possession is proven, rendering the CSR signature unnecessary.
Post a Comment
Your comment will be public. To contact me privately, email me. Please keep your comment polite, on-topic, and comprehensible. Your comment may be held for moderation before being published.
Comments
No comments yet.