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. ✅
| 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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I made a PR to wolfSSL that adds this check to wc_EncodeObjectId. Should we have checks in both spots for backwards compatibility?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Is this still an issue? It looks like this could still happen from glancing at wc_EncodeObjectId().
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
a3256a0 to
3f3b934
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. ✅
|
Has conflicts now:
|
3f3b934 to
f8136dc
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: 2
2 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
| return MEMORY_E; | ||
| } | ||
|
|
||
| if (XFREAD(*buffer, sizeof(char), fileLen, fp) != (word32)fileLen) { |
There was a problem hiding this comment.
🔵 [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.
There was a problem hiding this comment.
It might be worth specifying a max file size (that's lower than max word32) to compare against before truncating to word32.
philljj
left a comment
There was a problem hiding this comment.
Looks cool. Here's my first pass review.
| XFREE(p->entries , HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER); | ||
| } | ||
|
|
||
| XMEMSET(p, 0, sizeof(*p)); |
There was a problem hiding this comment.
This XMEMSET should probably be wc_ForceZero to prevent compiler optimizing it away.
| 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); |
There was a problem hiding this comment.
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().
There was a problem hiding this comment.
Nothing sensitive!
| return MEMORY_E; | ||
| } | ||
|
|
||
| if (XFREAD(*buffer, sizeof(char), fileLen, fp) != (word32)fileLen) { |
There was a problem hiding this comment.
It might be worth specifying a max file size (that's lower than max word32) to compare against before truncating to word32.
| 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) { |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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[] = { |
There was a problem hiding this comment.
Maybe add a comment saying this is from examples/asn1/oid_names.h from wolfssl repo.
| } | ||
| if (ret == WOLFCLU_SUCCESS) { | ||
| /* Confirm header. */ | ||
| for (i += 10; i < len; i++) { |
There was a problem hiding this comment.
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";
b4694a2 to
f91a34d
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
f91a34d to
98aa32a
Compare
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