Skip to content

Fixed encode policy oid #11019

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
stenslae wants to merge 1 commit into
base: master
Choose a base branch
from
+923 −118
Open

Fixed encode policy oid #11019

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 18 additions & 11 deletions src/ssl_asn1.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5845,14 +5845,16 @@ int wc_OBJ_sn2nid(const char *sn)
* buffer.
*
* String is of the form "1.2.840.113549.1.9.1" and is always NUL
* terminated. Truncated when the buffer is too small.
* terminated. Fails when the buffer is too small; unlike OpenSSL's
* OBJ_obj2txt(), it does not truncate and report the needed length.
*
* @param [out] buf Buffer to hold string.
* @param [in] bufLen Length of buffer in bytes.
* @param [in] a ASN.1 OBJECT_ID object.
* @return Length of string that would be written, excluding the NUL
* terminator, on success.
* @return 0 when decoding the object fails.
* @return ASN_PARSE_E when the object's length cannot be parsed.
*/
static int wolfssl_obj2txt_numeric(char *buf, int bufLen,
const WOLFSSL_ASN1_OBJECT *a)
Expand All @@ -5862,6 +5864,11 @@ int wc_OBJ_sn2nid(const char *sn)
word32 idx = 0;
byte tag;

/* Fail closed: keep buf a valid (empty) NUL-terminated string on
* every error path, since the doc contract promises it is always
* NUL terminated even when decoding fails. */
buf[0] = '\0';

if (GetASNTag(a->obj, &idx, &tag, a->objSz) != 0) {
return WOLFSSL_FAILURE;
}
Expand All @@ -5876,17 +5883,17 @@ int wc_OBJ_sn2nid(const char *sn)
return ASN_PARSE_E;
}

/* save an extra byte for null term. */
if (bufLen < MAX_OID_STRING_SZ) {
bufSz = bufLen - 1;
}
else {
bufSz = MAX_OID_STRING_SZ - 1;
}
/* wc_DecodePolicyOID() accounts for the NUL terminator itself, so
* pass the caller's full buffer length rather than reserving a
* byte here. Do not clamp to MAX_OID_STRING_SZ: that would turn a
* caller-supplied buffer larger than 64 bytes into a spurious
* failure for OID strings >= 64 chars. */
bufSz = bufLen;

if ((bufSz = DecodePolicyOID(buf, (word32)bufSz, a->obj + idx,
if ((bufSz = wc_DecodePolicyOID(buf, (word32)bufSz, a->obj + idx,

@dgarske dgarske Aug 6, 2026

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.

The doc comment states "String is of the form ... and is always NUL terminated. Truncated when the buffer is too small." The PR tightens wc_DecodePolicyOID's length checks from w > outSz - outIdx to w >= outSz - outIdx and adds a bound check on the first XSNPRINTF that had none, so the exact-fit case that previously produced a silently truncated (and therefore wrong) OID string now returns BUFFER_E, which this function maps to WOLFSSL_FAILURE. The stricter behavior is the right call - a silently shortened OID string is worse than an error, and the missing first-XSNPRINTF check was an out-of-bounds out[outIdx] = 0 write when the first identifier alone overflowed the caller's buffer - but the comment now describes behavior the code no longer has, and it diverges from OpenSSL's OBJ_obj2txt, which truncates and returns the length that would have been needed.

Code:

         * String is of the form "1.2.840.113549.1.9.1" and is always NUL
         * terminated. Truncated when the buffer is too small.
         ...
         * @return  0 when decoding the object fails.

Recommendation: Update the comment to say the call fails rather than truncates on a short buffer, and note the OpenSSL-compat divergence if it matters for callers of wolfSSL_OBJ_obj2txt(buf, small, obj, 1).

@stenslae stenslae Aug 6, 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.

I've updated the doc comment to explicitly state that the function will fail and return BUFFER_E when the buffer is too small. I also added a note regarding the divergence from OpenSSL's OBJ_obj2txt behavior.

Comment thread
stenslae marked this conversation as resolved.
Comment thread
stenslae marked this conversation as resolved.
(word32)length)) <= 0) {
WOLFSSL_MSG("Error decoding OID");
buf[0] = '\0';
return WOLFSSL_FAILURE;
}

Expand Down Expand Up @@ -6259,7 +6266,7 @@ int wc_OBJ_sn2nid(const char *sn)
}

#ifdef WOLFSSL_CERT_EXT
ret = EncodePolicyOID(out, &outSz, s, NULL);
ret = wc_EncodePolicyOID(out, &outSz, s, NULL);
if (ret == 0) {
/* sum OID */
sum = wc_oid_sum(out, outSz);
Expand Down Expand Up @@ -6324,7 +6331,7 @@ int wc_OBJ_sn2nid(const char *sn)
return NULL;

/* If s is numerical value, try to sum oid */
ret = EncodePolicyOID(out, &outSz, s, NULL);
ret = wc_EncodePolicyOID(out, &outSz, s, NULL);
if (ret == 0 && outSz > 0) {
/* If numerical encode succeeded then just
* create object from that because sums are
Expand Down
2 changes: 1 addition & 1 deletion src/x509.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1358,7 +1358,7 @@ static int wolfssl_x509_add_custom_ext(WOLFSSL_X509 *x509,

if (err == 0) {
XMEMCPY(val, ext->value.data, ext->value.length);
if (wolfSSL_OBJ_obj2txt(oid, MAX_OID_STRING_SZ, ext->obj, 1) < 0) {
if (wolfSSL_OBJ_obj2txt(oid, MAX_OID_STRING_SZ, ext->obj, 1) <= 0) {

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚪ [Info] Changed obj2txt failure guard in wolfssl_x509_add_custom_ext has no test coverage · Missing edge-case coverage on a function the PR also changed

The guard was widened from < 0 to <= 0 so that a wolfSSL_OBJ_obj2txt() return of exactly 0 no longer stores the uninitialized oid allocation into x509->custom_exts. No test in this PR drives wolfSSL_X509_add_ext() with an extension object whose OID fails to convert, so the newly covered branch is unexercised.

Fix: Add a WOLFSSL_CUSTOM_OID test that calls wolfSSL_X509_add_ext() with an unrecognized-NID extension carrying a malformed OBJECT IDENTIFIER.

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚪ [Info] Tightened wolfssl_x509_add_custom_ext return check has no test coverage · Missing edge-case coverage on a function the PR also changed

The < 0 to <= 0 change closes the path where a WOLFSSL_FAILURE (0) return let the uninitialized oid allocation be stored in x509->custom_exts[] and later read as a C string. No test drives wolfSSL_X509_add_ext with a custom-OID extension whose OBJ_obj2txt fails, so this branch stays uncovered.

Fix: Add a WOLFSSL_CUSTOM_OID test that adds an extension with an undecodable OBJECT IDENTIFIER and asserts wolfSSL_X509_add_ext returns failure.

err = 1;
}
}
Expand Down
Loading
Loading