Skip to content

raise ValueError for unsupported name attribute value tags#15076

Open
dxbjavid wants to merge 1 commit into
pyca:mainfrom
dxbjavid:name-attribute-tag-valueerror
Open

raise ValueError for unsupported name attribute value tags#15076
dxbjavid wants to merge 1 commit into
pyca:mainfrom
dxbjavid:name-attribute-tag-valueerror

Conversation

@dxbjavid

Copy link
Copy Markdown
Contributor

Parsing an X.509 name attribute whose value carries an ASN.1 tag outside the recognised string types looked the tag up in the _ASN1_TYPE_TO_ENUM dict without handling a miss, so a crafted certificate, CSR, CRL or OCSP response made accessing subject or issuer raise a bare KeyError rather than the ValueError every other malformed-input path raises. The value itself is already accepted into the catch-all AnyString variant, so only the type lookup was left unguarded. I map the miss to an invalid-value parse error so it surfaces as a ValueError with the usual field location, and add a regression test that mutates a built certificate's commonName tag.

@alex alex left a comment

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.

sorry about the slow review

Comment thread tests/x509/test_x509.py
# UTF8String tag with an unsupported tag value. The first occurrence
# is the issuer name.
idx = der.index(b"\x06\x03\x55\x04\x03")
der[idx + 5] = 0x69

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 right way to do these tests is with a checked in vector, not by generating and then manipulating the value

let py_tag = types::ASN1_TYPE_TO_ENUM
.get(py)?
.get_item(tag_val)
.map_err(|_| asn1::ParseError::new(asn1::ParseErrorKind::InvalidValue))?;

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.

I think UnexpectedTag is a more correct error to return here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants