fix(update): Replace manual ASN.1 DER parsing with mbedtls_asn1_get_tag() in ECDSA verifier

[Copilot]

The ECDSA verifier in Updater_Signing.cpp hand-rolled ASN.1 DER length parsing to strip zero-padding from the 512-byte signature buffer, only handling short-form and 0x81 long-form lengths. Replaced with mbedtls's own mbedtls_asn1_get_tag(), which handles all DER length forms, validates the tag and bounds internally, and is already present in ESP-IDF.

Changes

• libraries/Update/src/Updater_Signing.cpp
  • Added #include "mbedtls/asn1.h"
  • Replaced manual byte-twiddling in UpdaterECDSAVerifier::verify() with mbedtls_asn1_get_tag():

// Before
if (signatureLen >= 2 && sig[0] == 0x30) {
  if (sig[1] < 0x80) {
    actualSigLen = 2 + sig[1];
  } else if (sig[1] == 0x81 && signatureLen >= 3) {
    actualSigLen = 3 + sig[2];
  }
  if (actualSigLen > signatureLen) { actualSigLen = signatureLen; }
}

// After
unsigned char *p = (unsigned char *)sig;
const unsigned char *end = p + signatureLen;
size_t seq_len = 0;
if (mbedtls_asn1_get_tag(&p, end, &seq_len, MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_SEQUENCE) == 0) {
  size_t parsed = (size_t)(p - sig) + seq_len;
  if (parsed <= signatureLen) {
    actualSigLen = parsed;
  }
}

• On parse failure, actualSigLen falls back to signatureLen (same behaviour as before)
• RSA verifier is unchanged

Original prompt

Problem

In libraries/Update/src/Updater_Signing.cpp, the UpdaterECDSAVerifier::verify() method currently has hand-rolled ASN.1 DER parsing to determine the actual ECDSA signature length from a zero-padded 512-byte buffer. This manual parsing is fragile and only handles specific length encoding forms.

Required Change

Replace the manual ASN.1 parsing in UpdaterECDSAVerifier::verify() with mbedtls's own mbedtls_asn1_get_tag() function, which handles all DER length forms correctly and is already available in ESP-IDF.

Current code to replace (around lines 153-170):

  // ECDSA signatures are DER-encoded (SEQUENCE { INTEGER r, INTEGER s }).
  // The buffer may be zero-padded to a larger size (e.g. 512), so determine
  // the actual DER-encoded signature length from the ASN.1 header to avoid
  // MBEDTLS_ERR_PK_SIG_LEN_MISMATCH.
  const uint8_t *sig = (const uint8_t *)signature;
  size_t actualSigLen = signatureLen;
  if (signatureLen >= 2 && sig[0] == 0x30) {
    if (sig[1] < 0x80) {
      // Short form length: total = tag (1) + length byte (1) + content length
      actualSigLen = 2 + sig[1];
    } else if (sig[1] == 0x81 && signatureLen >= 3) {
      // Long form length (1 extra byte): total = tag (1) + 0x81 (1) + length byte (1) + content length
      actualSigLen = 3 + sig[2];
    }
    if (actualSigLen > signatureLen) {
      actualSigLen = signatureLen;
    }
  }

  int ret = mbedtls_pk_verify((mbedtls_pk_context *)_ctx, md_type, hashBytes, hash->getHashSize(), sig, actualSigLen);

Replace with:

  // ECDSA signatures are DER-encoded (SEQUENCE { INTEGER r, INTEGER s }).
  // The buffer may be zero-padded to a larger size (e.g. 512), so determine
  // the actual DER-encoded signature length from the ASN.1 header to avoid
  // MBEDTLS_ERR_PK_SIG_LEN_MISMATCH.
  const uint8_t *sig = (const uint8_t *)signature;
  size_t actualSigLen = signatureLen;
  unsigned char *p = (unsigned char *)sig;
  const unsigned char *end = p + signatureLen;
  size_t seq_len = 0;
  if (mbedtls_asn1_get_tag(&p, end, &seq_len, MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_SEQUENCE) == 0) {
    // actualSigLen = header bytes (p advanced past them) + content length
    actualSigLen = (size_t)(p - sig) + seq_len;
  }

  int ret = mbedtls_pk_verify((mbedtls_pk_context *)_ctx, md_type, hashBytes, hash->getHashSize(), sig, actualSigLen);

Also add the following include at the top of the file, alongside the other mbedtls includes:

#include "mbedtls/asn1.h"

Important constraints:

• Do NOT modify the RSA verifier — it is already correct.
• Do NOT modify any other file.
• Only modify libraries/Update/src/Updater_Signing.cpp.

The following is the prior conversation context from the user's chat exploration (may be truncated):

User: Review this PR if the changes are actually needed and, if yes, if they are properly implemented

Assistant:

I'll review this PR by examining the changes and understanding the context. Let me start by gathering the PR details and the actual code changes.

Assistant: Now let me look at the original (pre-PR) version of the file to understand the baseline, and also check how the signatureLen is passed from the caller:

Assistant: Now I have a comprehensive understanding of the issue and the PR changes. Let me also check an important detail about the mbedtls mbedtls_pk_verify_ext API to validate the approach:

Assistant: Now let me also check the mbedtls_pk_verify behavior for ECDSA with padded signature lengths:

Assistant: Now I have a thorough understanding of the issue, the original code on master, the signing tool, and the PR's changes. Here's my review:

---

Review of PR #12503

Is the fix needed?

Yes, absolutely. There are two real bugs in the current master code:

1. RSA: PSS vs PKCS#1 v1.5 mismatch — bin_signing.py signs with RSA-PSS padding (padding.PSS with salt_length=PSS.MAX_LENGTH), but the C++ verifier on master calls mbedtls_pk_verify(), which performs PKCS#1 v1.5 verification. These are completely different schemes—PSS-signed binaries will always fail PKCS#1 v1.5 verification. This is confirmed by issue #12427 with error -0x4380 (MBEDTLS_ERR_RSA_VERIFY_FAILED).

2. ECDSA/RSA: Signature length mismatch — The caller in Updater.cpp always passes maxSigSize = 512 as signatureLen:

   if (!_sign->verify(_hash, _signatureBuffer, maxSigSize)) {

   But mbedtls_pk_verify/mbedtls_pk_verify_ext expects the exact signature length, not a padded buffer size. For RSA-2048 the actual signature is 256 bytes, and for ECDSA the DER signature is variable-length (~70-72 bytes for P-256). Passing 512 causes MBEDTLS_ERR_PK_SIG_LEN_MISMATCH for everything except RSA-4096.

Is the fix properly implemented?

The RSA fix is correct. The ECDSA fix has a concern:

✅ RSA Verifier — Correct

</details>



<!-- START COPILOT CODING AGENT SUFFIX -->

*This pull request was created from Copilot chat.*
>

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}