[U-Boot] [PATCH] lib: net_utils: make string_to_ip stricter
Previously values greater than 255 were implicitly truncated. Add some stricter checking to reject addresses with components >255.
With the input "1234192.168.1.1" the old behaviour would truncate the address to 192.168.1.1. New behaviour rejects the string outright and returns 0.0.0.0, which for the purposes of IP addresses can be considered an error.
Signed-off-by: Chris Packham judge.packham@gmail.com --- This was part of my long running IPv6 patchset (which I promise I'll get back to someday). But I feel this stands on it's own merits.
lib/net_utils.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/lib/net_utils.c b/lib/net_utils.c index cfae84275241..f148b8a70a7d 100644 --- a/lib/net_utils.c +++ b/lib/net_utils.c @@ -24,11 +24,16 @@ struct in_addr string_to_ip(const char *s)
for (addr.s_addr = 0, i = 0; i < 4; ++i) { ulong val = s ? simple_strtoul(s, &e, 10) : 0; + if (val > 255) { + addr.s_addr = 0; + return addr; + } addr.s_addr <<= 8; addr.s_addr |= (val & 0xFF); - if (s) { - s = (*e) ? e+1 : e; - } + if (*e == '.') + s = e + 1; + else + break; }
addr.s_addr = htonl(addr.s_addr);
Hi Chris,
On 20 December 2016 at 11:01, Chris Packham judge.packham@gmail.com wrote:
Previously values greater than 255 were implicitly truncated. Add some stricter checking to reject addresses with components >255.
With the input "1234192.168.1.1" the old behaviour would truncate the address to 192.168.1.1. New behaviour rejects the string outright and returns 0.0.0.0, which for the purposes of IP addresses can be considered an error.
Signed-off-by: Chris Packham judge.packham@gmail.com
This was part of my long running IPv6 patchset (which I promise I'll get back to someday). But I feel this stands on it's own merits.
lib/net_utils.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/lib/net_utils.c b/lib/net_utils.c index cfae84275241..f148b8a70a7d 100644 --- a/lib/net_utils.c +++ b/lib/net_utils.c @@ -24,11 +24,16 @@ struct in_addr string_to_ip(const char *s)
for (addr.s_addr = 0, i = 0; i < 4; ++i) { ulong val = s ? simple_strtoul(s, &e, 10) : 0;
if (val > 255) {addr.s_addr = 0;return addr;} addr.s_addr <<= 8; addr.s_addr |= (val & 0xFF);
if (s) {s = (*e) ? e+1 : e;}
if (*e == '.')s = e + 1;elsebreak;
This change seems to be unrelated. Should it be a separate commit? Also, what happens with '192.168.4' with this change?
} addr.s_addr = htonl(addr.s_addr);-- 2.11.0.24.ge6920cf
Regards, Simon
On Mon, Dec 26, 2016 at 6:23 PM, Simon Glass sjg@chromium.org wrote:
Hi Chris,
On 20 December 2016 at 11:01, Chris Packham judge.packham@gmail.com wrote:
Previously values greater than 255 were implicitly truncated. Add some stricter checking to reject addresses with components >255.
With the input "1234192.168.1.1" the old behaviour would truncate the address to 192.168.1.1. New behaviour rejects the string outright and returns 0.0.0.0, which for the purposes of IP addresses can be considered an error.
Signed-off-by: Chris Packham judge.packham@gmail.com
This was part of my long running IPv6 patchset (which I promise I'll get back to someday). But I feel this stands on it's own merits.
lib/net_utils.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/lib/net_utils.c b/lib/net_utils.c index cfae84275241..f148b8a70a7d 100644 --- a/lib/net_utils.c +++ b/lib/net_utils.c @@ -24,11 +24,16 @@ struct in_addr string_to_ip(const char *s)
for (addr.s_addr = 0, i = 0; i < 4; ++i) { ulong val = s ? simple_strtoul(s, &e, 10) : 0;
if (val > 255) {addr.s_addr = 0;return addr;} addr.s_addr <<= 8; addr.s_addr |= (val & 0xFF);
if (s) {s = (*e) ? e+1 : e;}
if (*e == '.')s = e + 1;elsebreak;This change seems to be unrelated. Should it be a separate commit?
I should at least mention it's purpose in the commit message. It ensures that '.' is used as a separator and not some other arbitrary ascii character.
Also, what happens with '192.168.4' with this change?
Good point. I think it would be parsed as 0.192.168.4 which is clearly wrong. The else should probably set s_addr to 0 to flag the error.
} addr.s_addr = htonl(addr.s_addr);-- 2.11.0.24.ge6920cf
Regards, Simon
Previously values greater than 255 were implicitly truncated. Add some stricter checking to reject addresses with components >255.
With the input "1234192.168.1.1" the old behaviour would truncate the address to 192.168.1.1. New behaviour rejects the string outright and returns 0.0.0.0, which for the purposes of IP addresses can be considered an error.
Signed-off-by: Chris Packham judge.packham@gmail.com --- This was part of my long running IPv6 patchset (which I promise I'll get back to someday). But I feel this stands on it's own merits.
Changes in v2: - split into 2 patches
lib/net_utils.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/lib/net_utils.c b/lib/net_utils.c index cfae84275241..8f81e7801033 100644 --- a/lib/net_utils.c +++ b/lib/net_utils.c @@ -24,6 +24,10 @@ struct in_addr string_to_ip(const char *s)
for (addr.s_addr = 0, i = 0; i < 4; ++i) { ulong val = s ? simple_strtoul(s, &e, 10) : 0; + if (val > 255) { + addr.s_addr = 0; + return addr; + } addr.s_addr <<= 8; addr.s_addr |= (val & 0xFF); if (s) {
Ensure '.' is used to separate octets. If another character is seen reject the string outright and return 0.0.0.0.
Signed-off-by: Chris Packham judge.packham@gmail.com ---
Changes in v2: - new END
lib/net_utils.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/lib/net_utils.c b/lib/net_utils.c index 8f81e7801033..d06be22849fb 100644 --- a/lib/net_utils.c +++ b/lib/net_utils.c @@ -28,6 +28,10 @@ struct in_addr string_to_ip(const char *s) addr.s_addr = 0; return addr; } + if (i != 3 && *e != '.') { + addr.s_addr = 0; + return addr; + } addr.s_addr <<= 8; addr.s_addr |= (val & 0xFF); if (s) {
Dear Chris Packham,
In message 20170104003626.4211-2-judge.packham@gmail.com you wrote:
Ensure '.' is used to separate octets. If another character is seen reject the string outright and return 0.0.0.0.
What is this good for?
The old code was forgiving and would accept 192,168,1,2 as well. Do we need to be so strict here - at the cost of added code size?
Also, at least crtitical parts of the network code (NFS, TFTP) do not check the return value of string_to_ip() - so what is the benefit of this change?
Best regards,
Wolfgang Denk
On Wed, Jan 4, 2017 at 11:09 PM, Wolfgang Denk wd@denx.de wrote:
Dear Chris Packham,
In message 20170104003626.4211-2-judge.packham@gmail.com you wrote:
Ensure '.' is used to separate octets. If another character is seen reject the string outright and return 0.0.0.0.
What is this good for?
The old code was forgiving and would accept 192,168,1,2 as well.
Technically you can't enter that. The env_flags.c code prevents that from being added to environment variables that have been tagged as ip addresses. These patches are pushing the logic down a bit further. The code handling env_flags_vartype_ipaddr could be updated to use string_to_ip instead.
Do we need to be so strict here - at the cost of added code size?
Also, at least crtitical parts of the network code (NFS, TFTP) do not check the return value of string_to_ip() - so what is the benefit of this change?
The reasoning behind this change is to prepare for parsing ipv6 addresses, which can contain ipv4 format addresses provided they are at the end.
e.g. This is a valid ipv6 address "::ffff:192.168.1.1" and so is "::ffff:0192:0168:0001:0001" but the former triggers the ipv4 mapping logic.
Dear Chris,
In message CAFOYHZDK+cOs-1ODjSCBF7OTERTzMgBgBhRC+WSRG4C5iToB+A@mail.gmail.com you wrote:
The old code was forgiving and would accept 192,168,1,2 as well.
Technically you can't enter that. The env_flags.c code prevents that from being added to environment variables that have been tagged as ip addresses. These patches are pushing the logic down a bit further. The code handling env_flags_vartype_ipaddr could be updated to use string_to_ip instead.
I'm not 100% sure about that. U-Boot environment is a complex thing. For example, what happens if we use "env import" to import variable settings from text or binary formats? Are all these checks present then, too? [Sorry for asking, I really don't know.]
Also, at least crtitical parts of the network code (NFS, TFTP) do not check the return value of string_to_ip() - so what is the benefit of this change?
The reasoning behind this change is to prepare for parsing ipv6 addresses, which can contain ipv4 format addresses provided they are at the end.
e.g. This is a valid ipv6 address "::ffff:192.168.1.1" and so is "::ffff:0192:0168:0001:0001" but the former triggers the ipv4 mapping logic.
Ah, I see. Makes sense.
Should we add error handling to TFTP and NFS, then?
Best regards,
Wolfgang Denk
On Mon, Jan 9, 2017 at 7:45 AM, Wolfgang Denk wd@denx.de wrote:
Dear Chris,
In message CAFOYHZDK+cOs-1ODjSCBF7OTERTzMgBgBhRC+WSRG4C5iToB+A@mail.gmail.com you wrote:
The old code was forgiving and would accept 192,168,1,2 as well.
Technically you can't enter that. The env_flags.c code prevents that from being added to environment variables that have been tagged as ip addresses. These patches are pushing the logic down a bit further. The code handling env_flags_vartype_ipaddr could be updated to use string_to_ip instead.
I'm not 100% sure about that. U-Boot environment is a complex thing. For example, what happens if we use "env import" to import variable settings from text or binary formats? Are all these checks present then, too? [Sorry for asking, I really don't know.]
I can say for sure either. What I can say is that some checking already exists so technically this is duplicating functionality. The change to actually replace one with the other would need to consider these cases that you've raise.
Also, at least crtitical parts of the network code (NFS, TFTP) do not check the return value of string_to_ip() - so what is the benefit of this change?
The reasoning behind this change is to prepare for parsing ipv6 addresses, which can contain ipv4 format addresses provided they are at the end.
e.g. This is a valid ipv6 address "::ffff:192.168.1.1" and so is "::ffff:0192:0168:0001:0001" but the former triggers the ipv4 mapping logic.
Ah, I see. Makes sense.
Should we add error handling to TFTP and NFS, then?
I think the "checking" I'm referring to in net_check_prereq() seems to cover the bases for now. If we want to distinguish between invalid, set to 0.0.0.0 and not set then we'd need to start adding additional error handling.
On Wed, Jan 04, 2017 at 01:36:26PM +1300, Chris Packham wrote:
Ensure '.' is used to separate octets. If another character is seen reject the string outright and return 0.0.0.0.
Signed-off-by: Chris Packham judge.packham@gmail.com
Applied to u-boot/master, thanks!
Dear Chris,
In message 20170104003626.4211-1-judge.packham@gmail.com you wrote:
With the input "1234192.168.1.1" the old behaviour would truncate the address to 192.168.1.1. New behaviour rejects the string outright and returns 0.0.0.0, which for the purposes of IP addresses can be considered an error.
Does code that calls string_to_ip() check for such an error condition?
Best regards,
Wolfgang Denk
Hi Wolfgang,
On Wed, Jan 4, 2017 at 11:04 PM, Wolfgang Denk wd@denx.de wrote:
Dear Chris,
In message 20170104003626.4211-1-judge.packham@gmail.com you wrote:
With the input "1234192.168.1.1" the old behaviour would truncate the address to 192.168.1.1. New behaviour rejects the string outright and returns 0.0.0.0, which for the purposes of IP addresses can be considered an error.
Does code that calls string_to_ip() check for such an error condition?
It does by virtue of the fact that it will consider the global variables for the various ip addresses unset and complain.
On Wed, Jan 04, 2017 at 01:36:25PM +1300, Chris Packham wrote:
Previously values greater than 255 were implicitly truncated. Add some stricter checking to reject addresses with components >255.
With the input "1234192.168.1.1" the old behaviour would truncate the address to 192.168.1.1. New behaviour rejects the string outright and returns 0.0.0.0, which for the purposes of IP addresses can be considered an error.
Signed-off-by: Chris Packham judge.packham@gmail.com
Applied to u-boot/master, thanks!
participants (4)
-
Chris Packham -
Simon Glass -
Tom Rini -
Wolfgang Denk