Asn1parse - #253
Conversation
|
Tests will fail until wolfssl get this PR merged |
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
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
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
74bea95 to
c22c6b1
Compare
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
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
| #endif | ||
|
|
||
| typedef struct WOLFCLU_ASN1_PARSE_OPTIONS | ||
| #if !defined(NO_FILESYSTEM) && defined(WOLFSSL_ASN_PRINT) |
There was a problem hiding this comment.
Does this need to use WOLFCLU_NO_FILESYSTEM?
| word32 cap; | ||
| }WOLFCLU_OID_TO_NAME; | ||
|
|
||
| #if defined(HAVE_OID_TABLE) && !defined(NO_FILESYSTEM) && \ |
There was a problem hiding this comment.
should be WOLFCLU_NO_FILESYSTEM as well?
| static const OidName oid_name_table[1] = {0}; | ||
| static const word32 oid_names_len = 0; | ||
|
|
||
| #endif /* defined(HAVE_OID_TABLE) && !defined(NO_FILESYSTEM) && |
| #include <wolfssl/wolfcrypt/camellia.h> | ||
| #endif | ||
|
|
||
| /* API break introduced in 5.9.3 so compile out affected functionality |
There was a problem hiding this comment.
I don't understand this. The latest wolfssl release is 5.9.2, so this is hard disabled:
./wolfssl asn1parse -oid
wolfSSL is not configured to handle encoding oids
Error returned: -1.
There was a problem hiding this comment.
Yes this functionality relies on this PR wolfSSL/wolfssl#10710 (that breaks the API) which in turn needs this PR to be merged to have its tests pass. I hope to have the API break PR in 5.9.3 so this is preemptive. In the mean time if it is not in 5.9.3 I will update these errors and guards.
|
|
||
| # 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.
I think it will actually print Cannot parse Asn1 in this scenario, so this detection won't work.
Could this be detected in a better way?
There was a problem hiding this comment.
I agree this is fragile I changed it to search the binary for a long string in the OID table and if it exists we know the table is included in the build and can run the tests
| #define WOLFSSL_WC_DILITHIUM | ||
| #define WOLFSSL_EXPERIMENTAL_SETTINGS | ||
| #define WOLFSSL_DUAL_ALG_CERTS | ||
| #define WOLFSSL_ASN_PRINT |
There was a problem hiding this comment.
Should this user settings define HAVE_OID_TABLE?
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.
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