https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=19537 https://oss-fuzz.com/testcase-detail/5750762605117440 Jeremy: I have noticed that you have a strong emotional response to certain combinations of the characters 'A', 'S', 'N', and '1'. I am sorry to say that means you spring to mind when I see that word, and I assume you have LOTS OF VALUABLE EXPERIENCE. The fuzzer is complaining about this function: /* read an integer */ bool asn1_read_enumerated(struct asn1_data *data, int *v) { *v = 0; if (!asn1_start_tag(data, ASN1_ENUMERATED)) return false; while (!data->has_error && asn1_tag_remaining(data)>0) { uint8_t b; if (!asn1_read_uint8(data, &b)) { return false; } *v = (*v << 8) + b; } return asn1_end_tag(data); } because *v builds up to be greater than 1<<24, then on the next round it is not going to fit in 32 bits. It calls this undefined, which is technically true I guess, though in practice it is always going to chuck out some significant bits and possibly flip the sign. But that still doesn't really seem good. And if we limit it sizeof(int) rounds, it can still flip the sign, which is probably undefined anyway. It is lucky that sizeof(int) is de-facto fixed, otherwise we'd get different decodings on different architectures. What is the right thing to do here?
I'll try and take a look soon. Thanks.
Here is what libssl does here: https://manpages.debian.org/unstable/libssl-doc/ASN1_ENUMERATED_get.3ssl.en.html Specifically.. ASN1_ENUMERATED_get() returns the value of a in a similar way to ASN1_INTEGER_get() but it returns 0xffffffffL if the value of a will not fit in a long type. New applications should use ASN1_ENUMERATED_get_int64() instead.
We should probably do the same I think.
OK, I think an ASN.1 ENUMERATED type has to be non-negative. Still investigating.
Yep: https://ldap.com/ldapv3-wire-protocol-reference-asn1-ber/ states: Enumerated Values ... "Enumerated elements should not have negative numeric values. However, values are still encoded using the two’s complement representation of the value, so that it may be necessary to add a leading byte in which all bits are set to zero if the binary representation of the numeric value would have otherwise caused the most significant bit in the first byte to be set to one."
Created attachment 15751 [details] Possible patch for master. Here's what I've got in CI right now. Let me know what you think !
Comment on attachment 15751 [details] Possible patch for master. Passes CI. Let me know if you want a merge request !
Ping - do you think this one works ?
(In reply to Jeremy Allison from comment #8) Yes, looks good. RB+!
Is this worth backporting?
Oh, we should have added "credit to OSS-Fuzz".