Skip to content

Fix RSA-PSS signature verification mismatch in UpdaterRSAVerifier#12503

Merged
me-no-dev merged 5 commits into
masterfrom
copilot/fix-signing-verification-mismatch
Apr 9, 2026
Merged

Fix RSA-PSS signature verification mismatch in UpdaterRSAVerifier#12503
me-no-dev merged 5 commits into
masterfrom
copilot/fix-signing-verification-mismatch

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 7, 2026

  • Analyze the maxSigSize issue - confirmed it causes verification failure for all key sizes except RSA-4096
  • Fix UpdaterRSAVerifier::verify() to use key_len (from mbedtls_rsa_get_len()) as signature length instead of the padded 512 bytes
  • Fix UpdaterECDSAVerifier::verify() to compute actual DER signature length from the ASN.1 header instead of using the padded 512 bytes
  • Run validation - passed code review and CodeQL security scan
Original prompt

Problem

Issue: #12427

There is a signing/verification algorithm mismatch that causes RSA signature verification to always fail with error -0x4380 (MBEDTLS_ERR_RSA_VERIFY_FAILED) when using binaries signed by bin_signing.py.

Root Cause

tools/bin_signing.py signs using RSA-PSS padding with salt_length=padding.PSS.MAX_LENGTH (line 138-140):

signature = private_key.sign(
    binary_data, padding.PSS(mgf=padding.MGF1(hash_obj), salt_length=padding.PSS.MAX_LENGTH), hash_obj
)

libraries/Update/src/Updater_Signing.cpp verifies using mbedtls_pk_verify() (line 66), which performs PKCS#1 v1.5 verification — a completely different signature scheme:

int ret = mbedtls_pk_verify((mbedtls_pk_context *)_ctx, md_type, hashBytes, hash->getHashSize(), (const unsigned char *)signature, signatureLen);

PSS signatures and PKCS#1 v1.5 signatures are not interchangeable. A PSS-signed binary will never pass PKCS#1 v1.5 verification.

Required Fix

Update UpdaterRSAVerifier::verify() in libraries/Update/src/Updater_Signing.cpp to use mbedtls_pk_verify_ext() with MBEDTLS_PK_RSASSA_PSS and mbedtls_pk_rsassa_pss_options to properly verify RSA-PSS signatures.

