Bring back EKU verifications

[gmondada]

No description provided.

[github-actions]

AI Security Review

This pull request makes two meaningful changes: it removes a temporary EKU (Extended Key Usage) bypass that was stripping certificate constraints from server certificates before TLS handshake verification, and it rewrites the _dump_cert helper in conf_util.py to shell out to the openssl CLI via subprocess.run rather than using the Python cryptography library directly. The removal of the EKU bypass is a positive security improvement — it closes a previously acknowledged hole in certificate chain validation. However, the replacement implementation introduces a moderate-severity finding: the new code writes a DER-encoded certificate to a predictable temporary file path, spawns a subprocess, and then prints the combined stdout/stderr of that subprocess without checking the return code, creating both a reliability gap and a minor information-disclosure risk. No hardcoded secrets, injection vectors with attacker-controlled data, or payment/health data flows are introduced.

Security & Compliance Review — PR #16: Bring back EKU verifications

1. Executive Summary

This pull request makes two meaningful changes: it removes a temporary EKU (Extended Key Usage) bypass that was stripping certificate constraints from server certificates before TLS handshake verification, and it rewrites the _dump_cert helper in conf_util.py to shell out to the openssl CLI via subprocess.run rather than using the Python cryptography library directly. The removal of the EKU bypass is a positive security improvement — it closes a previously acknowledged hole in certificate chain validation. However, the replacement implementation introduces a moderate-severity finding: the new code writes a DER-encoded certificate to a predictable temporary file path, spawns a subprocess, and then prints the combined stdout/stderr of that subprocess without checking the return code, creating both a reliability gap and a minor information-disclosure risk. No hardcoded secrets, injection vectors with attacker-controlled data, or payment/health data flows are introduced.

---

2. Findings Table

Severity	Standards	File	Line(s)	Title
HIGH	ISO27001-A.8, NIST-SI-10, SOC2-CC6	components/wendy_server/src/wendy_server.c	removed (was ~248)	EKU bypass removed — verify full cert validation is now enforced
MEDIUM	ISO27001-A.8, NIST-SC-8, SOC2-CC6	tools/conf_util.py	77–92	Subprocess return code not checked; stderr silently mixed into output
LOW	ISO27001-A.8, NIST-SI-3, SOC2-CC7	tools/conf_util.py	77–84	Temp file created with delete=False — unlink may be skipped on Windows / edge cases
LOW	NIST-AC-3, ISO27001-A.9	tools/conf_util.py	79	Temp file permissions not explicitly restricted (world-readable on some systems)
INFORMATIONAL	ISO27001-A.8	tools/conf_util.py	86	openssl binary resolved from PATH — no version or path pinning

---

3. Detailed Findings

---

Finding 1 — EKU bypass removed: confirm full certificate validation is now enforced

Severity: HIGH (positive remediation, but requires verification)
Standards: [ISO27001-A.8] [NIST-SI-10] [SOC2-CC6]

Description

The previous code deliberately cleared the MBEDTLS_X509_EXT_EXTENDED_KEY_USAGE bit from the server certificate's ext_types field before the TLS handshake, with an accompanying log warning. This meant the server certificate could carry an EKU that was incompatible with TLS server authentication, and mbedTLS would silently accept it. The bypass is now removed, which is the correct behaviour.

The risk being closed is significant: without EKU enforcement, a certificate issued for code-signing, client authentication, or any other purpose could have been accepted as a TLS server certificate, violating the purpose-binding guarantee that EKU provides. This would have made impersonation attacks easier if an attacker obtained any certificate from the same CA.

Relevant snippet (removed code)

-static void _strip_server_eku(esp_tls_t *tls)
-{
-    // Clear the EKU extension bit so mbedtls_x509_crt_check_extended_key_usage
-    // treats the cert as having no EKU restriction (absent = no restriction).
-    tls->servercert.ext_types &= ~MBEDTLS_X509_EXT_EXTENDED_KEY_USAGE;
-    ESP_LOGW(TAG, "EKU check bypassed (temp workaround)");
-}

Remediation

1. Confirm that the server certificate in production now carries id-kp-serverAuth (OID 1.3.6.1.5.5.7.3.1) in its EKU extension, or is issued from a CA that does not include EKU at all (so mbedTLS treats it as unrestricted).
2. Add an integration test (or CI check) that attempts to connect using a certificate bearing only id-kp-clientAuth and asserts the handshake is rejected.
3. Review any staging or test PKI to ensure no certificates were provisioned under the assumption that EKU would be ignored.

---

Finding 2 — Subprocess return code not checked; stderr mixed into output

Severity: MEDIUM
Standards: [ISO27001-A.8] [NIST-SI-10] [SOC2-CC7]

Description

subprocess.run is called without check=True and without any inspection of r.returncode. If openssl fails (certificate is corrupt, file is unreadable, binary not found), r.stdout will be empty or partial and r.stderr will contain an error message from OpenSSL. Both streams are then unconditionally concatenated and printed. This means:

