Repository navigation
Asn1parse - #253
Asn1parse#253aidankeefe2022 wants to merge 7 commits into
Conversation
|
Tests will fail until wolfssl get this PR merged |
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #253
Scan targets checked: wolfclu-bugs, wolfclu-src
Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
e0068dd to
cfd1d42
Compare
cfd1d42 to
62149fa
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #253
Scan targets checked: wolfclu-bugs, wolfclu-src
No new issues found in the changed files. ✅
eb7fef0 to
9a1ccd0
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #253
Scan targets checked: wolfclu-bugs, wolfclu-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.
|
Jenkins retest this please |
4b15fc7 to
60b6f5a
Compare
c22c6b1 to
b59f371
Compare
There was a problem hiding this comment.
Pull request overview
Adds an asn1parse subcommand to wolfCLU modeled after OpenSSL’s tool, including re-parsing of nested BIT STRING / OCTET STRING payloads (-strparse) and support for user-supplied OID name mappings (-oid), plus build-time optional inclusion of a large built-in OID table.
Changes:
- Introduces
asn1parsecommand wiring, option parsing, and ASN.1 decode/print pipeline (DER/PEM/B64, offset/length/strparse, dump/indent/noout). - Adds optional built-in OID name table via
--enable-oid-tableconfigure flag, along with manpage documentation. - Adds Python tests for the new tool (including config probing) and a parallel Python test runner.
Reviewed changes
Copilot reviewed 19 out of 22 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| wolfclu/include.am | Installs new ASN.1 public header; adds OID table header to build. |
| wolfclu/clu_optargs.h | Adds new mode/flags enums for asn1parse. |
| wolfclu/clu_header_main.h | Version-gates NO_WC_ENCODE_OBJECT_ID; declares wolfCLU_StrToWord32. |
| wolfclu/asn1/clu_asn1.h | Defines ASN.1 parse options struct and public entry points. |
| wolfCLU.vcxproj.filters | Adds ASN.1 header to Visual Studio filters. |
| wolfCLU.vcxproj | Adds ASN.1 sources/headers to Windows project build. |
| tests/wolfclu_test.py | Adds helper to read generated wolfclu/config.h defines for feature-gated tests. |
| tests/run_tests_parallel.py | New parallel test runner for Python test files. |
| tests/asn1/include.am | Hooks ASN.1 test script into automake test distribution. |
| tests/asn1/asn1-test.py | Adds functional tests for DER/PEM/B64, offset/length, strparse, OID mapping, and OID table. |
| src/x509/clu_config.c | Refactors RID parsing to use wolfCLU_StrToWord32 and supports 32-bit arcs. |
| src/tools/clu_funcs.c | Adds asn1parse to help output; implements wolfCLU_StrToWord32. |
| src/include.am | Adds ASN.1 sources to automake build. |
| src/clu_main.c | Adds asn1parse mode and dispatch to wolfCLU_Asn1Setup. |
| src/asn1/clu_asn1_setup.c | New CLI option parsing / validation for asn1parse. |
| src/asn1/clu_asn1_parse.c | New core ASN.1 parsing/printing, PEM finding, base64 handling, OID callback support. |
| manpages/wolfssl-asn1parse.1 | New man page for wolfssl asn1parse. |
| Makefile.am | Installs the new man page; includes ASN.1 tests. |
| ide/winvs/user_settings.h | Enables WOLFSSL_ASN_PRINT for the Windows VS settings. |
| configure.ac | Adds --enable-oid-table and version gating for NO_WC_ENCODE_OBJECT_ID. |
| .gitignore | Updates ignored config template path. |
Comments suppressed due to low confidence (2)
src/asn1/clu_asn1_parse.c:288
- FindPem() confirms the PEM header by running XSTRNCMP(data+i, "-----", 5) without checking that i+5 is within bounds. This can read out of bounds when i is near len; guard the compare with a remaining-length check.
for (; i < len; i++) {
if ((data[i] == '-') &&
(XSTRNCMP((char*)data + i, "-----", 5) == 0)) {
break;
src/asn1/clu_asn1_parse.c:303
- FindPem() searches for the PEM footer using XSTRNCMP(data+j, "-----END", 8) without checking that j+8 is within bounds. Add a bounds check so the scan can't read past the end of the buffer.
for (j = i + 1; j < len; j++) {
if ((data[j] == '-') &&
(XSTRNCMP((char*)data + j, "-----END", 8) == 0)) {
break;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
35c7932 to
ba41d70
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #253
Scan targets checked: wolfclu-bugs, wolfclu-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.
|
No new feedback, just waiting on prerequisite wolfssl PR to settle: |
with openssl. added help to explain oid command added comments to header file resolved configure.ac merge conflict added oid support to header includes added man page for asn1parse and fixed more skoll oid are properly handled Made -out output DER of input after processing Removed extra fmt file and put all in parse custom oid now compiled out for 5.9.2 added header to include.am fenrir fixes moved oid table added WOLFSSL_ASN_PRINT to windows build skoll fixes
|
rebase and added wolfSSL name change |
|
Broken rn waiting for wolfSSL PR to merge |
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| #if LIBWOLFSSL_VERSION_HEX <= 0x05009002 | ||
| #ifndef NO_WC_ENCODE_OBJECT_ID | ||
| #define NO_WC_ENCODE_OBJECT_ID | ||
| #endif | ||
| #endif |
There was a problem hiding this comment.
🔴 With wolfSSL 5.9.2 or older, a dotted numeric RID subjectAltName in a req/x509 config now fails with "Couldn't encode RID". On the base branch it encoded fine. The header forces NO_WC_ENCODE_OBJECT_ID for every LIBWOLFSSL_VERSION_HEX <= 0x05009002, which compiles out the RID encoder in clu_config.c:383. Fix: keep RID working on old wolfSSL by choosing the decoded element type by version (word16 for <= 5.9.2, word32 after). Limit the forced NO_WC_ENCODE_OBJECT_ID to the asn1parse -oid path, which needs wc_EncodeObjectId32.
Why this was flagged
The trigger is a user linking against wolfSSL 5.9.2 or earlier with HAVE_OID_ENCODING, whose config has e.g. RID.1 = 1.2.3.4.5 with an OID that wolfSSL_OBJ_txt2obj does not know by name. On the base branch, wolfCLU_addAltName (src/x509/clu_config.c:381-435) built a word16 array and called wc_EncodeObjectId, which takes word16 there. AC_CHECK_FUNC in configure.ac only checks that the function exists, so that path compiled and worked. After this change, clu_header_main.h:116-120 defines NO_WC_ENCODE_OBJECT_ID for all of these versions. The #else at clu_config.c:436-450 then runs and returns WOLFCLU_FATAL_ERROR, and cert/req generation fails. The PR description only mentions the new -oid feature being unavailable on old wolfSSL, not this regression in existing RID support.
Verification: wolfclu/clu_header_main.h:116-120 now does #if LIBWOLFSSL_VERSION_HEX <= 0x05009002 ... #define NO_WC_ENCODE_OBJECT_ID, which compiles out the RID encoder at clu_config.c:383. The #else branch at lines 444-450 returns WOLFCLU_FATAL_ERROR. On the base commit this path compiled and worked on those versions. CI only builds against wolfssl master (ci.yml:65), so CI does not catch it.
| ret = WOLFCLU_FATAL_ERROR; | ||
| break; | ||
| } | ||
| /* require decimal-digits-only token so that things | ||
| * like "1a" or "abc" are rejected instead of being | ||
| * silently coerced by XATOI */ | ||
| for (digit = token; *digit != '\0'; digit++) { | ||
| if (*digit < '0' || *digit > '9') { | ||
| wolfCLU_LogError("Non-numeric RID " | ||
| "component '%s' in: %s", token, | ||
| value); | ||
| ret = WOLFCLU_FATAL_ERROR; | ||
| break; | ||
| } | ||
| } | ||
| if (ret != WOLFCLU_SUCCESS) { | ||
| break; | ||
| } | ||
| n = XATOI(token); | ||
| if (n < 0 || n > 0xFFFF) { | ||
| wolfCLU_LogError("RID component out of range " | ||
| "[0, 65535]: %s", token); | ||
| if (wolfCLU_StrToWord32(token, XSTRLEN(token), &n) != | ||
| WOLFCLU_SUCCESS) { | ||
| wolfCLU_LogError("RID component not a valid 32 bit" | ||
| " integer : %s", token); | ||
| ret = WOLFCLU_FATAL_ERROR; | ||
| break; | ||
| } | ||
| decoded[decodedCount] = (word16)n; | ||
| decoded[decodedCount] = n; | ||
| decodedCount++; | ||
| token = XSTRTOK(NULL, ".", &ptr); | ||
| } |
There was a problem hiding this comment.
🔴 Users whose RID alt name takes the manual-encoding fallback can get a wrong OID in the certificate, or a failed build, on wolfSSL newer than 5.9.2. The array changed from word16 to word32 but still goes to wc_EncodeObjectId at clu_config.c:427. The PR's last commit moved OidToDer to wc_EncodeObjectId32, which implies wc_EncodeObjectId still takes word16*. Fix: give the RID path the same 32-bit API as OidToDer: call wc_EncodeObjectId32 with the word32 array. Where only the word16 API exists, keep the base word16 array and its 0..65535 range check.
Why this was flagged
The trigger is a req/x509 config or -addext with RID.n = <dotted OID> that wolfSSL_OBJ_txt2obj rejects (clu_config.c:382), so the manual path at clu_config.c:383-435 runs. That path is compiled only when wolfSSL is newer than 5.9.2, because clu_header_main.h:116 forces NO_WC_ENCODE_OBJECT_ID otherwise. Under the same guard, commit 28b33a0 switched OidToDer (clu_asn1_parse.c:429) from wc_EncodeObjectId to wc_EncodeObjectId32 for its word32 arcs. clu_config.c:351 still declares word32 decoded[] and passes it to wc_EncodeObjectId at :427. If that function takes const word16*, GCC 14 rejects the incompatible pointer type and the build fails. Older compilers only warn, so on little-endian builds wolfSSL reads the halves of each word32. The base branch used word16 decoded[] with a 0..65535 range check, which matches that signature. The only RID test (x509-req-test.py:464, RID:1.2.3.4) resolves through txt2obj and never reaches this path.
Verification: Trigger: building against a wolfSSL newer than 5.9.2 in which wc_EncodeObjectId takes const word16*. src/x509/clu_config.c:351 now declares word32 decoded[ASN1_OID_DOTTED_MAX_SZ]; (it was word16). clu_config.c:427 still passes that array to wc_EncodeObjectId(decoded, decodedCount, oid, &oidSz). On compilers that treat incompatible pointer types as an error (GCC 14+), the build fails.
|
|
||
| # Skip when asn1parse is not compiled in (needs WOLFSSL_ASN_PRINT). | ||
| result = run_wolfssl("asn1parse", "-inform", "DER", "-in", cls.der) | ||
| if "Cannot Parse Asn1" in (result.stdout + result.stderr): |
There was a problem hiding this comment.
🔴 On wolfSSL builds without WOLFSSL_ASN_PRINT, the asn1parse test suite fails every test instead of skipping. The skip probe looks for "Cannot Parse Asn1" (capital P). The setup-layer message that actually prints there says "Cannot parse Asn1." (lowercase p). Fix: make both C messages use the same text and match it case-insensitively in the probe, or probe for "WOLFSSL_ASN_PRINT option not set", so every not-compiled-in path is recognised.
Why this was flagged
The trigger is a wolfSSL build where WOLFSSL_ASN_PRINT is not defined, run through make check / run_tests_parallel.py. In that build, wolfCLU_Asn1Setup at src/asn1/clu_asn1_setup.c returns from its #else branch before wolfCLU_Asn1Parse is ever called. It logs "WOLFSSL_ASN_PRINT option not set. Cannot parse Asn1." with a lowercase p. The capital-P "Cannot Parse Asn1." text lives only in the unreachable #else of wolfCLU_Asn1Parse (src/asn1/clu_asn1_parse.c:679). setUpClass at tests/asn1/asn1-test.py:57 uses a case-sensitive in, so it never matches and SkipTest is never raised. Every test in TestBasicFunctions then fails, for example on the returncode and "SEQUENCE" asserts. The suite is new, so the base branch has no such failure.
Verification: The skip probe at tests/asn1/asn1-test.py:57 is case-sensitive: if "Cannot Parse Asn1" in (result.stdout + result.stderr):. In the non-ASN_PRINT build, src/asn1/clu_asn1_setup.c:339 logs "WOLFSSL_ASN_PRINT option not set. Cannot parse Asn1." and returns WOLFCLU_FATAL_ERROR. As a result the probe never matches and setUpClass never raises SkipTest.
| token = XSTRTOK((char *)fileBuffer, " ", &tracker); | ||
| while (ret == WOLFCLU_SUCCESS && token != NULL) { | ||
| word32 len; | ||
| oid = token; | ||
| len = XSTRLEN(oid); | ||
|
|
||
| ret = OidToDer(oid, &len); | ||
| if (ret != WOLFCLU_SUCCESS) { | ||
| wolfCLU_LogError("Could not convert Oid : %s to der check line %u", | ||
| token, newOids->len + 1); | ||
| break; | ||
| } | ||
|
|
||
| /* skip short name */ | ||
| token = XSTRTOK(NULL, " ", &tracker); | ||
| if (token == NULL || XSTRSTR(token, "\n") != NULL) { | ||
| wolfCLU_LogError("malformed tokens, check line %u for bad " | ||
| "formatting; wanted short name", | ||
| newOids->len + 1); | ||
| ret = WOLFCLU_FATAL_ERROR; | ||
| break; | ||
| } | ||
|
|
||
| token = XSTRTOK(NULL, "\n", &tracker); | ||
| if (token == NULL) { | ||
| wolfCLU_LogError("malformed tokens, check line %u for bad " | ||
| "formatting; wanted long name", | ||
| newOids->len + 1); | ||
| ret = WOLFCLU_FATAL_ERROR; | ||
| break; | ||
| } | ||
|
|
||
| /* realloc array if need more slots */ | ||
| if (newOids->len >= newOids->cap) { | ||
| tmpPtr = XREALLOC(newOids->entries, | ||
| sizeof(OidName) * (newOids->cap + 10), HEAP_HINT, | ||
| DYNAMIC_TYPE_TMP_BUFFER); | ||
|
|
||
| if (tmpPtr == NULL) { | ||
| ret = MEMORY_E; | ||
| break; | ||
| } | ||
| else { | ||
| newOids->entries = tmpPtr; | ||
| newOids->cap += 10; | ||
| } | ||
| } | ||
|
|
||
| newOids->entries[newOids->len].name = token; | ||
| newOids->entries[newOids->len].oid = (byte *)oid; | ||
| newOids->entries[newOids->len].len = len; | ||
| newOids->len++; | ||
|
|
||
| token = XSTRTOK(NULL, " ", &tracker); |
There was a problem hiding this comment.
🔴 Users whose -oid file has a blank line, or ends with an extra newline, get no output: asn1parse aborts with "Could not convert Oid". The loader splits on spaces, not lines. After a long name, the next XSTRTOK(NULL, " ") returns "\n" or "\n1.2.3". OidToDer rejects that token, so the whole file fails. Fix: parse the buffer one line at a time, trim CR and surrounding whitespace, and skip empty lines before splitting out oid, short name and long name. The same per-line trim also strips the trailing '\r' that CRLF files leave in each long name.
Why this was flagged
Take an -oid file such as "1.2.3.4 a A\n\n1.2.3.5 b B\n", or one that ends in "\n\n". Pass it via wolfssl asn1parse -in x.der -oid f, which reaches WOLFCLU_OID_TO_NAME_initWFile from wolfCLU_Asn1Parse (src/asn1/clu_asn1_parse.c:633). The long-name XSTRTOK(..., "\n") at line 499 consumes only one newline. XSTRTOK(NULL, " ") at line 529 then returns a token that starts with '\n'. OidToDer (line 482) passes it to wolfCLU_StrToWord32, which rejects the non-digit, so ret is set to WOLFCLU_FATAL_ERROR. The run logs "Could not convert Oid ... check line N" and "Error parsing file passed to -oid flag" and exits with an error without printing anything. Nothing skips blank lines, and the only test (asn1-test.py:218, :253) writes one line with at most a single trailing newline. With CRLF files the '\r' stays in entries[].name and is printed with each resolved OID. This is new code, so the base has no -oid feature to compare against.
Verification: Trigger: the -oid file has a blank line between entries or ends with more than one newline. In src/asn1/clu_asn1_parse.c, line 529, token = XSTRTOK(NULL, " ", &tracker);, skips only spaces. Line 482 calls OidToDer on that token. wolfCLU_StrToWord32 rejects any non-digit (src/tools/clu_funcs.c:1329). The command fails and prints no parse output.
This is based on the asn1.c in the wolfSSL examples. But with the added new strparse feature which allows for the re-parsing of BIT STRINGS and OCTET STRINGS. And the ability for the user to load a custom OID database. Both of these are feature ideas from OpenSSL
The flags and behavior are meant to mimic OpenSSL to allow for users to be comfortable with the tool. Not all of the asn1parse capability is included however.
A man page has been created along with tests for this tool.
A new flag was introduced that enables the build in OID table this is because the table make the binary pretty large. We could remove the table or make it always on if the configure flag is overkill.
The diff says +7000 something but minus the OID table it is about 2000 new lines. The table was taken from asn1.c example code.
If wolfSSL is 5.9.2 or less the -oid feature will not work due to API break