Conversation
AI Security Review
|
| Severity | Standards | File | Line(s) | Title |
|---|---|---|---|---|
| CRITICAL | SOC2-CC6, ISO27001-A.8, ISO27001-A.9, NIST-SC-8, NIST-IA-3 | (PR description / TLS layer, not fully shown) | N/A | TLS peer certificate validation does not verify client org matches device org |
| HIGH | SOC2-CC9, ISO27001-A.8, NIST-SI-2 | components/nanopb/pb.h, components/nanopb/CMakeLists.txt |
All | Vendored nanopb 0.4.9 without integrity verification or dependency pinning |
| HIGH | SOC2-CC6, NIST-IA-3 | (PR description) | N/A | Disabled USB CDC interface cannot be re-enabled after structural change — loss of emergency access path |
| MEDIUM | SOC2-CC7, ISO27001-A.12, NIST-AU-2 | (PR description) | N/A | Severely limited error reporting — most failures collapse to "unknown error", degrading audit trail and anomaly detection |
| MEDIUM | SOC2-CC8, ISO27001-A.8 | (PR description) | N/A | Only partition 0 used of two WASM app partitions — dead code/state with no documented rollback path |
| LOW | SOC2-CC8, ISO27001-A.8 | .vscode/c_cpp_properties.json |
All | IDE configuration file committed to source repository |
| INFORMATIONAL | ISO27001-A.8 | (PR description) | N/A | Inconsistent terminology ("microwendy" vs "wendy-lite") — increases risk of misconfiguration and documentation errors |
3. Detailed Findings
Finding 1 — CRITICAL: TLS Peer Certificate Validation Does Not Verify Client Org Matches Device Org
Standards violated: SOC2-CC6 (Logical Access Controls), ISO27001-A.9 (Access Control), ISO27001-A.8 (Secure Coding), NIST SP 800-53 SC-8, IA-3
Description:
The PR author explicitly states:
"TLS certificate checks are not verifying that the client org matches the device org"
In a device-to-server mutual TLS (mTLS) scenario, verifying that the certificate presented by the peer belongs to the expected organisational unit is essential for device identity assurance. Without this check, the following attacks are feasible:
- A certificate issued to any organisation by the same CA (or a compromised CA) can impersonate a legitimate device.
- A device from Organisation A can successfully authenticate to the backend of Organisation B if they share a CA trust anchor.
- In IoT fleet deployments, a compromised device from one tenant could communicate with another tenant's backend.
This is not a "nice to have" — it is a fundamental broken authentication condition.
Relevant snippet (from PR description):
TLS certificate checks are not verifying that the client org matches the device org
Remediation:
- After the TLS handshake completes, extract the peer certificate's Subject Distinguished Name (DN) fields (specifically
O=,OU=,CN=). - Compare the extracted organisation field against the expected organisation stored locally on the device (e.g., provisioned at manufacture time).
- Tear down the connection and log a security event if the values do not match.
- In ESP-IDF / mbedTLS, this can be implemented using
mbedtls_x509_crt_verify()with a customf_vrfycallback that inspectscrt->subject. - This PR must not be merged until this check is implemented and has test coverage.
Finding 2 — HIGH: Vendored nanopb 0.4.9 Without Integrity Verification or Dependency Pinning
Standards violated: SOC2-CC9 (Third-Party Risk), ISO27001-A.8 (Vulnerability Management), NIST SP 800-53 SI-2
Description:
The diff vendors the nanopb runtime library (version 0.4.9, confirmed by the NANOPB_VERSION define in pb.h) directly into components/nanopb/. There is no cryptographic hash, checksum, or lockfile entry to verify the integrity of the vendored files. The CMakeLists.txt registers only source files (pb_common.c, pb_encode.c, pb_decode.c) with no version pinning mechanism.
nanopb 0.4.9 is the current stable release as of 2024, but:
- Supply-chain tampering of vendored files is undetectable without hashes.
- Future contributors may update the files without realising they have changed the library version.
- Known vulnerabilities in older nanopb versions (e.g., heap/stack overflows in
pb_decode) would not be caught by automated tooling if the version string is not machine-readable in a dependency manifest.
Relevant snippet:
/* Version of the nanopb library. Just in case you want to check it in
* your own program. */
#define NANOPB_VERSION "nanopb-0.4.9"# nanopb runtime — source files are populated by tools/update_protobuf.py
idf_component_register(
SRCS
"pb_common.c"
"pb_encode.c"
"pb_decode.c"
INCLUDE_DIRS
"."
)Remediation:
- Add a
SHA-256orSHA-512hash of the upstream nanopb release archive in aCHECKSUMSorSBOMfile adjacent to the component. - Alternatively, use ESP-IDF's component manager (
idf_component.yml) to declare nanopb as a dependency with a pinned version and verified hash, rather than vendoring manually. - Add a CI step that verifies the vendored file hashes against the recorded values on every build.
- Document in
components/nanopb/README.mdwhich upstream tag/commit this corresponds to and how to update it.
Finding 3 — HIGH: Disabled USB CDC Interface Cannot Be Re-Enabled After Structural Change
Standards violated: SOC2-CC6 (Least Privilege / Access Path Management), NIST SP 800-53 IA-3 (Device Identification and Authentication)
Description:
The PR author states:
"Not sure if the USB CDC interface was functional. It was and still is disabled, but now that the structure has changed, it cannot be enabled anymore."
USB CDC interfaces on embedded devices often serve as a recovery/debug channel. If the firmware structure has been changed in a way that makes it impossible to re-enable USB CDC, this may eliminate an out-of-band access path that is critical for:
- Factory provisioning and testing
- Field recovery without OTA (if OTA is broken)
- Debug access during incident response
Even if this interface was intentionally disabled for production, the inability to re-enable it represents an unplanned loss of capability that should be formally assessed and documented.
Remediation:
- Before merging, determine definitively whether this is intentional (security hardening — USB disabled permanently) or a regression.
- If intentional: document this as a deliberate architectural decision in the codebase and update the threat model accordingly.
- If a regression: fix the structural issue so USB CDC can be optionally compiled in via a Kconfig flag.
- Ensure at least one recovery path exists if OTA fails (e.g., JTAG, UART recovery).
Finding 4 — MEDIUM: Severely Limited Error Reporting Degrades Audit Trail and Anomaly Detection
Standards violated: SOC2-CC7 (System Operations / Anomaly Detection), ISO27001-A.12 (Logging and Monitoring), NIST SP 800-53 AU-2, AU-12
Description:
The PR author notes:
"There is a limited set of errors. Most of the problems end up in a 'unknown error'."
From a security perspective, collapsing distinct failure modes (e.g., TLS certificate rejection, protobuf decode failure, authentication denial, network timeout) into a single "unknown error" response has multiple negative consequences:
- Audit logs become useless for reconstructing what happened during a security incident.
- Anomaly detection systems cannot distinguish authentication failures (which may indicate a brute-force or replay attack) from transient network errors.
- Paradoxically, overly generic errors on the device side (not the client-facing API) also impede debugging of security-relevant failures, increasing the time-to-detect for incidents.
Remediation:
- Define a structured error enum that covers at least: TLS handshake failure, certificate validation failure (distinct from handshake), protobuf decode error, authentication rejection, authorisation rejection, timeout, and internal/unexpected error.
- Ensure that all error paths log the specific error code and relevant context (without leaking sensitive data) to the device's logging subsystem.
- Map errors to monitoring metrics so that spikes in authentication failures can trigger alerts.
Finding 5 — MEDIUM: Only Partition 0 Used of Two WASM App Partitions — Uncontrolled Dead State
Standards violated: SOC2-CC8 (Change Management), ISO27001-A.8 (Secure Coding)
Description:
The PR author notes:
"In the code, we see that there are two partitions for storing WASM apps, but currently only partition 0 is used."
Unused but allocated partitions represent:
- An attack surface if partition selection logic can be influenced by untrusted input (e.g., a malicious server could cause the device to boot from partition 1 if validation is absent).
- A storage area that may contain stale or undefined data, which could be confused for valid WASM code by future code changes.
- A potential privilege escalation vector if partition 1 can be written to without the same integrity checks applied to partition 0.
Remediation:
- Document explicitly whether partition 1 is reserved for A/B OTA-style updates or for a future feature.
- If unused, add bounds-checking to ensure the partition index can only ever be 0 until partition 1 is intentionally activated.
- Add an assertion or compile-time guard (
_Static_assert) that prevents partition 1 from being selected in the current build. - Ensure that any future activation of partition 1 goes through the same signature verification and integrity checking as partition 0.
Finding 6 — LOW: IDE Configuration File Committed to Repository
Standards violated: SOC2-CC8 (Change Management), ISO27001-A.8
Description:
The file .vscode/c_cpp_properties.json is committed to the repository. While this specific file is low-risk (it contains only a reference to the build output directory for IntelliSense), committing IDE configuration files to a shared repository is a poor practice for several reasons:
- It forces all contributors to use VS Code, or causes noise for those who do not.
- More sensitive VS Code files (e.g.,
.vscode/settings.json,.vscode/launch.json) may contain local paths, credentials, or debug configurations and may be committed in future by analogy. - It establishes a pattern that is difficult to reverse once other developers copy the configuration.
Relevant snippet:
{
"configurations": [
{
"name": "ESP-IDF",
"compileCommands": "${workspaceFolder}/build/compile_commands.json"
}
],
"version": 4
}Remediation:
- Add
.vscode/to.gitignore. - If team-wide VS Code configuration is desired, document the recommended settings in
CONTRIBUTING.mdand let individual developers apply them locally.
Finding 7 — INFORMATIONAL: Inconsistent Terminology ("microwendy" vs "wendy-lite")
Standards violated: ISO27001-A.8 (Secure Coding Practices)
Description:
The PR author notes that both "microwendy" and "wendy-lite" are in use and asks whether one should be abandoned. While purely a naming issue, inconsistent terminology in security-critical embedded firmware creates risk of:
- Misconfiguration (e.g., applying a security patch to "microwendy" builds but missing "wendy-lite" builds).
- Incorrect device identification in audit logs and fleet management systems.
- Documentation drift where security guidance applies to one variant but is ignored for the other.
Remediation:
Choose one canonical term before merging, update all code, documentation, and tooling to use it, and add a note to the coding standards document prohibiting the abandoned term.
4. Compliance Summary
| Framework | Checked | Violations Found |
|---|---|---|
| SOC 2 (CC6, CC7, CC8, CC9, A1, C1) | ✅ | Yes — CC6 (broken mTLS org check), CC7 (insufficient error/audit logging), CC8 (dead partition state, IDE file in repo), CC9 (unverified vendored dependency) |
| ISO/IEC 27001:2022 (A.8, A.9, A.12) | ✅ | Yes — A.8 (vendored dep integrity, error handling, dead code), A.9 (broken certificate identity check), A.12 (insufficient error logging) |
| PCI DSS v4.0 | ⬜ Not applicable | No payment/card data flows visible in this diff |
| GDPR / Privacy | ✅ | No personal data flows identified in this diff — no violations |
| HIPAA | ⬜ Not applicable | No health/medical data flows visible in this diff |
| NIST SP 800-53 / CSF 2.0 (AC, AU, IA, SC, SI) | ✅ | Yes — IA-3 (device identity not fully verified), SC-8 (incomplete TLS validation), AU-2/AU-12 (insufficient audit logging), SI-2 (unverified dependency) |
5. Merge Recommendation
This PR should NOT be merged in its current state. The critical TLS certificate organisation validation gap (Finding 1) alone is sufficient to block merge — it means that device identity assurance, which is presumably a core security property of the WendyCom protocol, is fundamentally broken. The PR author is aware of this and it should be treated as a required fix, not a follow-up task. All CRITICAL and HIGH findings should be resolved before merge; MEDIUM findings should have tracked issues created with agreed resolution timelines.
|
Mind that the wendy-lite repo has its own facilities for the old UDP-based reload, https://github.com/wendylabsinc/wendy-lite/blob/main/tools/wendy_serve.py I'm not sure if it's practical or sensible to maintain this in addition to the wendy CLI. If it won't work any more then we'd better remove it at the same time and update any docs. |
New features:
Open points:
This PR goes in pair with wendylabsinc/WendyOS#1059