Skip to content

X509_NAME_oneline: escape '/', '+' and '\' inside attribute values - #11446

Open
aidankeefe2022 wants to merge 1 commit into
wolfSSL:masterfrom
aidankeefe2022:wolfssl_issue_x509
Open

aidankeefe2022 wants to merge 1 commit into
wolfSSL:masterfrom
aidankeefe2022:wolfssl_issue_x509

Conversation

@aidankeefe2022

Copy link
Copy Markdown
Member

A single attribute whose value contains the RDN separator rendered byte-identical to a name made of several attributes: CN="foo/O=bar" and CN=foo, O=bar both came out as "/CN=foo/O=bar". X509_NAME_cmp() compares this string, so the two names compared equal, and so did the issuer checks built on it.

Description

Add X509CertEscapeName(), which prefixes every '', '/' and '+' in a value with '', and use it wherever the one-line form is built:

  • GetRDN() and GetCertName(), which build the DecodedCert subject and issuer strings. The WC_ASN_NAME_MAX check now uses the escaped length.
  • AddAllEntry() and RebuildFullName(), which rebuild the string from the X509_NAME entries. The sizing pass skips entries with no value.

OpenSSL 3 escapes '/' and '+'. '' is escaped here as well: otherwise CN="foo", O=bar still renders as CN="foo/O=bar" does, and CN="a+b" as CN="a+b" does. A value containing '' therefore renders differently than it does in OpenSSL.

This changes the strings from wolfSSL_X509_NAME_oneline(), wc_GetDecodedCertSubject() and wc_GetDecodedCertIssuer() for values containing these characters, and so the caSubject that wc_OcspResponder_AddSigner() expects. Document the escaping for each.

Fixes zd#

Testing

test_wolfSSL_X509_NAME_oneline_escape covers names built from entries and names parsed from a signed certificate, that X509_NAME_dup() does not escape twice, and a value long enough that escaping doubles it.

Checklist

  • added tests
  • updated/added doxygen

A single attribute whose value contains the RDN separator rendered
byte-identical to a name made of several attributes: CN="foo/O=bar"
and CN=foo, O=bar both came out as "/CN=foo/O=bar". X509_NAME_cmp()
compares this string, so the two names compared equal, and so did the
issuer checks built on it.

Add X509CertEscapeName(), which prefixes every '\', '/' and '+' in a
value with '\', and use it wherever the one-line form is built:

- GetRDN() and GetCertName(), which build the DecodedCert subject and
  issuer strings. The WC_ASN_NAME_MAX check now uses the escaped
  length.
- AddAllEntry() and RebuildFullName(), which rebuild the string from
  the X509_NAME entries. The sizing pass skips entries with no value.

OpenSSL 3 escapes '/' and '+'. '\' is escaped here as well:
otherwise CN="foo\", O=bar still renders as CN="foo/O=bar" does, and
CN="a\+b" as CN="a+b" does. A value containing '\' therefore renders
differently than it does in OpenSSL.

This changes the strings from wolfSSL_X509_NAME_oneline(),
wc_GetDecodedCertSubject() and wc_GetDecodedCertIssuer() for values
containing these characters, and so the caSubject that
wc_OcspResponder_AddSigner() expects. Document the escaping for each.

test_wolfSSL_X509_NAME_oneline_escape covers names built from entries
and names parsed from a signed certificate, that X509_NAME_dup() does
not escape twice, and a value long enough that escaping doubles it.

Issue: wolfSSL#11392

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

Scan targets checked: wolfcrypt-src, wolfcrypt-bugs, wolfssl-src, wolfssl-bugs

Findings: 4
4 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Reported findings require changes before merge.

Comment thread wolfcrypt/src/asn.c
Comment thread wolfcrypt/src/asn_orig.c
Comment thread wolfcrypt/src/asn_orig.c
Comment thread src/x509.c
@github-actions

Copy link
Copy Markdown

MemBrowse Memory Report

gcc-arm-cortex-m0plus

  • FLASH: .text +88 B (+0.1%, 67,047 B / 262,144 B, total: 26% used)

gcc-arm-cortex-m3

  • FLASH: .text +64 B (+0.1%, 125,839 B / 262,144 B, total: 48% used)

gcc-arm-cortex-m4

  • FLASH: .text +64 B (+0.0%, 205,021 B / 262,144 B, total: 78% used)

gcc-arm-cortex-m4-baremetal

  • FLASH: .text +64 B (+0.1%, 69,539 B / 262,144 B, total: 27% used)

gcc-arm-cortex-m4-crypto-only

  • FLASH: .text +64 B (+0.0%, 178,968 B / 262,144 B, total: 68% used)

gcc-arm-cortex-m4-dtls13

  • FLASH: .text +64 B (+0.0%, 187,324 B / 1,048,576 B, total: 18% used)

gcc-arm-cortex-m4-min-ecc

  • FLASH: .text +64 B (+0.1%, 64,389 B / 262,144 B, total: 25% used)

gcc-arm-cortex-m4-openssl-compat

  • FLASH: .text +64 B (+0.0%, 783,908 B / 1,048,576 B, total: 75% used)

gcc-arm-cortex-m4-pkcs7

  • FLASH: .text +64 B (+0.0%, 217,692 B / 262,144 B, total: 83% used)

gcc-arm-cortex-m4-pq

  • FLASH: .text +64 B (+0.0%, 302,152 B / 1,048,576 B, total: 29% used)

gcc-arm-cortex-m4-sp-math

  • FLASH: .text +64 B (+0.1%, 64,389 B / 262,144 B, total: 25% used)

gcc-arm-cortex-m4-tls12

  • FLASH: .text +64 B (+0.1%, 126,643 B / 262,144 B, total: 48% used)

gcc-arm-cortex-m4-tls13

  • FLASH: .text +64 B (+0.0%, 242,703 B / 262,144 B, total: 93% used)

gcc-arm-cortex-m7

  • FLASH: .text +64 B (+0.0%, 205,021 B / 262,144 B, total: 78% used)

gcc-arm-cortex-m7-pq

  • FLASH: .text +64 B (+0.0%, 303,112 B / 1,048,576 B, total: 29% used)

gcc-arm-cortex-m7-tls13

  • FLASH: .text +64 B (+0.0%, 242,703 B / 262,144 B, total: 93% used)

linuxkm-pie

  • Data: __patchable_function_entries +24 B (+0.1%, 27,096 B)

linuxkm-standard

  • Data: __patchable_function_entries +16 B (+0.0%, 49,888 B)

stm32-sim-stm32h753

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

Fenrir already completed a review of this PR at commit 8c3db19c5b5f (run 2502); its findings are the review threads on the PR. Push new commits to get a re-review of what changed, or comment @wolfSSL-Fenrir-bot review force to run the full review again at this commit.

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.

2 participants