Skip to content

[C] Software updates: size signature buffers from the request buffer - #398

Merged
Ewerton Scaboro da Silva (ewertons) merged 9 commits into
mainfrom
ewertons/su-signature-buffers
Oct 10, 2026
Merged

Ewerton Scaboro da Silva (ewertons) merged 9 commits into
mainfrom
ewertons/su-signature-buffers

Conversation

@ewertons

@ewertons Ewerton Scaboro da Silva (ewertons) commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

Software updates rejected current service offers:

  • AZ_IOT_SU_REQUEST_BUFFER_SIZE (4096) is smaller than a one-file offer (~4.8 KB).
  • verify_manifest_core() decoded into fixed stack buffers (header/sjwk 2048, sjwk payload 2048, sjwk header 512). A 2369-byte header overflowed and was logged as "not valid base64url".

Changes:

  • AZ_IOT_SU_REQUEST_BUFFER_SIZE default 16384. sizeof(az_iot_su_client) 14216 -> 38792 bytes (x86-64); AZ_IOT_SU_STATE_BLOB_MAX_SIZE 4748 -> 17036.
  • Verification decodes into a caller-supplied scratch. The managed client passes its persistence scratch (free during verification). The SJWK signature, payload and signing key are decoded in place within the decoded outer header, so a signature that fits the request buffer verifies unless its SJWK header alone exceeds a quarter of it. Verifier stack 6544 -> 656 bytes (gcc -O2).
  • az_iot_su_parse_update_request() uses new AZ_IOT_SU_VERIFY_SCRATCH_SIZE (8192) bytes of stack.
  • A part that does not fit is logged as too large, with sizes; malformed input as not valid.
  • Base64 goes through new az_iot_base64_decode() (src/core/base64.c): validates alphabet, padding and zero trailing bits, then decodes with az_core in 64-character blocks, so in place is safe. azure-sdk-for-c left-shifts a negative value (UBSan) on out-of-alphabet input, e.g. a standard-base64 modulus given to the base64url decoder.
  • ESP32 sample: AZ_IOT_SU_REQUEST_BUFFER_SIZE=8192 for the IDF build; nvs partition 24 -> 36 KB so two generations of the largest checkpoint (8844 bytes) fit. OTA slots unchanged; existing devices need a flash erase to adopt the new table.
  • Docs: client-configuration, CHANGELOG, test coverage, ESP32 README.

Tests: large_signature_is_verified, near_limit_nested_signing_key_is_verified, oversized_signature_is_reported_as_too_large (all fail before this change); the first two assert the decoded key, exponent and signature. base64_test.c covers the helper. Partition tables checked with gen_esp32part.py; the ESP32 build was not run. Local: unit tests pass (gcc, ASan/UBSan clean), clang-format 18 and eng/check-*.sh clean.

- AZ_IOT_SU_REQUEST_BUFFER_SIZE defaults to 16384 (was 4096). A one-file
  offer is about 5 KiB, so the former default refused it.
- Manifest verification decodes into a caller-supplied scratch instead of
  fixed stack buffers (header and sjwk 2048, sjwk payload 2048, sjwk header
  512). The managed client passes its persistence scratch, so any signature
  that fits the request buffer decodes; the sjwk and modulus are unescaped in
  place instead of copied. Verifier stack: 6544 -> 624 bytes.
- az_iot_su_parse_update_request() uses AZ_IOT_SU_VERIFY_SCRATCH_SIZE (8192)
  bytes of stack.
- A part that does not fit is logged as too large, with its size, instead of
  as invalid base64url.
- Base64 input is checked against its alphabet before decoding, avoiding a
  negative left shift in azure-sdk-for-c on a standard-base64 modulus.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Near-limit nested signatures can exceed verification scratch, and ESP32 NVS cannot reliably replace maximum-sized checkpoints.

2 open findings
What changed in this PR

Expands software-update offer capacity and replaces fixed signature buffers with shared scratch storage.

Changes:

  • Raises the request-buffer default to 16 KiB.
  • Adds scratch-based JWS/SJWK decoding and improved diagnostics.
  • Adds large-signature tests and updates documentation.
File Description
c/​src/​features/​su/​su_client.c Implements scratch-based signature verification.
c/​inc/​azure/​iot/​az_iot_su.h Increases buffer defaults and documents scratch usage.
c/​tests/​unit/​su_client_test.c Tests large and oversized signatures.
c/​samples/​software_update/​esp32/​main/​app_main.c Updates client-size documentation.
c/​docs/​client-configuration.md Documents new sizing defaults.
c/​docs/​eng/​test-coverage.md Records added coverage.
c/​CHANGELOG.md Describes behavioral and memory changes.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread c/src/features/su/su_client.c
Comment thread c/inc/azure/iot/az_iot_su.h
…eckpoints

- The SJWK signature and payload, and the signing key in it, are decoded in
  place within the decoded outer header, with a decoder that is safe in
  place. Only the SJWK header, the manifest signature or the JWS payload is
  decoded beside the outer header, so a near-limit offer whose size is
  mostly the signing key verifies in the client's scratch.
