asn1parse: Reject some invalid tags #7697
Conversation
ASN.1 requires tags to be greater than zero, and multi-byte tags are not supported by this library. Check that tags taken from input data are valid. Tags provided by the caller are assumed valid. Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com>
bensze01
added
bug
needs-work
needs-review
|
Please rebase on top of development to fix the CI issues (See #7699), and provide a non-regression test. |
gilles-peskine-arm
left a comment
gilles-peskine-arm
left a comment
There was a problem hiding this comment.
This is a very minor robustness improvement and I'm not convinced it's worth the trouble.
If it is worth it, we need some non-regression tests.
It's not really a bug fix, only a robustness improvement so it doesn't need a changelog entry or a backport.
| return 0; | ||
| } | ||
|
|
||
| static int mbedtls_asn1_bad_tag(int tag) |
There was a problem hiding this comment.
In this library, as you note, a tag is always a single byte.
| static int mbedtls_asn1_bad_tag(int tag) | |
| static int mbedtls_asn1_bad_tag(unsigned char tag) |
|
|
||
| static int mbedtls_asn1_bad_tag(int tag) | ||
| { | ||
| return tag < 1 || (tag & ~0xE0) >= 0x1F; |
There was a problem hiding this comment.
tag is always ≤255, so (tag & ~0xE0) >= 0x1F can't happen. tag is always ≥0 so tag < 1 is equivalent to tag == 0.
| return tag < 1 || (tag & ~0xE0) >= 0x1F; | |
| return tag == 0; |
There was a problem hiding this comment.
(tag & ~0xE0) == 0x1F can indeed happen (consider tag == 0xFF). Also tag < 1 should be (tag & 0xDF) == 0.
There was a problem hiding this comment.
Ah, my bad, I'd reasoned as if it was > 0x1F rather than >= 0x1F. It would indeed have been more readable with ==. And please be consistent with 0xDF vs 0xE0. In general, please avoid negation when not needed, especially if it leads to double negation. A is_good_tag function is likely to make the code more readable than is_bad_tag.
| while (*p < end) { | ||
| unsigned char const tag = *(*p)++; | ||
|
|
||
| if (mbedtls_asn1_bad_tag(tag)) { |
There was a problem hiding this comment.
I don't understand why we'd specifically reject 0 here, but accept all other tags. The caller will need to filter tags anyway.
I'm not sure why mbedtls_asn1_traverse_sequence_of is a public function anyway. (I approved it in ARMmbed/mbed-crypto#263, but now I don't know why. Maybe because it's unit-tested and at the time we couldn't unit-test private functions? But unit tests for mbedtls_asn1_get_sequence_of would have sufficed. Or maybe because it was needed for future X.509 work that we never finished?) Inside the library, it's only used by mbedtls_asn1_get_sequence_of which doesn't need all this flexibility, in particular tag will be constrained to one specific value. So I don't think we need to make this function more robust.
| params->tag = **p; | ||
| (*p)++; | ||
|
|
||
| if (mbedtls_asn1_bad_tag(params->tag)) { |
There was a problem hiding this comment.
This is only useful if the caller doesn't subsequently check the tag, which seems unlikely. If the caller doesn't check the tag, then with this patch we'll reject a guaranteed-invalid tag but not a potentially valid tag that is not understood, so the benefit is very limited.
gilles-peskine-arm
added
enhancement
size-xs
3 participants
Description
ASN.1 requires tags to be greater than zero, and multi-byte tags are not supported by this library. Check that tags taken from input data are valid. Tags provided by the caller are assumed valid.
PR checklist
Please tick as appropriate and edit the reasons (e.g.: "backport: not needed because this is a new feature")
Notes for the submitter
Please refer to the contributing guidelines, especially the
checklist for PR contributors.