Skip to content

Asn1parse#253

Open
aidankeefe2022 wants to merge 1 commit into
wolfSSL:mainfrom
aidankeefe2022:ans1-parser
Open

Asn1parse#253
aidankeefe2022 wants to merge 1 commit into
wolfSSL:mainfrom
aidankeefe2022:ans1-parser

Conversation

@aidankeefe2022

@aidankeefe2022 aidankeefe2022 commented Jun 17, 2026

Copy link
Copy Markdown
Member

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

@aidankeefe2022

Copy link
Copy Markdown
Member Author

Tests will fail until wolfssl get this PR merged

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

Comment thread wolfclu/asn1/clu_asn1.h Outdated
Comment thread src/asn1/clu_asn1_fmt.c Outdated
Comment thread src/asn1/clu_asn1_fmt.c Outdated
Comment thread wolfclu/asn1/clu_asn1.h Outdated

@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 #253

Scan targets checked: wolfclu-bugs, wolfclu-src

No new issues found in the changed files. ✅

Comment thread wolfclu/asn1/clu_oid_name_table.h
Comment thread src/asn1/clu_asn1_parse.c
if (ret == WOLFCLU_SUCCESS) {
/* oid is overwritten by and replaces with DER encoding */
XMEMSET(oid, '\0', *oidSz);
if ((err = wc_EncodeObjectId(arc, idx, (byte*)oid, oidSz)) != 0) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Claude has some thoughts on an extra check in this function:

The -oid file format is per line; if the OID field contains only a single number with no dots (e.g. the line 5 myName My Long Name), OidToDer allocates the arc array with one element per character of the OID string — here just 1 — and calls wc_EncodeObjectId(arc, 1, ...). wolfSSL's EncodeObjectId only rejects inSz <= 0 and always computes the first byte as in[0]*40 + in[1], so with inSz == 1 it reads arc[1], one element past the end of the heap allocation. Please reject OIDs with fewer than 2 components before calling wc_EncodeObjectId.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I made a PR to wolfSSL that adds this check to wc_EncodeObjectId. Should we have checks in both spots for backwards compatibility?

@aidankeefe2022 aidankeefe2022 Jul 2, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Just checked again and the wc_EncodeObjectId function is not compiled in unless it has the updated arg checking this is guarded by the NO_WC_ENCODE_OBJECT_ID macro that checks wolfSSL version

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.

Is this still an issue? It looks like this could still happen from glancing at wc_EncodeObjectId().

Comment thread src/asn1/clu_asn1_parse.c Outdated
Comment thread src/asn1/clu_asn1_setup.c Outdated
Comment thread wolfCLU.vcxproj Outdated
Comment thread src/asn1/clu_asn1_setup.c
Comment thread src/asn1/clu_asn1_setup.c Outdated
Comment thread src/asn1/clu_asn1_setup.c Outdated
Comment thread src/asn1/clu_asn1_setup.c Outdated
Comment thread src/x509/clu_config.c Outdated
@cconlon cconlon assigned aidankeefe2022 and unassigned cconlon Jul 1, 2026
@aidankeefe2022 aidankeefe2022 force-pushed the ans1-parser branch 3 times, most recently from eb7fef0 to 9a1ccd0 Compare July 2, 2026 18:28

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

Comment thread src/asn1/clu_asn1_parse.c
Comment thread src/asn1/clu_asn1_parse.c
@aidankeefe2022

Copy link
Copy Markdown
Member Author

Jenkins retest this please

@aidankeefe2022 aidankeefe2022 force-pushed the ans1-parser branch 2 times, most recently from 4b15fc7 to 60b6f5a Compare July 2, 2026 21:59

@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 #253

Scan targets checked: wolfclu-bugs, wolfclu-src

No new issues found in the changed files. ✅

@dgarske dgarske assigned wolfSSL-Bot and philljj and unassigned cconlon Jul 7, 2026
@dgarske dgarske requested a review from philljj July 7, 2026 21:39
@philljj

philljj commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Has conflicts now:

  • wolfclu/clu_header_main.h
  • wolfclu/clu_optargs.h

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

Comment thread src/asn1/clu_asn1_parse.c
Comment thread src/asn1/clu_asn1_parse.c
return MEMORY_E;
}

if (XFREAD(*buffer, sizeof(char), fileLen, fp) != (word32)fileLen) {

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] Integer truncation in asn1_ReadFile XFREAD check and bufLen assignment · Integer overflows

XFREAD returns size_t (64-bit on LP64) but is compared against (word32)fileLen, silently truncating fileLen when it exceeds UINT32_MAX; *bufLen = (word32)fileLen at line 364 has the same truncation, causing callers to operate with a wrong data size.

Fix: Cast the XFREAD return value and fileLen consistently to size_t, and add an explicit fileLen > (long)UINT32_MAX guard before narrowing to word32.

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.

It might be worth specifying a max file size (that's lower than max word32) to compare against before truncating to word32.

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

Looks cool. Here's my first pass review.

Comment thread src/asn1/clu_asn1_parse.c Outdated
XFREE(p->entries , HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER);
}

XMEMSET(p, 0, sizeof(*p));

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.

This XMEMSET should probably be wc_ForceZero to prevent compiler optimizing it away.

Comment thread src/asn1/clu_asn1_parse.c
static int WOLFCLU_OID_TO_NAME_free(WOLFCLU_OID_TO_NAME* p)
{
if (p->dataBuffer != NULL) {
XFREE(p->dataBuffer, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER);

@philljj philljj Jul 15, 2026

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.

Do p->dataBuffer or p->entries hold sensitive (private key, etc) data?

If so, then need wc_ForceZero() here as well.

But you will need to save off the length of dataBuffer and entries members during WOLFCLU_OID_TO_NAME_initWFile().

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Nothing sensitive!

Comment thread src/asn1/clu_asn1_parse.c
Comment thread src/asn1/clu_asn1_parse.c
return MEMORY_E;
}

if (XFREAD(*buffer, sizeof(char), fileLen, fp) != (word32)fileLen) {

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.

It might be worth specifying a max file size (that's lower than max word32) to compare against before truncating to word32.

Comment thread src/asn1/clu_asn1_parse.c
if (ret == WOLFCLU_SUCCESS) {
/* oid is overwritten by and replaces with DER encoding */
XMEMSET(oid, '\0', *oidSz);
if ((err = wc_EncodeObjectId(arc, idx, (byte*)oid, oidSz)) != 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.

Is this still an issue? It looks like this could still happen from glancing at wc_EncodeObjectId().


typedef struct WOLFCLU_OID_TO_NAME{
OidName* entries;
byte* dataBuffer;

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.

I think the WOLFCLU_OID_TO_NAME members need some short comments explaining what they do.

E.g. when I first looked in the code it seemed dataBuffer wasn't actually used, before I realized entries is a dynamic table that indexes tokens into dataBuffer.


#ifdef HAVE_OID_TABLE

static const OidName oid_name_table[] = {

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.

Maybe add a comment saying this is from examples/asn1/oid_names.h from wolfssl repo.

Comment thread src/asn1/clu_asn1_parse.c Outdated
}
if (ret == WOLFCLU_SUCCESS) {
/* Confirm header. */
for (i += 10; i < len; i++) {

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.

It could be replaced with something like

#define BEGIN_PRIV_KEY_PREFIX "-----BEGIN"

or like we do in wolfcrypt/src/asn.c

static wcchar BEGIN_PRIV_KEY_PREFIX = "-----BEGIN";

@philljj philljj assigned aidankeefe2022 and unassigned philljj Jul 15, 2026
@aidankeefe2022 aidankeefe2022 force-pushed the ans1-parser branch 5 times, most recently from b4694a2 to f91a34d Compare July 15, 2026 20:32
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
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