- ESP32 sample: AZ_IOT_SU_REQUEST_BUFFER_SIZE=8192 for the whole build and a
  36 KB NVS partition, so two generations of the largest checkpoint (8844
  bytes) fit while NVS replaces one with the other.
Copilot AI balanced review requested due to automatic review settings October 9, 2026 18:39

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The new decoder bypasses established az::core primitives, and its modulus decoding is not meaningfully verified.

2 open findings
1 resolved since last review
Previously missed (1)

In code that hasn't changed since last review

Medium severity Assert decoded RSA key bytes and add malformed padding boundary tests

c/​tests/​unit/​su_client_test.c:4667

This test does not prove that the newly supported escaped standard-base64 modulus is decoded correctly: mock_verify_rs256() discards the modulus, exponent, and signature bytes, so any nonempty decoded output passes. Capture and assert the key bytes (or exercise a real verification backend), and add malformed/padding boundary cases for the new decoder; the C convention requires boundary and overflow coverage for new helpers.

🧠 Review effort: Balanced

Comment thread c/src/features/su/su_client.c Outdated
…lper

- az_iot_base64_decode() (src/core/base64.c) validates the alphabet, padding
  and zero trailing bits, then decodes through az_base64_url_decode() /
  az_base64_decode() in 64-character blocks, so it may decode in place.
  Replaces the local decoder in su_client.c.
- Tests: base64_test.c (tails, alphabets, malformed and non-canonical input,
  bounds, block boundaries in place). The SU mock records the key, exponent
  and signature passed to verify_rs256; the large-signature and near-limit
  tests assert the decoded bytes.
Copilot AI balanced review requested due to automatic review settings October 9, 2026 18:53

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Signature-verification memory reuse is security-sensitive, and the modified ESP32 configuration was not build-validated.

2 open findings

🧠 Review effort: Balanced

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The security-sensitive in-place signature-verification rewrite and unbuilt ESP32 configuration warrant final human validation.

2 open findings

🧠 Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings October 10, 2026 00:39

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Security-critical verification and embedded memory-layout changes warrant human review, particularly because the ESP32 build was not run.

1 open finding
1 resolved since last review

🧠 Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings October 10, 2026 01:07

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The security-sensitive verifier rewrite and unexecuted ESP32 integration warrant final human validation.

0 open findings

1 resolved since last review

🧠 Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings October 10, 2026 01:34

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Security-sensitive verification and ESP32 storage-layout changes warrant final human review, particularly because the ESP32 build was not run.

0 open findings

🧠 Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings October 10, 2026 05:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

The implementation, configuration updates, documentation, and boundary-focused tests are consistent with the stated behavior.

0 open findings

🧠 Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings October 10, 2026 07:13

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

The implementation addresses the reported capacity limits with bounded decoding, appropriate tests, and consistent platform configuration.

0 open findings

🧠 Review effort: Balanced

* @file base64.h
* @brief Canonical base64 decoding, in place or not.
*
* Validates the input before handing it to az_core, which left-shifts a

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Why are we adding this? Don't we have base64 functionality in azure-sdk-for-c?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It does use azure-sdk-for-c: the decoding itself is az_base64_url_decode()/az_base64_decode(). The wrapper adds what those lack for untrusted signature input:

  • Validation first. On a character outside the alphabet, az_core left-shifts a negative value (UBSan: az_base64.c:249, hit by the standard-base64 modulus the service sends when given to the url decoder); it also accepts nonzero trailing bits (AB decodes like AA).
  • In-place decoding. Verification decodes the SJWK signature, payload and signing key in place within the decoded header so a near-limit offer fits the scratch; az_core does not document overlapping input/output, so the wrapper decodes in 64-character blocks through a small buffer.
  • One call that picks url or standard alphabet (the modulus may be either).
    It replaced a local decoder in su_client.c after review asked for az_core plus a reusable, tested az_iot_* helper. No change made.

Comment thread c/src/core/base64.c
@@ -0,0 +1,118 @@
// Copyright (c) Microsoft. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license
// information.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Why are we adding this? Don't we have base64 functionality in azure-sdk-for-c?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It does use azure-sdk-for-c: the decoding itself is az_base64_url_decode()/az_base64_decode(). The wrapper adds what those lack for untrusted signature input:

  • Validation first. On a character outside the alphabet, az_core left-shifts a negative value (UBSan: az_base64.c:249, hit by the standard-base64 modulus the service sends when given to the url decoder); it also accepts nonzero trailing bits (AB decodes like AA).
  • In-place decoding. Verification decodes the SJWK signature, payload and signing key in place within the decoded header so a near-limit offer fits the scratch; az_core does not document overlapping input/output, so the wrapper decodes in 64-character blocks through a small buffer.
  • One call that picks url or standard alphabet (the modulus may be either).
    It replaced a local decoder in su_client.c after review asked for az_core plus a reusable, tested az_iot_* helper. No change made.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Approved.

@ewertons
Ewerton Scaboro da Silva (ewertons) merged commit 8656a5e into main Oct 10, 2026
54 checks passed
@ewertons
Ewerton Scaboro da Silva (ewertons) deleted the ewertons/su-signature-buffers branch October 10, 2026 07:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants