Skip to content

OIDs represented as word32 instead of word16#10710

Open
aidankeefe2022 wants to merge 1 commit into
wolfSSL:masterfrom
aidankeefe2022:Asn1-word16-word32-migration
Open

OIDs represented as word32 instead of word16#10710
aidankeefe2022 wants to merge 1 commit into
wolfSSL:masterfrom
aidankeefe2022:Asn1-word16-word32-migration

Conversation

@aidankeefe2022

@aidankeefe2022 aidankeefe2022 commented Jun 16, 2026

Copy link
Copy Markdown
Member

Description

This fixes two issues in how OIDs are represented in wolfSSL.
Breaking change. Word16 cannot represent many real OID arcs -- e.g. 1.2.840.113549.1.1.1 (RSA encryption). This was widened to word 32 in three public symbols:

  • Two callback types: wc_UnknownExtCallback and wc_UnknownExtCallbackEx
  • One function: wc_EncodeObjectId

The callback types appear as fields in WOLFSSL_CERT_MANAGER, wc_PKCS7, and DecodedCert.

New internal functions were added to handle this change EncodeObjectId_ex and DecodeObjectId_ex both handle
word32 OIDs. The non _ex functions were left due to them being called by FIPS code and that code expects word16.

Non-breaking bug fix. DER encoding packs the first two arcs into a single value (40 x first + second), so decoding assumed the second arc was 0-39. That assumption holds only when the first arc is 0 or 1; when the first arc is 2, the second arc is unbounded and base-128 encodes to a value above 127. Decoding now special-cases this.

Example -- 2.100.3:
encodes -> 0x81, 0x34, 0x03
before: decoded as 4.20.3 (0x81,0x34 unpacked to 180; 180/40=4 rem 20)
after: decodes as 2.100.3

Testing

Added tests for DecodeObjectId and EncodeObjectId covering word32 OIDs, plus a test for an OID whose first arc is 2 and second arc is 100, verifying correct decoding of the byte form.

Checklist

  • added tests
  • updated/added doxygen
  • updated appropriate READMEs
  • updated manual and documentation

@github-actions

github-actions Bot commented Jun 17, 2026

Copy link
Copy Markdown

@aidankeefe2022 aidankeefe2022 added the For This Release Release version 5.9.2 label Jun 17, 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 #10710

Scan targets checked: wolfcrypt-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
Findings: 1

Low (1)

OID arcs widened to word32 still printed with %d

File: wolfcrypt/src/asn.c:14386
Function: GenerateDNSEntryRIDString
Category: Incorrect sizeof/type usage

tmpName was widened from word16 to word32 but is still printed via XSNPRINTF(..., "%d.", tmpName[i]); word32 (unsigned int) no longer matches %d, producing a -Wformat warning that breaks -Werror builds. Same pattern in PrintObjectIdNum (asn.c:37660). Arc values are bounded, so no runtime misprint.

Recommendation: Use "%u" (or cast the argument to int) at both call sites that print word32 OID arcs.

Referenced code: wolfcrypt/src/asn.c:14386-14387 (2 lines)


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

@JacobBarthelmeh

Copy link
Copy Markdown
Contributor

For some reason I had just envisioned an API argument change. It makes sense though for some of the internals of the function to change too, this looks good but it is more risk than I had envisioned.

@JacobBarthelmeh JacobBarthelmeh removed the For This Release Release version 5.9.2 label Jun 17, 2026
@aidankeefe2022 aidankeefe2022 force-pushed the Asn1-word16-word32-migration branch 3 times, most recently from cc4ecf8 to 4f68f69 Compare June 17, 2026 19:09
will not be truncated. Added tests for updated OID handling.

made change fips compatable

removed tests for EncodeObjectId caused symbol error in CI/CD
@aidankeefe2022 aidankeefe2022 force-pushed the Asn1-word16-word32-migration branch from 4f68f69 to 524bf6d Compare June 17, 2026 19:23
@aidankeefe2022

Copy link
Copy Markdown
Member Author

Jenkins retest this please

@Frauschi Frauschi 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.

🐺 Skoll Code Review

Overall recommendation: APPROVE
Findings: 6 total — 5 posted, 4 skipped

Posted findings

  • [Low] DecodeObjectId_ex still caps arcs at 28 bits, so word32 arcs >= 2^28 are rejectedwolfcrypt/src/asn.c:7431-7433
  • [Low] Japanese doxygen for the unknown-ext callback still shows word16doc/dox_comments/header_files-ja/asn_public.h:1937,1951
  • [Low] EncodeObjectId_ex reads in[1] when inSz == 1 (out-of-bounds read)wolfcrypt/src/asn.c:7180-7185
  • [Info] printf uses %d for word32 OID arc valueswolfcrypt/src/asn.c:7555-7557,37819
  • [Info] Leftover #undef LARGE_ARC_EXPECTED for a macro that is never definedtests/api/test_asn.c:1748,1826
Skipped findings
  • [Medium] Public API wc_EncodeObjectId signature changed in place instead of adding an _ex variant
  • [Low] Japanese doxygen for the unknown-ext callback still shows word16
  • [Info] printf uses %d for word32 OID arc values
  • [Info] Leftover #undef LARGE_ARC_EXPECTED for a macro that is never defined

Review generated by Skoll via Claude/Codex

Comment thread wolfcrypt/src/asn.c
}

/* decode bytes */
while (inSz--) {

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.

🔵 [Low] DecodeObjectId_ex still caps arcs at 28 bits, so word32 arcs >= 2^28 are rejected
💡 SUGGEST question

The stated goal of the PR is to represent OID arcs that don't fit in a word16 without truncation. DecodeObjectId_ex outputs word32, but it inherits the if (cnt == 4) return ASN_OBJECT_ID_E; guard from the word16 function, which limits any single arc to 4 base-128 bytes = 28 bits (max 268,435,455). Arcs that need the full word32 range (e.g. the large sub-arcs under 2.25.) still fail to decode. So the widening is partial: it fixes the 2^16..2^28 range (e.g. 113549) but not the full word32 range implied by the type. This is a carry-over of pre-existing behavior, not a regression, but it undercuts the 'word32 without truncation' framing.

Suggestion:

Suggested change
while (inSz--) {
/* allow up to 5 bytes (35 bits worth) and cap the accumulator at word32 */
if (cnt == 5)
return ASN_OBJECT_ID_E;

Comment thread wolfcrypt/src/asn.c
word32 d, t;

/* check args */
if (in == NULL || outSz == NULL || inSz <= 0) {

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.

🔵 [Low] EncodeObjectId_ex reads in[1] when inSz == 1 (out-of-bounds read)
🔧 NIT bug

The arg check is inSz <= 0 (i.e. only rejects inSz == 0, since inSz is unsigned), then unconditionally computes d = in[0]*40 + in[1]. With inSz == 1 this reads in[1] out of bounds before the encode loop (which then does nothing). This mirrors the pre-existing behavior in the word16 EncodeObjectId, so it is a faithful copy rather than a new defect, but the new function now backs the public wc_EncodeObjectId. A valid OID always has at least 2 arcs, so the guard should reflect that.

Suggestion:

Suggested change
if (in == NULL || outSz == NULL || inSz <= 0) {
if (in == NULL || outSz == NULL || inSz < 2) {
return BAD_FUNC_ARG;
}

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.

5 participants