From ca4f4e32795734a24950070043901f420c29f347 Mon Sep 17 00:00:00 2001 From: Todd Short Date: Tue, 16 Jul 2024 16:33:36 -0400 Subject: [PATCH] Update certificate code to be more strict (#1056) Deal with malformed PEM that is skipped by the golang standard library Signed-off-by: Todd Short --- internal/httputil/certutil.go | 219 ++++++++++++++++++ .../{httputil_test.go => certutil_test.go} | 8 +- internal/httputil/httputil.go | 66 ------ testdata/certs/empty/empty.pem | 2 + testdata/certs/good/Amazon_Root_CA_2.pem | 1 + testdata/certs/ugly2/Amazon_Root_CA_1.pem | 20 ++ testdata/certs/ugly3/not_a_cert.pem | 1 + 7 files changed, 249 insertions(+), 68 deletions(-) create mode 100644 internal/httputil/certutil.go rename internal/httputil/{httputil_test.go => certutil_test.go} (58%) create mode 100644 testdata/certs/empty/empty.pem create mode 100644 testdata/certs/ugly2/Amazon_Root_CA_1.pem create mode 100644 testdata/certs/ugly3/not_a_cert.pem diff --git a/internal/httputil/certutil.go b/internal/httputil/certutil.go new file mode 100644 index 000000000..23e132164 --- /dev/null +++ b/internal/httputil/certutil.go @@ -0,0 +1,219 @@ +package httputil + +import ( + "bytes" + "crypto/x509" + "encoding/base64" + "encoding/pem" + "fmt" + "os" + "path/filepath" +) + +var pemStart = []byte("\n-----BEGIN ") +var pemEnd = []byte("\n-----END ") +var pemEndOfLine = []byte("-----") +var colon = []byte(":") + +// getLine results the first \r\n or \n delineated line from the given byte +// array. The line does not include trailing whitespace or the trailing new +// line bytes. The remainder of the byte array (also not including the new line +// bytes) is also returned and this will always be smaller than the original +// argument. +func getLine(data []byte) ([]byte, []byte) { + i := bytes.IndexByte(data, '\n') + var j int + if i < 0 { + i = len(data) + j = i + } else { + j = i + 1 + if i > 0 && data[i-1] == '\r' { + i-- + } + } + return bytes.TrimRight(data[0:i], " \t"), data[j:] +} + +// removeSpacesAndTabs returns a copy of its input with all spaces and tabs +// removed, if there were any. Otherwise, the input is returned unchanged. +// +// The base64 decoder already skips newline characters, so we don't need to +// filter them out here. +func removeSpacesAndTabs(data []byte) []byte { + if !bytes.ContainsAny(data, " \t") { + // Fast path; most base64 data within PEM contains newlines, but + // no spaces nor tabs. Skip the extra alloc and work. + return data + } + result := make([]byte, len(data)) + n := 0 + + for _, b := range data { + if b == ' ' || b == '\t' { + continue + } + result[n] = b + n++ + } + + return result[0:n] +} + +// This version of pem.Decode() is a bit less flexible, it will not skip over bad PEM +// It is basically the guts of pem.Decode() inside the outer for loop, with error +// returns rather than continues +func pemDecode(data []byte) (*pem.Block, []byte) { + // pemStart begins with a newline. However, at the very beginning of + // the byte array, we'll accept the start string without it. + rest := data + if bytes.HasPrefix(rest, pemStart[1:]) { + rest = rest[len(pemStart)-1:] + } else if _, after, ok := bytes.Cut(rest, pemStart); ok { + rest = after + } else { + return nil, data + } + + var typeLine []byte + typeLine, rest = getLine(rest) + if !bytes.HasSuffix(typeLine, pemEndOfLine) { + return nil, data + } + typeLine = typeLine[0 : len(typeLine)-len(pemEndOfLine)] + + p := &pem.Block{ + Headers: make(map[string]string), + Type: string(typeLine), + } + + for { + // This loop terminates because getLine's second result is + // always smaller than its argument. + if len(rest) == 0 { + return nil, data + } + line, next := getLine(rest) + + key, val, ok := bytes.Cut(line, colon) + if !ok { + break + } + + key = bytes.TrimSpace(key) + val = bytes.TrimSpace(val) + p.Headers[string(key)] = string(val) + rest = next + } + + var endIndex, endTrailerIndex int + + // If there were no headers, the END line might occur + // immediately, without a leading newline. + if len(p.Headers) == 0 && bytes.HasPrefix(rest, pemEnd[1:]) { + endIndex = 0 + endTrailerIndex = len(pemEnd) - 1 + } else { + endIndex = bytes.Index(rest, pemEnd) + endTrailerIndex = endIndex + len(pemEnd) + } + + if endIndex < 0 { + return nil, data + } + + // After the "-----" of the ending line, there should be the same type + // and then a final five dashes. + endTrailer := rest[endTrailerIndex:] + endTrailerLen := len(typeLine) + len(pemEndOfLine) + if len(endTrailer) < endTrailerLen { + return nil, data + } + + restOfEndLine := endTrailer[endTrailerLen:] + endTrailer = endTrailer[:endTrailerLen] + if !bytes.HasPrefix(endTrailer, typeLine) || + !bytes.HasSuffix(endTrailer, pemEndOfLine) { + return nil, data + } + + // The line must end with only whitespace. + if s, _ := getLine(restOfEndLine); len(s) != 0 { + return nil, data + } + + base64Data := removeSpacesAndTabs(rest[:endIndex]) + p.Bytes = make([]byte, base64.StdEncoding.DecodedLen(len(base64Data))) + n, err := base64.StdEncoding.Decode(p.Bytes, base64Data) + if err != nil { + return nil, data + } + p.Bytes = p.Bytes[:n] + + // the -1 is because we might have only matched pemEnd without the + // leading newline if the PEM block was empty. + _, rest = getLine(rest[endIndex+len(pemEnd)-1:]) + return p, rest +} + +// This version of (*x509.CertPool).AppendCertsFromPEM() will error out if parsing fails +func appendCertsFromPEM(s *x509.CertPool, pemCerts []byte) error { + n := 1 + for len(pemCerts) > 0 { + var block *pem.Block + block, pemCerts = pemDecode(pemCerts) + if block == nil { + return fmt.Errorf("unable to PEM decode cert %d", n) + } + // ignore non-certificates (e.g. keys) + if block.Type != "CERTIFICATE" { + continue + } + if len(block.Headers) != 0 { + // This is a cert, but we're ignoring it, so bump the counter + n++ + continue + } + + cert, err := x509.ParseCertificate(block.Bytes) + if err != nil { + return fmt.Errorf("unable to parse cert %d: %w", n, err) + } + // no return values - panics or always succeeds + s.AddCert(cert) + n++ + } + + return nil +} + +func NewCertPool(caDir string) (*x509.CertPool, error) { + caCertPool, err := x509.SystemCertPool() + if err != nil { + return nil, err + } + if caDir == "" { + return caCertPool, nil + } + + dirEntries, err := os.ReadDir(caDir) + if err != nil { + return nil, err + } + for _, e := range dirEntries { + if e.IsDir() { + continue + } + file := filepath.Join(caDir, e.Name()) + data, err := os.ReadFile(file) + if err != nil { + return nil, fmt.Errorf("error reading cert file %q: %w", file, err) + } + err = appendCertsFromPEM(caCertPool, data) + if err != nil { + return nil, fmt.Errorf("error adding cert file %q: %w", file, err) + } + } + + return caCertPool, nil +} diff --git a/internal/httputil/httputil_test.go b/internal/httputil/certutil_test.go similarity index 58% rename from internal/httputil/httputil_test.go rename to internal/httputil/certutil_test.go index a76c66117..9f72b51a8 100644 --- a/internal/httputil/httputil_test.go +++ b/internal/httputil/certutil_test.go @@ -22,11 +22,15 @@ func TestNewCertPool(t *testing.T) { msg string }{ {"../../testdata/certs/good", ""}, - {"../../testdata/certs/bad", "unable to PEM decode cert 1"}, - {"../../testdata/certs/ugly", ""}, + {"../../testdata/certs/bad", `error adding cert file "../../testdata/certs/bad/Amazon_Root_CA_2.pem": unable to PEM decode cert 1`}, + {"../../testdata/certs/ugly", `error adding cert file "../../testdata/certs/ugly/Amazon_Root_CA.pem": unable to PEM decode cert 2`}, + {"../../testdata/certs/ugly2", `error adding cert file "../../testdata/certs/ugly2/Amazon_Root_CA_1.pem": unable to PEM decode cert 1`}, + {"../../testdata/certs/ugly3", `error adding cert file "../../testdata/certs/ugly3/not_a_cert.pem": unable to PEM decode cert 1`}, + {"../../testdata/certs/empty", `error adding cert file "../../testdata/certs/empty/empty.pem": unable to parse cert 1: x509: malformed certificate`}, } for _, caDir := range caDirs { + t.Logf("Loading certs from %q", caDir.dir) pool, err := httputil.NewCertPool(caDir.dir) if caDir.msg == "" { require.NoError(t, err) diff --git a/internal/httputil/httputil.go b/internal/httputil/httputil.go index adec40ec0..2f15bfaf1 100644 --- a/internal/httputil/httputil.go +++ b/internal/httputil/httputil.go @@ -3,76 +3,10 @@ package httputil import ( "crypto/tls" "crypto/x509" - "encoding/pem" - "fmt" "net/http" - "os" - "path/filepath" "time" ) -// This version of (*x509.CertPool).AppendCertsFromPEM() will error out if parsing fails -func appendCertsFromPEM(s *x509.CertPool, pemCerts []byte) error { - n := 1 - for len(pemCerts) > 0 { - var block *pem.Block - block, pemCerts = pem.Decode(pemCerts) - if block == nil { - return fmt.Errorf("unable to PEM decode cert %d", n) - } - // ignore non-certificates (e.g. keys) - if block.Type != "CERTIFICATE" { - continue - } - if len(block.Headers) != 0 { - // This is a cert, but we're ignoring it, so bump the counter - n++ - continue - } - - cert, err := x509.ParseCertificate(block.Bytes) - if err != nil { - return fmt.Errorf("unable to parse cert %d: %w", n, err) - } - // no return values - panics or always succeeds - s.AddCert(cert) - n++ - } - - return nil -} - -func NewCertPool(caDir string) (*x509.CertPool, error) { - caCertPool, err := x509.SystemCertPool() - if err != nil { - return nil, err - } - if caDir == "" { - return caCertPool, nil - } - - dirEntries, err := os.ReadDir(caDir) - if err != nil { - return nil, err - } - for _, e := range dirEntries { - if e.IsDir() { - continue - } - file := filepath.Join(caDir, e.Name()) - data, err := os.ReadFile(file) - if err != nil { - return nil, fmt.Errorf("error reading cert file %q: %w", file, err) - } - err = appendCertsFromPEM(caCertPool, data) - if err != nil { - return nil, fmt.Errorf("error adding cert file %q: %w", file, err) - } - } - - return caCertPool, nil -} - func BuildHTTPClient(caCertPool *x509.CertPool) (*http.Client, error) { httpClient := &http.Client{Timeout: 10 * time.Second} diff --git a/testdata/certs/empty/empty.pem b/testdata/certs/empty/empty.pem new file mode 100644 index 000000000..c412709aa --- /dev/null +++ b/testdata/certs/empty/empty.pem @@ -0,0 +1,2 @@ +-----BEGIN CERTIFICATE----- +-----END CERTIFICATE----- diff --git a/testdata/certs/good/Amazon_Root_CA_2.pem b/testdata/certs/good/Amazon_Root_CA_2.pem index efe3c9fed..35746cc2f 100644 --- a/testdata/certs/good/Amazon_Root_CA_2.pem +++ b/testdata/certs/good/Amazon_Root_CA_2.pem @@ -1,4 +1,5 @@ -----BEGIN CERTIFICATE----- +Header: value MIIFQTCCAymgAwIBAgITBmyf0pY1hp8KD+WGePhbJruKNzANBgkqhkiG9w0BAQwF ADA5MQswCQYDVQQGEwJVUzEPMA0GA1UEChMGQW1hem9uMRkwFwYDVQQDExBBbWF6 b24gUm9vdCBDQSAyMB4XDTE1MDUyNjAwMDAwMFoXDTQwMDUyNjAwMDAwMFowOTEL diff --git a/testdata/certs/ugly2/Amazon_Root_CA_1.pem b/testdata/certs/ugly2/Amazon_Root_CA_1.pem new file mode 100644 index 000000000..8f0f64972 --- /dev/null +++ b/testdata/certs/ugly2/Amazon_Root_CA_1.pem @@ -0,0 +1,20 @@ +-----BEGIN CERTIFICATE +MIIDQTCCAimgAwIBAgITBmyfz5m/jAo54vB4ikPmljZbyjANBgkqhkiG9w0BAQsF +ADA5MQswCQYDVQQGEwJVUzEPMA0GA1UEChMGQW1hem9uMRkwFwYDVQQDExBBbWF6 +b24gUm9vdCBDQSAxMB4XDTE1MDUyNjAwMDAwMFoXDTM4MDExNzAwMDAwMFowOTEL +MAkGA1UEBhMCVVMxDzANBgNVBAoTBkFtYXpvbjEZMBcGA1UEAxMQQW1hem9uIFJv +b3QgQ0EgMTCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBALJ4gHHKeNXj +ca9HgFB0fW7Y14h29Jlo91ghYPl0hAEvrAIthtOgQ3pOsqTQNroBvo3bSMgHFzZM +9O6II8c+6zf1tRn4SWiw3te5djgdYZ6k/oI2peVKVuRF4fn9tBb6dNqcmzU5L/qw +IFAGbHrQgLKm+a/sRxmPUDgH3KKHOVj4utWp+UhnMJbulHheb4mjUcAwhmahRWa6 +VOujw5H5SNz/0egwLX0tdHA114gk957EWW67c4cX8jJGKLhD+rcdqsq08p8kDi1L +93FcXmn/6pUCyziKrlA4b9v7LWIbxcceVOF34GfID5yHI9Y/QCB/IIDEgEw+OyQm +jgSubJrIqg0CAwEAAaNCMEAwDwYDVR0TAQH/BAUwAwEB/zAOBgNVHQ8BAf8EBAMC +AYYwHQYDVR0OBBYEFIQYzIU07LwMlJQuCFmcx7IQTgoIMA0GCSqGSIb3DQEBCwUA +A4IBAQCY8jdaQZChGsV2USggNiMOruYou6r4lK5IpDB/G/wkjUu0yKGX9rbxenDI +U5PMCCjjmCXPI6T53iHTfIUJrU6adTrCC2qJeHZERxhlbI1Bjjt/msv0tadQ1wUs +N+gDS63pYaACbvXy8MWy7Vu33PqUXHeeE6V/Uq2V8viTO96LXFvKWlJbYK8U90vv +o/ufQJVtMVT8QtPHRh8jrdkPSHCa2XV4cdFyQzR1bldZwgJcJmApzyMZFo6IQ6XU +5MsI+yMRQ+hDKXJioaldXgjUkK642M4UwtBV8ob2xJNDd2ZhwLnoQdeXeGADbkpy +rqXRfboQnoZsG4q5WTP468SQvvG5 +-----END CERTIFICATE----- diff --git a/testdata/certs/ugly3/not_a_cert.pem b/testdata/certs/ugly3/not_a_cert.pem new file mode 100644 index 000000000..980a0d5f1 --- /dev/null +++ b/testdata/certs/ugly3/not_a_cert.pem @@ -0,0 +1 @@ +Hello World!