The key change in UpdaterRSAVerifier::verify() method (around line 66):

  1. Get the RSA context from the PK context using mbedtls_pk_rsa().
  2. Get the RSA key length using mbedtls_rsa_get_len().
  3. Compute expected_salt_len = key_len - hash_size - 2 (this matches Python's PSS.MAX_LENGTH behavior).
  4. Set up mbedtls_pk_rsassa_pss_options with the correct md_type and expected_salt_len.
  5. Call mbedtls_pk_verify_ext(MBEDTLS_PK_RSASSA_PSS, &pss_opts, ctx, md_type, hashBytes, hash_size, signature, signatureLen) instead of mbedtls_pk_verify().

Here is what the updated verify method for RSA should look like:

bool UpdaterRSAVerifier::verify(SHA2Builder *hash, const void *signature, size_t signatureLen) {
  if (!_valid || !hash) {
    log_e("Invalid RSA verifier or hash");
    return false;
  }

  mbedtls_md_type_t md_type;
  switch (_hashType) {
    case HASH_SHA256: md_type = MBEDTLS_MD_SHA256; break;
    case HASH_SHA384: md_type = MBEDTLS_MD_SHA384; break;
    case HASH_SHA512: md_type = MBEDTLS_MD_SHA512; break;
    default:          log_e("Invalid hash type"); return false;
  }

  // Get hash bytes from the builder
  uint8_t hashBytes[64];  // Max hash size (SHA-512)
  hash->getBytes(hashBytes);
  size_t hash_size = hash->getHashSize();

  // Use RSA-PSS verification to match bin_signing.py which signs with PSS padding
  // and salt_length=PSS.MAX_LENGTH (which equals key_len - hash_size - 2)
  mbedtls_rsa_context *rsa_ctx = mbedtls_pk_rsa(*(mbedtls_pk_context *)_ctx);
  if (!rsa_ctx) {
    log_e("Failed to get RSA context");
    return false;
  }

  size_t key_len = mbedtls_rsa_get_len(rsa_ctx);
  // PSS.MAX_LENGTH salt = key_len - hash_size - 2
  int expected_salt_len = (int)(key_len - hash_size - 2);
  if (expected_salt_len < 0) {
    log_e("RSA key too small for hash algorithm");
    return false;
  }

  mbedtls_pk_rsassa_pss_options pss_opts;
  pss_opts.mgf1_hash_id = md_type;
  pss_opts.expected_salt_len = expected_salt_len;

  int ret = mbedtls_pk_verify_ext(
    MBEDTLS_PK_RSASSA_PSS, &pss_opts,
    (mbedtls_pk_context *)_ctx, md_type,
    hashBytes, hash_size,
    (const unsigned char *)signature, signatureLen
  );

  if (ret == 0) {
    log_i("RSA signature verified successfully");
    return true;
  } else {
    log_e("RSA signature verification failed: -0x%04X", -ret);
    return false;
  }
}

Also update the verify_signature function in tools/bin_signing.py to use the same PSS parameters for consistency — currently it already uses PSS.MAX_LENGTH on line 210 for verification, so it should already be consistent.

Important: The ECDSA verification path (UpdaterECDSAVerifier::verify()) uses mbedtls_pk_verify() which is correct for ECDSA, so do NOT change the ECDSA verifier.

Files to modify:

  • libraries/Update/src/Updater_Signing.cpp — Update UpdaterRSAVerifier::verify() to use PSS verification as described above.

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

User: Analyze this issue and fix it if it is valid

Assistant:

I'll analyze this issue thoroughly. The problem is about RSA signature verification failing when using signatures created by bin_signing.py because of a PSS padding salt length mismatch between the Python signing script and the mbedtls verification in Updater_Signing.cpp.

Let me examine the relevant code files first.

Assistant: Now let me look at the header file to understand the class structure and any existing includes:

Assistant: Now I have a thorough understanding of the issue. Let me analyze it:

Root Cause Analysis:

The issue is a **mismatch between the signing and verification ...

This pull request was created from Copilot chat.

@CLAassistant
Copy link
Copy Markdown

CLA assistant check
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.

@CLAassistant
Copy link
Copy Markdown

CLA assistant check
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.

Copilot AI changed the title [WIP] Fix RSA signing verification algorithm mismatch Fix RSA-PSS signature verification mismatch in UpdaterRSAVerifier Apr 7, 2026
Copilot AI requested a review from lucasssvaz April 7, 2026 17:11
@lucasssvaz lucasssvaz requested a review from Copilot April 7, 2026 20:40
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 7, 2026

Warnings
⚠️

Some issues found for the commit messages in this PR:

  • the commit message "Fix RSA-PSS signature verification mismatch in UpdaterRSAVerifier::verify()":
    • probably contains Jira ticket reference (4155-92). Please remove Jira tickets from commit messages.
    • body's lines must not be longer than 100 characters
    • summary looks empty
    • type/action looks empty
  • the commit message "Fix signature length mismatch: use actual sig length instead of padded maxSigSize":
    • probably contains Jira ticket reference (6-6120, 4141-8682). Please remove Jira tickets from commit messages.
    • body's lines must not be longer than 100 characters
    • summary looks empty
    • type/action looks empty
  • the commit message "Fix unsigned subtraction underflow in RSA-PSS salt length calculation":
    • probably contains Jira ticket reference (4155-92). Please remove Jira tickets from commit messages.
    • body's lines must not be longer than 100 characters
    • summary looks empty
    • type/action looks empty
  • the commit message "Initial plan":
    • summary looks empty
    • type/action looks empty
  • the commit message "fix(update): Replace manual ASN.1 DER parsing with mbedtls_asn1_get_tag() in ECDSA verifier (#12504)":
    • probably contains Jira ticket reference (80838-3, 0-45). Please remove Jira tickets from commit messages.
    • body's lines must not be longer than 100 characters
    • summary appears to be too long

Please fix these commit messages - here are some basic tips:

  • follow Conventional Commits style
  • correct format of commit message should be: <type/action>(<scope/component>): <summary>, for example fix(esp32): Fixed startup timeout issue
  • allowed types are: change,ci,docs,feat,fix,refactor,remove,revert,test
  • sufficiently descriptive message summary should be between 10 to 72 characters and start with upper case letter
  • avoid Jira references in commit messages (unavailable/irrelevant for our customers)

TIP: Install pre-commit hooks and run this check when committing (uses the Conventional Precommit Linter).

👋 Hello Copilot, we appreciate your contribution to this project!


📘 Please review the project's Contributions Guide for key guidelines on code, documentation, testing, and more.

🖊️ Please also make sure you have read and signed the Contributor License Agreement for this project.

Click to see more instructions ...


This automated output is generated by the PR linter DangerJS, which checks if your Pull Request meets the project's requirements and helps you fix potential issues.

DangerJS is triggered with each push event to a Pull Request and modify the contents of this comment.

Please consider the following:
- Danger mainly focuses on the PR structure and formatting and can't understand the meaning behind your code or changes.
- Danger is not a substitute for human code reviews; it's still important to request a code review from your colleagues.
- Resolve all warnings (⚠️ ) before requesting a review from human reviewers - they will appreciate it.
- To manually retry these Danger checks, please navigate to the Actions tab and re-run last Danger workflow.

Review and merge process you can expect ...


We do welcome contributions in the form of bug reports, feature requests and pull requests.

1. An internal issue has been created for the PR, we assign it to the relevant engineer.
2. They review the PR and either approve it or ask you for changes or clarifications.
3. Once the GitHub PR is approved we do the final review, collect approvals from core owners and make sure all the automated tests are passing.
- At this point we may do some adjustments to the proposed change, or extend it by adding tests or documentation.
4. If the change is approved and passes the tests it is merged into the default branch.

Generated by 🚫 dangerJS against 41ed233

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes OTA RSA signature verification in the Update library by aligning the verifier with the RSA-PSS scheme used by bin_signing.py, resolving consistent verification failures caused by a PKCS#1 v1.5 vs PSS mismatch.

Changes:

  • Switch RSA verification from mbedtls_pk_verify() (PKCS#1 v1.5) to mbedtls_pk_verify_ext() configured for MBEDTLS_PK_RSASSA_PSS.
  • Derive and apply the PSS expected salt length (key_len - hash_size - 2) to match Python’s PSS.MAX_LENGTH.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…ag() in ECDSA verifier (#12504)

* Initial plan

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

Agent-Logs-Url: https://github.com/espressif/arduino-esp32/sessions/85a80838-3bb0-45b2-a613-b6583ce036a6

Co-authored-by: lucasssvaz <32426024+lucasssvaz@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: lucasssvaz <32426024+lucasssvaz@users.noreply.github.com>
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 8, 2026

Memory usage test (comparing PR against master branch)

The table below shows the summary of memory usage change (decrease - increase) in bytes and percentage for each target.

MemoryFLASH [bytes]FLASH [%]RAM [bytes]RAM [%]
TargetDECINCDECINCDECINCDECINC
ESP320⚠️ +3080.00⚠️ +0.03000.000.00
ESP32C30⚠️ +3380.00⚠️ +0.03000.000.00
ESP32C50⚠️ +3380.00⚠️ +0.03000.000.00
ESP32C60⚠️ +3380.00⚠️ +0.03000.000.00
ESP32H2000.000.00000.000.00
ESP32P40⚠️ +3560.00⚠️ +0.04000.000.00
ESP32S20⚠️ +2880.00⚠️ +0.03000.000.00
ESP32S30⚠️ +2800.00⚠️ +0.03000.000.00
Click to expand the detailed deltas report [usage change in BYTES]
TargetESP32ESP32C3ESP32C5ESP32C6ESP32H2ESP32P4ESP32S2ESP32S3
ExampleFLASHRAMFLASHRAMFLASHRAMFLASHRAMFLASHRAMFLASHRAMFLASHRAMFLASHRAM
libraries/ArduinoOTA/examples/BasicOTA00000000--000000
libraries/ArduinoOTA/examples/SignedOTA⚠️ +3080⚠️ +3380⚠️ +3380⚠️ +3380--⚠️ +3560⚠️ +2880⚠️ +2800
libraries/HTTPUpdate/examples/httpUpdate00000000--000000
libraries/HTTPUpdate/examples/httpUpdateSPIFFS00000000--000000
libraries/HTTPUpdate/examples/httpUpdateSecure00000000--000000
libraries/HTTPUpdateServer/examples/WebUpdater00000000--000000
libraries/Update/examples/AWS_S3_OTA_Update00000000--000000
libraries/Update/examples/HTTP_Client_AES_OTA_Update00000000--000000
libraries/Update/examples/HTTP_Server_AES_OTA_Update00000000--000000
libraries/Update/examples/OTAWebUpdater00000000--000000
libraries/Update/examples/SD_Update0000000000000000
libraries/Update/examples/Signed_OTA_Update⚠️ +560⚠️ +560⚠️ +560⚠️ +560--⚠️ +600⚠️ +480⚠️ +520
libraries/WebServer/examples/HttpAdvancedAuth00000000--000000
libraries/WebServer/examples/HttpAuthCallback00000000--000000
libraries/WebServer/examples/HttpAuthCallbackInline00000000--000000
libraries/WebServer/examples/HttpBasicAuth00000000--000000
libraries/WebServer/examples/HttpBasicAuthSHA100000000--000000
libraries/WebServer/examples/HttpBasicAuthSHA1orBearerToken00000000--000000
libraries/WebServer/examples/WebUpdate00000000--000000

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 8, 2026

Test Results

101 files  101 suites   36m 9s ⏱️
 95 tests  92 ✅ 0 💤  3 ❌
823 runs  812 ✅ 0 💤 11 ❌

For more details on these failures, see this check.

Results for commit 41ed233.

♻️ This comment has been updated with latest results.

@lucasssvaz lucasssvaz marked this pull request as ready for review April 8, 2026 16:00
@lucasssvaz lucasssvaz requested a review from me-no-dev as a code owner April 8, 2026 16:00
@lucasssvaz
Copy link
Copy Markdown
Member

lucasssvaz commented Apr 8, 2026

Tested locally. Everything works fine. CI failure is unrelated.

@me-no-dev me-no-dev added the Status: Pending Merge Pull Request is ready to be merged label Apr 8, 2026
@me-no-dev me-no-dev merged commit 938c295 into master Apr 9, 2026
86 of 92 checks passed
@me-no-dev me-no-dev deleted the copilot/fix-signing-verification-mismatch branch April 9, 2026 08:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Status: Pending Merge Pull Request is ready to be merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants