stricter parser for ipv4_from_asc #24438
Conversation
crypto/x509/v3_utl.c
Outdated
| /* get_ipv4_component consumes one IPv4 component, terminated by either '.' or | ||
| * the end of the string, from |*str|. On success, it returns one, sets |*out| |
There was a problem hiding this comment.
Please reformat the comments. Also there are C++ style comments in the tests.
crypto/x509/v3_utl.c
Outdated
| if (!get_ipv4_component(&v4[0], &in) || !get_ipv4_dot(&in) || | ||
| !get_ipv4_component(&v4[1], &in) || !get_ipv4_dot(&in) || | ||
| !get_ipv4_component(&v4[2], &in) || !get_ipv4_dot(&in) || | ||
| !get_ipv4_component(&v4[3], &in) || *in != '\0') { |
There was a problem hiding this comment.
This needs also code style reformat
test/x509_test.c
Outdated
| if (((rc == 0) && (test_samples[i].out[0] == '\0')) || | ||
| ((rc != 0) && (memcmp(buf, test_samples[i].out, BUF_LEN) == 0))) { |
There was a problem hiding this comment.
and here || should be on the beginning of a line
reject invalid IPv4 addresses in ipv4_from_asc The old scanf-based parser accepted all kinds of invalid inputs like: "1.2.3.4.5" "1.2.3.4 " "1.2.3. 4" " 1.2.3.4" "1.2.3.4." "1.2.3.+4" "1.2.3.4.example.test" "1.2.3.01" "1.2.3.0x1" Thanks to Amir Mohamadi for pointing this out.
|
new version of PR
|
| {"1.2", NULL, 0 }, | ||
| {"1.2.", NULL, 0 }, | ||
| {"1.2.3", NULL, 0 }, | ||
| {"1.2.3.", NULL, 0 }, |
There was a problem hiding this comment.
Some of those are valid IPv4 addresses:
- "1" is "0.0.0.1"
- "1.2" is "1.0.0.2" (and so 127.1 is 127.0.0.1)
- "1.2.3" is "1.2.0.3"
I'm not saying we should accept them, but they are valid and supported by almost all tools.
There was a problem hiding this comment.
This is a valid point. The current implementation in openssl 3.3 considers those IP addresses as invalid. I'm not sure if we should change the behavior in this PR. I think this needs some more thought. I prefer to keep behavior same. The code in question is part of x509 subsystem. The reason I want to keep 1.2 input refused is it may be source of confusion. Consider situation as follows:
- the address entered as 1.2 in ASCIIZ form
- it gets converted to 1.0.0.2 binary and encoded using ber/der
- converting the binary value back to ASCIIZ will give 1.0.0.2 which is different to what user entered.
There was a problem hiding this comment.
+1 to what @Sashan said. If OpenSSL wants to switch to accepting those syntaxes, I think that's best done separately. I wrote those tests because test coverage was generally lacking.
That said, what "is" a valid IPv4 address is very context-dependent. If you ask the IETF URL RFC, the answer is no, those variants are not valid.
But if you ask the WHATWG URL standard, those are allowed, but then you're also allowed to write components in hex or octal, which OpenSSL has also never supported.
If you ask RFC 952, cited by RFC 1123, you have to have four decimal numbers, so the answer is again no.
Given OpenSSL's previous behavior, the more narrow syntax seems to be the best path here.
reject invalid IPv4 addresses in ipv4_from_asc
The old scanf-based parser accepted all kinds of invalid inputs like: "1.2.3.4.5"
"1.2.3.4 "
"1.2.3. 4"
" 1.2.3.4"
"1.2.3.4."
"1.2.3.+4"
"1.2.3.4.example.test"
"1.2.3.01"
"1.2.3.0x1"
Thanks to Amir Mohamadi for pointing this out.
Checklist