Skip to content

support for large and file-backed custom TLVs#828

Draft
bigbrett wants to merge 5 commits into
wolfSSL:masterfrom
bigbrett:remove-custom-tlv-limit
Draft

support for large and file-backed custom TLVs#828
bigbrett wants to merge 5 commits into
wolfSSL:masterfrom
bigbrett:remove-custom-tlv-limit

Conversation

@bigbrett

@bigbrett bigbrett commented Jul 21, 2026

Copy link
Copy Markdown
Contributor
  • Removes the arbitrary small size restriction on custom TLV size
  • Adds a mechanism for the sign tool to build custom TLVs with a data payload from a file
  • Adds test coverage for new functionality

@bigbrett bigbrett self-assigned this Jul 21, 2026

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Fenrir Automated Review — PR #828

Scan targets checked: wolfboot-bugs, wolfboot-src

Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Findings are non-blocking.

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.

Pull request overview

This pull request extends the tools/keytools/sign utility to support large custom TLVs (up to the 16-bit TLV wire-format limit) and introduces a file-backed mechanism for providing large TLV payloads, along with unit/CI coverage and documentation updates.

Changes:

  • Update sign.c to remove the previous small-size cap for --custom-tlv-buffer / --custom-tlv-string, add --custom-tlv-file, and ensure header sizing is precomputed consistently for both full and delta signing.
  • Add a new Python unit test that validates >255-byte custom TLVs, header growth behavior, boundary conditions, and delta inverse-patch consistency.
  • Update docs and CI to cover and exercise the new functionality.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
tools/unit-tests/unit-sign-custom-tlv-large.py New regression/unit test covering large and file-backed custom TLVs, header growth, and delta inverse-patch invariants
tools/unit-tests/Makefile Adds the new unit test to the unit-test runner target
tools/keytools/sign.c Implements large custom TLV handling, adds --custom-tlv-file, and adjusts header sizing logic for full/delta signing paths
docs/Signing.md Documents new max TLV sizes, the new --custom-tlv-file option, and header auto-growth behavior
.github/workflows/test-custom-tlv-simulator.yml Adds a simulator workflow job that signs and validates retrieval of larger custom TLVs

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

Comment thread tools/keytools/sign.c Outdated
Comment thread tools/keytools/sign.c Outdated
Comment thread tools/keytools/sign.c Outdated
Comment thread tools/keytools/sign.c
Comment thread tools/keytools/sign.c
Comment thread tools/keytools/sign.c

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Fenrir Automated Review — PR #828

Scan targets checked: wolfboot-bugs, wolfboot-src

Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Findings are non-blocking.

Comment thread tools/keytools/sign.c Outdated
printf("Warning: certificate chain file size is invalid (%jd)\n",
(intmax_t)chain_file_sz);
if ((CMD.cert_chain_file != NULL) || (CMD.custom_tlvs > 0)) {
uint32_t cert_chain_sz = 0;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 [Low] Inner cert_chain_sz shadows outer variable of same name in make_header_ex · Variable shadowing

The PR introduces uint32_t cert_chain_sz = 0; at line 1381 inside the expanded if block, shadowing the outer cert_chain_sz at line 1373. Assignments to the inner variable (line 1399) never update the outer one, which remains 0 until it is re-read from disk at line 1586.

Fix: Rename the inner variable to hdr_cert_chain_sz (or similar) to avoid shadowing the outer cert_chain_sz used later for cert chain reading.

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.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

tools/keytools/sign.c:3256

  • When the hex string is empty, len becomes 0 and malloc(0) may legally return NULL; the current NULL check would then treat an empty TLV as an allocation failure on some platforms. Allocate at least 1 byte (while keeping len=0), or explicitly reject empty values, to avoid platform-dependent behavior.
            len = (uint16_t)(slen / 2);
            CMD.custom_tlv[p].tag = tag;
            CMD.custom_tlv[p].len = len;
            CMD.custom_tlv[p].buffer = malloc(len);
            if (CMD.custom_tlv[p].buffer == NULL) {
                fprintf(stderr, "Error malloc for custom tlv buffer %d\n", len);

tools/keytools/sign.c:3299

  • An empty --custom-tlv-string value results in len=0 and malloc(0), which may return NULL per the C standard; the current check would then fail with a misleading malloc error on some platforms. Allocate at least 1 byte (keeping len=0) or explicitly reject empty strings.
            len = (uint16_t)slen;
            CMD.custom_tlv[p].tag = tag;
            CMD.custom_tlv[p].len = len;
            CMD.custom_tlv[p].buffer = malloc(len);
            if (CMD.custom_tlv[p].buffer == NULL) {
                fprintf(stderr, "Error malloc for custom tlv buffer %d\n", len);
                exit(16);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants