From 1ecbe4c45a4e014784dfa06e469dee792b634622 Mon Sep 17 00:00:00 2001 From: Matt Brown Date: Thu, 17 Sep 2026 19:18:39 -0400 Subject: [PATCH] fix: require a body + matching END for PEM private-key findings The secrets pass reported a private key from a bare PEM begin-header alone. Crypto libraries (mbedtls, OpenSSL, wolfSSL) embed the PEM label strings back to back in .rodata with no key body between them, so every such binary produced false positives, and a PUBLIC KEY label sitting next to a private-key end marker was even classified as a private key. Gate match_pem_private on three properties a real key has that a bare label does not: the begin header's own type must be a private-key type (never PUBLIC KEY / CERTIFICATE), at least 64 base64 body chars must follow, and the block must close with the matching end marker of the same type. Bounded 64 KiB window, all reads through Reader; the finding still reports the header marker, not key bytes. Add test_pem_label_false_positives (bare header; begin-then-end with no body; PUBLIC KEY with a body; begin/end type mismatch; and the exact NUL-separated mbedtls .rodata label layout). Update the positive PEM tests and the id_rsa integration fixture to carry a realistic body. Fixes #26 --- src/rules_builtin.cpp | 85 ++++++++++++++++++++++++++++++--------- tests/test_secrets.py | 6 ++- tests/unit/unit_tests.cpp | 50 ++++++++++++++++++++++- 3 files changed, 120 insertions(+), 21 deletions(-) diff --git a/src/rules_builtin.cpp b/src/rules_builtin.cpp index 31eb358..9afa746 100644 --- a/src/rules_builtin.cpp +++ b/src/rules_builtin.cpp @@ -74,29 +74,78 @@ std::optional match_jwt(const Reader& r, size_t off) { return m; } -// PEM private-key body: anchor "-----BEGIN "; require "PRIVATE KEY-----" within -// a short run of key-type words. Structural, not entropy-gated. +// PEM base64 body alphabet (standard, plus '=' padding). Newlines / whitespace +// are handled by the body scanner, not this predicate. +inline bool c_base64_pem(uint8_t c) { return c_alnum(c) || c == '+' || c == '/' || c == '='; } + +// PEM private-key block: anchor "-----BEGIN "; parse the BEGIN header, then require +// three things a real key has and a library's embedded PEM label string does not +// (GH #26): (1) the header's own key type is a *private* key ("... PRIVATE KEY"), +// so a "PUBLIC KEY" / "CERTIFICATE" label is never reported as a private key; +// (2) a substantial base64 body follows the header; and (3) the block closes with +// the matching "-----END -----". Crypto libraries (mbedtls, OpenSSL, +// wolfSSL) embed the bare BEGIN/END label strings back to back in .rodata with no +// body between them; those must not be reported. The returned match length is the +// header line only (a marker, not secret bytes) -- the body scan is just a gate. std::optional match_pem_private(const Reader& r, size_t off) { constexpr size_t PREFIX = 11; // "-----BEGIN " - const char* tail = "PRIVATE KEY-----"; - const size_t tlen = 16; - for (size_t k = 0; k <= 40; ++k) { - auto b = r.bytes(off + PREFIX + k, tlen); + const uint8_t* DASH5 = reinterpret_cast("-----"); + + // 1. Read the key-type token, from after "-----BEGIN " up to the closing + // "-----" of the header. Type words are uppercase letters and single spaces + // (e.g. "RSA PRIVATE KEY", "ENCRYPTED PRIVATE KEY", "OPENSSH PRIVATE KEY"). + constexpr size_t MAX_TYPE = 40; + size_t typelen = 0; + for (; typelen <= MAX_TYPE; ++typelen) { + auto b = r.bytes(off + PREFIX + typelen, 5); if (!b) return std::nullopt; - if (std::equal(b->begin(), b->end(), reinterpret_cast(tail))) { - size_t hdr = PREFIX + k + tlen; - Match m; - m.len = hdr; - m.confidence = Confidence::Structural; - std::string header = token_str(r, off, hdr); - std::string kt = pem_key_type(header); - m.label = header; // the header line is a marker, not secret bytes - m.evidence = "PEM private-key header"; - m.description = "Private key" + (kt.empty() ? std::string() : " (" + kt + ")"); - return m; + if (std::equal(b->begin(), b->end(), DASH5)) break; + uint8_t c = (*b)[0]; + if (!((c >= 'A' && c <= 'Z') || c == ' ')) return std::nullopt; + } + if (typelen == 0 || typelen > MAX_TYPE) return std::nullopt; + std::string type = token_str(r, off + PREFIX, typelen); + + // 2. The header must itself be a PRIVATE KEY label (rejects PUBLIC KEY, + // CERTIFICATE, EC PARAMETERS, DH PARAMETERS, ...). + const std::string PK = "PRIVATE KEY"; + if (type.size() < PK.size() || type.compare(type.size() - PK.size(), PK.size(), PK) != 0) + return std::nullopt; + + const size_t hdr_end = off + PREFIX + typelen + 5; // past "-----BEGIN -----" + + // 3. Require a real base64 body, then the matching "-----END -----". + // The first 5-dash run we meet must be that END and must be preceded by at + // least MIN_BODY base64 chars; otherwise (BEGIN immediately followed by an + // END/BEGIN, wrong END type, or no body) this is a bare label -> reject. + const std::string end_marker = "-----END " + type + "-----"; + const auto* end_bytes = reinterpret_cast(end_marker.data()); + constexpr size_t WINDOW = 1u << 16; // 64 KiB: ample for any PEM key body + constexpr size_t MIN_BODY = 64; // smallest real key body is far larger than this + size_t body_b64 = 0; + bool closed = false; + for (size_t p = hdr_end; p < hdr_end + WINDOW; ++p) { + auto b = r.bytes(p, 5); + if (!b) return std::nullopt; + if (std::equal(b->begin(), b->end(), DASH5)) { + if (body_b64 >= MIN_BODY && + r.matches_at(p, std::span(end_bytes, end_marker.size()))) + closed = true; + break; // first dash-run decides it: matching END with body, or reject } + if (c_base64_pem((*b)[0])) ++body_b64; } - return std::nullopt; + if (!closed) return std::nullopt; + + Match m; + m.len = PREFIX + typelen + 5; // header line only; a marker, not secret bytes + m.confidence = Confidence::Structural; + std::string header = token_str(r, off, m.len); + std::string kt = pem_key_type(header); + m.label = header; + m.evidence = "PEM private-key block (base64 body + matching END)"; + m.description = "Private key" + (kt.empty() ? std::string() : " (" + kt + ")"); + return m; } // Generic assignment: KEY . Noisy -> hard entropy gate diff --git a/tests/test_secrets.py b/tests/test_secrets.py index 861fdfb..fbeecb7 100755 --- a/tests/test_secrets.py +++ b/tests/test_secrets.py @@ -37,9 +37,13 @@ def build_tree(d): # canonical AWS example key: must be filtered as a false positive. f.write('const demo = "AKIAIOSFODNN7EXAMPLE";\n') + # A realistic on-disk private key: a real body (several base64 lines) and the + # matching END. The detector requires a body, so a bare header is not enough (#26). with open(os.path.join(d, "etc/id_rsa"), "w") as f: f.write("-----BEGIN OPENSSH PRIVATE KEY-----\n") - f.write("b3BlbnNzaC1rZXktdjEAAAAABG5vbmU\n") + body = "b3BlbnNzaC1rZXktdjEAAAAABG5vbmUAAAAEbm9uZQAAAAAAAAABAAAAMwAAAAtz" + for _ in range(6): + f.write(body + "\n") f.write("-----END OPENSSH PRIVATE KEY-----\n") # A base64 config blob hiding an AWS key (decode-then-scan path). diff --git a/tests/unit/unit_tests.cpp b/tests/unit/unit_tests.cpp index 3b1243c..13d3424 100644 --- a/tests/unit/unit_tests.cpp +++ b/tests/unit/unit_tests.cpp @@ -78,6 +78,15 @@ static bool has_type(const std::vector& fs, const std::string& t) { return find_type(fs, t) != nullptr; } +// A well-formed PEM private-key block of `type` with a substantial base64 body +// (three 64-char lines) and the matching END, for the detector's body gate (#26). +static std::string pem_block(const std::string& type) { + const std::string line = + "MIIEpAIBAAKCAQEA0Z3VS5JJcds3xfnygWyF0qFrTfCK3myL7GTdrz6iApW5R2t8W"; + return "-----BEGIN " + type + "-----\n" + line + "\n" + line + "\n" + line + "\n" + + "-----END " + type + "-----\n"; +} + static std::string b64(const std::string& in) { static const char* T = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/"; std::string o; @@ -181,7 +190,7 @@ static void test_detectors() { CHECK(has_type(scan("glpat-" "AbCdEf0123456789xYzQ end"), "gitlab-pat")); CHECK(has_type(scan("k=sk_live_" "0123456789abcdefABCDEF01 end"), "stripe-key")); CHECK(has_type(scan("key AIzaabcdefghijklmnopqrstuvwxyz012345678 x"), "google-api-key")); - CHECK(has_type(scan("-----BEGIN RSA PRIVATE KEY-----\nMIIE...\n"), "private-key")); + CHECK(has_type(scan(pem_block("RSA PRIVATE KEY")), "private-key")); CHECK(has_type(scan("password = s3cr3tP@ssw0rd_9xQ7zLmN end"), "generic-secret")); } @@ -255,10 +264,46 @@ static void test_validators_pem() { CHECK(ft::pem_key_type("-----BEGIN RSA PRIVATE KEY-----") == "RSA"); CHECK(ft::pem_key_type("-----BEGIN OPENSSH PRIVATE KEY-----") == "OPENSSH"); CHECK(ft::pem_key_type("-----BEGIN PRIVATE KEY-----") == ""); - auto fs = scan("-----BEGIN EC PRIVATE KEY-----\nMHc...\n"); + // A real block (header + body + matching END) is Structural with the type noted. + auto fs = scan(pem_block("EC PRIVATE KEY")); const ft::Finding* f = find_type(fs, "private-key"); CHECK(f && f->confidence == static_cast(ft::Confidence::Structural)); CHECK(f && f->description.find("EC") != std::string::npos); + // PKCS#8 unadorned and encrypted headers are private-key labels too. + CHECK(has_type(scan(pem_block("PRIVATE KEY")), "private-key")); + CHECK(has_type(scan(pem_block("ENCRYPTED PRIVATE KEY")), "private-key")); +} + +// GH #26: crypto libraries (mbedtls/OpenSSL/wolfSSL) embed the bare PEM label +// strings in .rodata with no key body. None of these must yield a private-key. +static void test_pem_label_false_positives() { + // PEM begin-markers below are written as two adjacent string literals + // ("-----BEGIN " "-----"), which the compiler concatenates to the exact + // bytes at runtime while the committed source holds no contiguous PEM header + // (the repo convention for synthetic key fixtures; keeps secret scanners quiet). + // Bare header, no body, no END. + CHECK(!has_type(scan("-----BEGIN " "RSA PRIVATE KEY-----\n"), "private-key")); + // BEGIN immediately followed by its END, zero body. + CHECK(!has_type(scan("-----BEGIN " "RSA PRIVATE KEY-----\n-----END RSA PRIVATE KEY-----\n"), + "private-key")); + // A PUBLIC KEY label (with a real body) is never a private key. + CHECK(!has_type(scan(pem_block("PUBLIC KEY")), "private-key")); + // Type mismatch: a real body but the END type differs from the BEGIN type. + CHECK(!has_type(scan("-----BEGIN " "RSA PRIVATE KEY-----\n" + + std::string(80, 'A') + "\n-----END EC PRIVATE KEY-----\n"), + "private-key")); + // The exact mbedtls .rodata layout: NUL-separated labels packed back to back, + // including a BEGIN PUBLIC KEY adjacent to an END RSA PRIVATE KEY (target #2). + std::string labels; + for (const char* s : {"-----BEGIN " "RSA PRIVATE KEY-----", "-----END RSA PRIVATE KEY-----", + "-----BEGIN " "EC PRIVATE KEY-----", "-----END EC PRIVATE KEY-----", + "-----BEGIN " "PUBLIC KEY-----", "-----END PUBLIC KEY-----", + "-----BEGIN " "PUBLIC KEY-----", "-----END RSA PRIVATE KEY-----", + "-----BEGIN " "ENCRYPTED PRIVATE KEY-----"}) { + labels += s; + labels.push_back('\0'); + } + CHECK(scan(labels).empty()); } // ---------------------------------------------------------------- glob + path rules @@ -1756,6 +1801,7 @@ int main() { test_validators_github_crc(); test_validators_jwt(); test_validators_pem(); + test_pem_label_false_positives(); test_derkey(); test_glob(); test_path_rules();