• Silent failure: a corrupt or attacker-crafted DER blob may produce no diagnostic output, but the tool will appear to succeed (exit code 0 from Python's perspective).
• Information disclosure via stderr: OpenSSL error messages can reveal internal file paths, library versions, or other operational details that could be useful to an attacker observing tool output.
• Misleading output: if openssl is absent, a FileNotFoundError from subprocess.run will propagate uncaught; but if it runs and fails, the error is silently swallowed into the printed output with no indication of failure.

Relevant snippet

        r = subprocess.run(
            ["openssl", "x509", "-text", "-noout", "-fingerprint", "-sha256",
             "-inform", "DER", "-in", tmp],
            capture_output=True, text=True,
        )
    finally:
        os.unlink(tmp)

    print(f"\n  {label}:")
    for line in (r.stdout + r.stderr).splitlines():
        print(f"    {line}")

Remediation

        r = subprocess.run(
            ["openssl", "x509", "-text", "-noout", "-fingerprint", "-sha256",
             "-inform", "DER", "-in", tmp],
            capture_output=True, text=True,
        )
    finally:
        os.unlink(tmp)

    if r.returncode != 0:
        sys.exit(f"openssl failed (rc={r.returncode}): {r.stderr.strip()}")

    print(f"\n  {label}:")
    for line in r.stdout.splitlines():       # print only stdout, not stderr
        print(f"    {line}")

---

Finding 3 — Temp file with delete=False may not be cleaned up

Severity: LOW
Standards: [ISO27001-A.8] [NIST-SI-3]

Description

NamedTemporaryFile(delete=False) is used so the file persists after fh.close() (implicitly at context-manager exit) for the subsequent subprocess.run call. The os.unlink is placed in a finally block, which is correct — however, on Windows, if the process is killed between the write and the unlink (e.g., via Ctrl-C raising KeyboardInterrupt before the try block is entered, or a hard kill), the file will persist in the system temp directory. The file contains a DER-encoded certificate, which is low-sensitivity but still an unnecessary artefact.

The deeper concern is that the finally block is inside the try that wraps only subprocess.run, not the file-write. If an exception occurs during fh.write(der) (e.g., disk full), the finally is never reached.

Relevant snippet

    with tempfile.NamedTemporaryFile(suffix=".der", delete=False) as fh:
        fh.write(der)
        tmp = fh.name
    try:
        r = subprocess.run(...)
    finally:
        os.unlink(tmp)

Remediation

Restructure so the unlink is always attempted after the file is created:

    with tempfile.NamedTemporaryFile(suffix=".der", delete=False) as fh:
        fh.write(der)
        tmp = fh.name
    try:
        r = subprocess.run(...)
    finally:
        try:
            os.unlink(tmp)
        except OSError:
            pass

Or, preferably, use Python's tempfile.TemporaryDirectory / pass the file descriptor directly via stdin to avoid the temp file entirely:

    r = subprocess.run(
        ["openssl", "x509", "-text", "-noout", "-fingerprint", "-sha256",
         "-inform", "DER"],
        input=der,            # pass DER bytes via stdin
        capture_output=True,
    )

This eliminates the temp file and all associated risks.

---

Finding 4 — Temp file permissions not explicitly restricted

Severity: LOW
Standards: [NIST-AC-3] [ISO27001-A.9]

Description

tempfile.NamedTemporaryFile on most Unix systems creates files with mode 0600 by default (readable only by the owner), so this is low risk in normal circumstances. However, the mode parameter is not explicitly set, meaning the actual permissions depend on the Python version, OS, and umask. In a shared CI/CD environment or a container where the effective umask is 0000, the file could be world-readable.

Relevant snippet

    with tempfile.NamedTemporaryFile(suffix=".der", delete=False) as fh:

Remediation

    with tempfile.NamedTemporaryFile(suffix=".der", delete=False, mode="w+b") as fh:
        os.chmod(fh.name, 0o600)

Or, as noted in Finding 3, eliminate the file entirely by piping via stdin.

---

Finding 5 — openssl resolved from PATH without pinning

Severity: INFORMATIONAL
Standards: [ISO27001-A.8]

Description

subprocess.run(["openssl", ...]) resolves the binary from the current PATH. In a developer or CI environment where PATH is attacker-influenced (e.g., a compromised dependency injecting a fake openssl binary earlier in PATH), this could lead to execution of arbitrary code. This is a low-likelihood but non-zero supply-chain risk for a build tool.

Relevant snippet

        r = subprocess.run(
            ["openssl", "x509", ...],

Remediation

• Document the minimum required OpenSSL version in the project README.
• In CI, use an absolute path (e.g., /usr/bin/openssl) or verify the binary hash.
• Consider reverting to the cryptography Python library, which avoids shell execution entirely, is more portable, and does not require a system binary. The previous implementation was strictly safer in this regard.

---

4. Compliance Summary

Framework	Checked	Violations Found
SOC 2 (CC6, CC7, CC8, CC9, A1, C1)	✅	CC6: EKU bypass removal is positive; subprocess return code gap is a minor CC7 (monitoring/output integrity) concern
ISO/IEC 27001:2022 (A.8, A.9, A.12)	✅	A.8: subprocess error handling and temp file hygiene; A.9: temp file permissions
PCI DSS v4.0	✅	Not applicable — no payment data flows touched
GDPR / Privacy	✅	Not applicable — no PII flows present
HIPAA	✅	Not applicable — no health data flows present
NIST SP 800-53 / CSF 2.0 (AC, AU, IA, SC, SI)	✅	SI-10: subprocess error not checked; AC-3: file permissions

Overall: The EKU restoration is a clear security improvement and should be merged after confirming production certificates carry the correct EKU. The conf_util.py refactor introduces minor subprocess-hygiene and temp-file issues that should be addressed before merge; none are critical. The strongest recommendation is to revert _dump_cert to the Python cryptography library approach (or fix the subprocess error handling and eliminate the temp file via stdin piping).