-
Notifications
You must be signed in to change notification settings - Fork 565
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
cert-manager: [Medium] patch for CVE-2024-12401 (#12013)
Co-authored-by: jslobodzian <[email protected]>
- Loading branch information
1 parent
e7b95d9
commit c3f7102
Showing
2 changed files
with
370 additions
and
3 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,363 @@ | ||
| From 776bf7178df5f40d1c64c9691aec8186fc493fe0 Mon Sep 17 00:00:00 2001 | ||
| From: Kevin Lockwood <[email protected]> | ||
| Date: Tue, 21 Jan 2025 14:13:08 -0800 | ||
| Subject: [PATCH] [Medium] fix CVE-2024-12401 | ||
|
|
||
| Link: https://github.com/cert-manager/cert-manager/pull/7403 | ||
| --- | ||
| internal/controller/certificates/secrets.go | 10 +- | ||
| internal/pem/decode.go | 125 ++++++++++++++++++++ | ||
| pkg/controller/acmeorders/sync.go | 20 ++-- | ||
| pkg/util/pki/csr.go | 13 +- | ||
| pkg/util/pki/parse.go | 29 +++-- | ||
| 5 files changed, 170 insertions(+), 27 deletions(-) | ||
| create mode 100644 internal/pem/decode.go | ||
|
|
||
| diff --git a/internal/controller/certificates/secrets.go b/internal/controller/certificates/secrets.go | ||
| index 0a401c2..f2f1852 100644 | ||
| --- a/internal/controller/certificates/secrets.go | ||
| +++ b/internal/controller/certificates/secrets.go | ||
| @@ -19,9 +19,9 @@ package certificates | ||
| import ( | ||
| "bytes" | ||
| "crypto/x509" | ||
| - "encoding/pem" | ||
| "strings" | ||
|
|
||
| + "github.com/cert-manager/cert-manager/internal/pem" | ||
| apiutil "github.com/cert-manager/cert-manager/pkg/api/util" | ||
| cmapi "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" | ||
| utilpki "github.com/cert-manager/cert-manager/pkg/util/pki" | ||
| @@ -54,7 +54,13 @@ func AnnotationsForCertificateSecret(crt *cmapi.Certificate, certificate *x509.C | ||
| // OutputFormatDER returns the byte slice of the private key in DER format. To | ||
| // be used for Certificate's Additional Output Format DER. | ||
| func OutputFormatDER(privateKey []byte) []byte { | ||
| - block, _ := pem.Decode(privateKey) | ||
| + // NOTE: This call to pem.SafeDecodePrivateKey ignores errors. | ||
| + // This is acceptable here since we're calling this function only on PEM data which we created | ||
| + // by encoding the private key. As such, we can be fairly confident that: | ||
| + // 1) The PEM is valid | ||
| + // 2) The PEM isn't attacker-controlled (and as such unsafe to decode) | ||
| + | ||
| + block, _, _ := pem.SafeDecodePrivateKey(privateKey) | ||
| return block.Bytes | ||
| } | ||
|
|
||
| diff --git a/internal/pem/decode.go b/internal/pem/decode.go | ||
| new file mode 100644 | ||
| index 0000000..1042f98 | ||
| --- /dev/null | ||
| +++ b/internal/pem/decode.go | ||
| @@ -0,0 +1,125 @@ | ||
| +/* | ||
| +Copyright 2024 The cert-manager Authors. | ||
| + | ||
| +Licensed under the Apache License, Version 2.0 (the "License"); | ||
| +you may not use this file except in compliance with the License. | ||
| +You may obtain a copy of the License at | ||
| + | ||
| + http://www.apache.org/licenses/LICENSE-2.0 | ||
| + | ||
| +Unless required by applicable law or agreed to in writing, software | ||
| +distributed under the License is distributed on an "AS IS" BASIS, | ||
| +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| +See the License for the specific language governing permissions and | ||
| +limitations under the License. | ||
| +*/ | ||
| + | ||
| +// Package pem provides utility functions for safely decoding PEM data, placing upper limits on the size | ||
| +// of data that will be processed. It functions as an extension to the standard library "encoding/pem" functions. | ||
| +package pem | ||
| + | ||
| +import ( | ||
| + stdpem "encoding/pem" | ||
| + "errors" | ||
| + "fmt" | ||
| +) | ||
| + | ||
| +// The constants below are estimates at reasonable upper bounds for sizes of PEM data that cert-manager might encounter. | ||
| +// cert-manager supports RSA, ECDSA and Ed25519 keys, of which RSA keys are by far the largest. | ||
| + | ||
| +// We'll aim to support RSA certs / keys which are larger than the maximum size (defined in pkg/util/pki.MaxRSAKeySize). | ||
| + | ||
| +// RSA keys grow proportional to the size of the RSA key used. For example: | ||
| +// PEM-encoded RSA Keys: 4096-bit is ~3kB, 8192-bit is ~6kB and a 16k-bit key is ~12kB. | ||
| + | ||
| +// Certificates have two variables; the public key of the cert, and the signature from the signing cert. | ||
| +// An N-bit key produces an N-byte signature, so as a worst case for us, a 16kB RSA key will create a 2kB signature. | ||
| + | ||
| +// PEM-encoded RSA X.509 certificates: | ||
| +// Signed with 1k-bit RSA key: 4096-bit is ~1.4kB, 8192-bit is ~2kB, 16k-bit is ~3.5kB | ||
| +// Signed with 16k-bit RSA key: 4096-bit is ~3.3kB, 8192-bit is ~4kB, 16k-bit is ~5.4kB | ||
| + | ||
| +// See https://fm4dd.com/openssl/certexamples.shtm for examples of large RSA certs / keys | ||
| + | ||
| +const ( | ||
| + // maxCertificatePEMSize is the maximum size, in bytes, of a single PEM-encoded X.509 certificate which SafeDecodeSingleCertificate will accept. | ||
| + // The value is based on how large a "realistic" (but still very large) self-signed 16k-bit RSA certficate might be. | ||
| + // 16k-bit RSA keys are impractical on most on modern hardware due to how slow they can be, | ||
| + // so we can reasonably assume that no real-world PEM-encoded X.509 cert will be this large. | ||
| + // Note that X.509 certificates can contain extra abitrary data (e.g. DNS names, policy names, etc) whose size is hard to predict. | ||
| + // So we guess at how much of that data we'll allow in very large certs and allow about 1kB of such data. | ||
| + maxCertificatePEMSize = 6500 | ||
| + | ||
| + // maxPrivateKeyPEMSize is the maximum size, in bytes, of PEM-encoded private keys which SafeDecodePrivateKey will accept. | ||
| + // cert-manager supports RSA, ECDSA and Ed25519 keys, of which RSA is by far the largest. | ||
| + // The value is based on how large a "realistic" (but very large) 16k-bit RSA private key might be. | ||
| + // Given that 16k-bit RSA keys are so slow to use as to be impractical on modern hardware, | ||
| + // we can reasonably assume that no real-world PEM-encoded key will be this large. | ||
| + maxPrivateKeyPEMSize = 13000 | ||
| + | ||
| + // maxChainSize is the maximum number of 16k-bit RSA certificates signed by 16k-bit RSA CAs we'll allow in a given call to SafeDecodeMultipleCertificates. | ||
| + // This is _not_ the maximum number of certificates cert-manager will process in a given chain, which could be much larger. | ||
| + // This is simply the maximum number of worst-case certificates we'll accept in a chain. | ||
| + maxChainSize = 10 | ||
| +) | ||
| + | ||
| +var ( | ||
| + // ErrNoPEMData is returned when the given data contained no PEM | ||
| + ErrNoPEMData = errors.New("no PEM data was found in given input") | ||
| +) | ||
| + | ||
| +// ErrPEMDataTooLarge is returned when the given data is larger than the maximum allowed | ||
| +type ErrPEMDataTooLarge int | ||
| + | ||
| +// Error returns an error string | ||
| +func (e ErrPEMDataTooLarge) Error() string { | ||
| + return fmt.Sprintf("provided PEM data was larger than the maximum %dB", int(e)) | ||
| +} | ||
| + | ||
| +func safeDecodeInternal(b []byte, maxSize int) (*stdpem.Block, []byte, error) { | ||
| + if len(b) > maxSize { | ||
| + return nil, b, ErrPEMDataTooLarge(maxSize) | ||
| + } | ||
| + | ||
| + block, rest := stdpem.Decode(b) | ||
| + if block == nil { | ||
| + return nil, rest, ErrNoPEMData | ||
| + } | ||
| + | ||
| + return block, rest, nil | ||
| +} | ||
| + | ||
| +// SafeDecodePrivateKey calls [encoding/pem.Decode] on the given input as long as it's within a sensible range for | ||
| +// how large we expect a private key to be. The baseline is a 16k-bit RSA private key, which is larger than the maximum | ||
| +// supported by cert-manager for key generation. | ||
| +func SafeDecodePrivateKey(b []byte) (*stdpem.Block, []byte, error) { | ||
| + return safeDecodeInternal(b, maxPrivateKeyPEMSize) | ||
| +} | ||
| + | ||
| +// SafeDecodeCSR calls [encoding/pem.Decode] on the given input as long as it's within a sensible range for | ||
| +// how large we expect a single PEM-encoded PKCS#10 CSR to be. | ||
| +// We assume that a PKCS#12 CSR is smaller than a single certificate because our assumptions are that | ||
| +// a certificate has a large public key and a large signature, which is roughly the case for a CSR. | ||
| +// We also assume that we'd only ever decode one CSR which is the case in practice. | ||
| +func SafeDecodeCSR(b []byte) (*stdpem.Block, []byte, error) { | ||
| + return safeDecodeInternal(b, maxCertificatePEMSize) | ||
| +} | ||
| + | ||
| +// SafeDecodeSingleCertificate calls [encoding/pem.Decode] on the given input as long as it's within a sensible range for | ||
| +// how large we expect a single PEM-encoded X.509 certificate to be. | ||
| +// The baseline is a 16k-bit RSA certificate signed by a different 16k-bit RSA CA, which is larger than the maximum | ||
| +// supported by cert-manager for key generation. | ||
| +func SafeDecodeSingleCertificate(b []byte) (*stdpem.Block, []byte, error) { | ||
| + return safeDecodeInternal(b, maxCertificatePEMSize) | ||
| +} | ||
| + | ||
| +// SafeDecodeMultipleCertificates calls [encoding/pem.Decode] on the given input as long as it's within a sensible range for | ||
| +// how large we expect a reasonable-length PEM-encoded X.509 certificate chain to be. | ||
| +// The baseline is several 16k-bit RSA certificates, all signed by 16k-bit RSA keys, which is larger than the maximum | ||
| +// supported by cert-manager for key generation. | ||
| +// The maximum number of chains supported by this function is not reflective of the maximum chain length supported by | ||
| +// cert-manager; a larger chain of smaller certificate should be supported. | ||
| +func SafeDecodeMultipleCertificates(b []byte) (*stdpem.Block, []byte, error) { | ||
| + return safeDecodeInternal(b, maxCertificatePEMSize*maxChainSize) | ||
| +} | ||
| + | ||
| diff --git a/pkg/controller/acmeorders/sync.go b/pkg/controller/acmeorders/sync.go | ||
| index 6cde88a..0557174 100644 | ||
| --- a/pkg/controller/acmeorders/sync.go | ||
| +++ b/pkg/controller/acmeorders/sync.go | ||
| @@ -20,7 +20,7 @@ import ( | ||
| "bytes" | ||
| "context" | ||
| "crypto/x509" | ||
| - "encoding/pem" | ||
| + stdpem "encoding/pem" | ||
| "fmt" | ||
| "time" | ||
|
|
||
| @@ -36,6 +36,7 @@ import ( | ||
|
|
||
| "github.com/cert-manager/cert-manager/internal/controller/feature" | ||
| internalorders "github.com/cert-manager/cert-manager/internal/controller/orders" | ||
| + "github.com/cert-manager/cert-manager/internal/pem" | ||
| "github.com/cert-manager/cert-manager/pkg/acme" | ||
| acmecl "github.com/cert-manager/cert-manager/pkg/acme/client" | ||
| cmacme "github.com/cert-manager/cert-manager/pkg/apis/acme/v1" | ||
| @@ -498,13 +499,18 @@ func (c *controller) finalizeOrder(ctx context.Context, cl acmecl.Interface, o * | ||
| // only supported DER encoded CSRs and not PEM encoded as they are intended | ||
| // to be as part of our API. | ||
| // To work around this, we first attempt to decode the Request into DER bytes | ||
| - // by running pem.Decode. If the PEM block is empty, we assume that the Request | ||
| + // by running pem.SafeDecodeCSR. If the PEM block is empty, we assume that the Request | ||
| // is DER encoded and continue to call FinalizeOrder. | ||
| var derBytes []byte | ||
| - block, _ := pem.Decode(o.Spec.Request) | ||
| - if block == nil { | ||
| - log.V(logf.WarnLevel).Info("failed to parse Request as PEM data, attempting to treat Request as DER encoded for compatibility reasons") | ||
| - derBytes = o.Spec.Request | ||
| + block, _, err := pem.SafeDecodeCSR(o.Spec.Request) | ||
| + | ||
| + if err != nil { | ||
| + if err == pem.ErrNoPEMData { | ||
| + log.V(logf.WarnLevel).Info("failed to parse Request as PEM data, attempting to treat Request as DER encoded for compatibility reasons") | ||
| + derBytes = o.Spec.Request | ||
| + } else { | ||
| + return err | ||
| + } | ||
| } else { | ||
| derBytes = block.Bytes | ||
| } | ||
| @@ -589,7 +595,7 @@ func (c *controller) storeCertificateOnStatus(ctx context.Context, o *cmacme.Ord | ||
| // encode the retrieved certificates (including the chain) | ||
| certBuffer := bytes.NewBuffer([]byte{}) | ||
| for _, cert := range certs { | ||
| - err := pem.Encode(certBuffer, &pem.Block{Type: "CERTIFICATE", Bytes: cert}) | ||
| + err := stdpem.Encode(certBuffer, &stdpem.Block{Type: "CERTIFICATE", Bytes: cert}) | ||
| if err != nil { | ||
| log.Error(err, "invalid certificate data returned by ACME server") | ||
| c.setOrderState(&o.Status, string(cmacme.Errored)) | ||
| diff --git a/pkg/util/pki/csr.go b/pkg/util/pki/csr.go | ||
| index bb4776f..b2173d5 100644 | ||
| --- a/pkg/util/pki/csr.go | ||
| +++ b/pkg/util/pki/csr.go | ||
| @@ -23,7 +23,7 @@ import ( | ||
| "crypto/x509" | ||
| "crypto/x509/pkix" | ||
| "encoding/asn1" | ||
| - "encoding/pem" | ||
| + stdpem "encoding/pem" | ||
| "errors" | ||
| "fmt" | ||
| "math/big" | ||
| @@ -33,6 +33,7 @@ import ( | ||
| "time" | ||
|
|
||
| "github.com/cert-manager/cert-manager/internal/controller/feature" | ||
| + "github.com/cert-manager/cert-manager/internal/pem" | ||
| apiutil "github.com/cert-manager/cert-manager/pkg/api/util" | ||
| v1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" | ||
| utilfeature "github.com/cert-manager/cert-manager/pkg/util/feature" | ||
| @@ -449,8 +450,8 @@ func GenerateTemplateFromCSRPEM(csrPEM []byte, duration time.Duration, isCA bool | ||
| } | ||
|
|
||
| func GenerateTemplateFromCSRPEMWithUsages(csrPEM []byte, duration time.Duration, isCA bool, keyUsage x509.KeyUsage, extKeyUsage []x509.ExtKeyUsage) (*x509.Certificate, error) { | ||
| - block, _ := pem.Decode(csrPEM) | ||
| - if block == nil { | ||
| + block, _, err := pem.SafeDecodeCSR(csrPEM) | ||
| + if err != nil { | ||
| return nil, errors.New("failed to decode csr") | ||
| } | ||
|
|
||
| @@ -512,7 +513,7 @@ func SignCertificate(template *x509.Certificate, issuerCert *x509.Certificate, p | ||
| } | ||
|
|
||
| pemBytes := bytes.NewBuffer([]byte{}) | ||
| - err = pem.Encode(pemBytes, &pem.Block{Type: "CERTIFICATE", Bytes: derBytes}) | ||
| + err = stdpem.Encode(pemBytes, &stdpem.Block{Type: "CERTIFICATE", Bytes: derBytes}) | ||
| if err != nil { | ||
| return nil, nil, fmt.Errorf("error encoding certificate PEM: %s", err.Error()) | ||
| } | ||
| @@ -558,7 +559,7 @@ func EncodeCSR(template *x509.CertificateRequest, key crypto.Signer) ([]byte, er | ||
| // EncodeX509 will encode a single *x509.Certificate into PEM format. | ||
| func EncodeX509(cert *x509.Certificate) ([]byte, error) { | ||
| caPem := bytes.NewBuffer([]byte{}) | ||
| - err := pem.Encode(caPem, &pem.Block{Type: "CERTIFICATE", Bytes: cert.Raw}) | ||
| + err := stdpem.Encode(caPem, &stdpem.Block{Type: "CERTIFICATE", Bytes: cert.Raw}) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| @@ -584,7 +585,7 @@ func EncodeX509Chain(certs []*x509.Certificate) ([]byte, error) { | ||
| continue | ||
| } | ||
|
|
||
| - err := pem.Encode(caPem, &pem.Block{Type: "CERTIFICATE", Bytes: cert.Raw}) | ||
| + err := stdpem.Encode(caPem, &stdpem.Block{Type: "CERTIFICATE", Bytes: cert.Raw}) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| diff --git a/pkg/util/pki/parse.go b/pkg/util/pki/parse.go | ||
| index dff3539..428b2e8 100644 | ||
| --- a/pkg/util/pki/parse.go | ||
| +++ b/pkg/util/pki/parse.go | ||
| @@ -22,8 +22,9 @@ import ( | ||
| "crypto/x509" | ||
| "crypto/x509/pkix" | ||
| "encoding/asn1" | ||
| - "encoding/pem" | ||
| + stdpem "encoding/pem" | ||
|
|
||
| + "github.com/cert-manager/cert-manager/internal/pem" | ||
| "github.com/cert-manager/cert-manager/pkg/util/errors" | ||
| "github.com/go-ldap/ldap/v3" | ||
| ) | ||
| @@ -32,8 +33,8 @@ import ( | ||
| // It supports ECDSA and RSA private keys only. All other types will return err. | ||
| func DecodePrivateKeyBytes(keyBytes []byte) (crypto.Signer, error) { | ||
| // decode the private key pem | ||
| - block, _ := pem.Decode(keyBytes) | ||
| - if block == nil { | ||
| + block, _, err := pem.SafeDecodePrivateKey(keyBytes) | ||
| + if err != nil { | ||
| return nil, errors.NewInvalidData("error decoding private key PEM block") | ||
| } | ||
|
|
||
| @@ -75,8 +76,8 @@ func DecodePrivateKeyBytes(keyBytes []byte) (crypto.Signer, error) { | ||
| // DecodePKCS1PrivateKeyBytes will decode a PEM encoded RSA private key. | ||
| func DecodePKCS1PrivateKeyBytes(keyBytes []byte) (*rsa.PrivateKey, error) { | ||
| // decode the private key pem | ||
| - block, _ := pem.Decode(keyBytes) | ||
| - if block == nil { | ||
| + block, _, err := pem.SafeDecodePrivateKey(keyBytes) | ||
| + if err != nil { | ||
| return nil, errors.NewInvalidData("error decoding private key PEM block") | ||
| } | ||
| // parse the private key | ||
| @@ -95,13 +96,17 @@ func DecodePKCS1PrivateKeyBytes(keyBytes []byte) (*rsa.PrivateKey, error) { | ||
| func DecodeX509CertificateChainBytes(certBytes []byte) ([]*x509.Certificate, error) { | ||
| certs := []*x509.Certificate{} | ||
|
|
||
| - var block *pem.Block | ||
| + var block *stdpem.Block | ||
| + var err error | ||
|
|
||
| for { | ||
| - // decode the tls certificate pem | ||
| - block, certBytes = pem.Decode(certBytes) | ||
| - if block == nil { | ||
| - break | ||
| + block, certBytes, err = pem.SafeDecodeMultipleCertificates(certBytes) | ||
| + if err != nil { | ||
| + if err == pem.ErrNoPEMData { | ||
| + break | ||
| + } | ||
| + | ||
| + return nil, err | ||
| } | ||
|
|
||
| // parse the tls certificate | ||
| @@ -131,8 +136,8 @@ func DecodeX509CertificateBytes(certBytes []byte) (*x509.Certificate, error) { | ||
|
|
||
| // DecodeX509CertificateRequestBytes will decode a PEM encoded x509 Certificate Request. | ||
| func DecodeX509CertificateRequestBytes(csrBytes []byte) (*x509.CertificateRequest, error) { | ||
| - block, _ := pem.Decode(csrBytes) | ||
| - if block == nil { | ||
| + block, _, err := pem.SafeDecodeCSR(csrBytes) | ||
| + if err != nil { | ||
| return nil, errors.NewInvalidData("error decoding certificate request PEM block") | ||
| } | ||
|
|
||
| -- | ||
| 2.34.1 | ||
|
|
Oops, something went wrong.