lib: net_utils: make string_to_ip stricter
Subsystems (2)
PATCH 1/1 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.
---
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(-)
lib/net_utils.c +8 -3
@@ -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);
Signature · 1 line
2.11.0.24.ge6920cf
PATCH 1/1 lib: net_utils: make string_to_ip stricter
Hi Chris,
Quoted 27 lines · Chris Packham
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 <[email protected]>---
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(-)lib/net_utils.c +3
-0@@ -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;
lib/net_utils.c struct in_addr string_to_ip(const char *s) · line 36
Quoted 10 lines
lib/net_utils.c +5
-3+ } addr.s_addr <<=8 ; addr.s_addr |= (val &0xFF ); -if (s) { - s = (*e) ? e+1 : e; - } +if (*e =='.' ) + s = e +1 ; +else +break ;
This change seems to be unrelated. Should it be a separate commit? Also, what happens with '192.168.4' with this change?
Quoted 6 lines
lib/net_utils.c +0
-1} addr.s_addr =htonl (addr.s_addr); --2.11.0.24.ge6920cf
Regards,
Simon
PATCH 1/1 lib: net_utils: make string_to_ip stricter
Quoted 34 lines · Simon Glass
Hi Chris,
Quoted 31 lines · Chris Packham
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 <[email protected]>---
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(-)lib/net_utils.c +4
-1@@ -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) {
Quoted 8 lines
lib/net_utils.c struct in_addr string_to_ip(const char *s) · line 36
Quoted 6 lines
lib/net_utils.c +4
-2- s = (*e) ? e+1 : e; - } +if (*e =='.' ) + s = e +1 ; +else +break ;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.
Quoted 2 lines
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.
Quoted 9 lines
Quoted 6 lines
lib/net_utils.c +0
-1} addr.s_addr =htonl (addr.s_addr); --2.11.0.24.ge6920cf
Regards,
Simon
PATCH v2 1/2 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.
---
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(+)
lib/net_utils.c +4 -0
@@ -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) {
Signature · 1 line
2.11.0.24.ge6920cf
PATCH v2 2/2 lib: net_utils: enforce '.' as octet separator in string_to_ip
Ensure '.' is used to separate octets. If another character is seen reject the string outright and return 0.0.0.0.
---
Changes in v2:
- new
END
lib/net_utils.c | 4 ++++ 1 file changed, 4 insertions(+)
lib/net_utils.c +4 -0
@@ -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) {
Signature · 1 line
2.11.0.24.ge6920cf
PATCH v2 2/2 lib: net_utils: enforce '.' as octet separator in string_to_ip
Dear Chris Packham,
Quoted 2 lines · In message <[email protected]> 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
Signature · 5 lines
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de People are always a lot more complicated than you think. It's very important to remember that. - Terry Pratchett, _Truckers_
PATCH v2 2/2 lib: net_utils: enforce '.' as octet separator in string_to_ip
Quoted 9 lines · Wolfgang Denk
Dear Chris Packham,
Quoted 2 lines · In message <[email protected]> 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.
Quoted 6 lines
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.
PATCH v2 2/2 lib: net_utils: enforce '.' as octet separator in string_to_ip
Dear Chris,
Quoted 8 lines · In message <CAFOYHZDK+cOs-1ODjSCBF7OTERTzMgBgBhRC+WSRG4C5iToB+A@mail.gmail.com>…
Quoted 2 lines
The old code was forgiving and would accept 192,168,1,2 as well.
Technically you can't enter that. The
env_flags.ccode 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 handlingenv_flags_vartype_ipaddrcould be updated to usestring_to_ipinstead.
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.]
Quoted 5 lines
Quoted 4 lines
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?
Quoted 7 lines
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
Signature · 4 lines
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de The world is coming to an end -- save your buffers!
PATCH v2 2/2 lib: net_utils: enforce '.' as octet separator in string_to_ip
Quoted 12 lines · Wolfgang Denk
Dear Chris,
Quoted 8 lines · In message <CAFOYHZDK+cOs-1ODjSCBF7OTERTzMgBgBhRC+WSRG4C5iToB+A@mail.gmail.com>…
Quoted 2 lines
The old code was forgiving and would accept 192,168,1,2 as well.
Technically you can't enter that. The
env_flags.ccode 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 handlingenv_flags_vartype_ipaddrcould be updated to usestring_to_ipinstead.
Quoted 5 lines
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.
Quoted 9 lines
Quoted 9 lines
Quoted 4 lines
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.
Quoted 8 lines
Quoted 3 lines
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.
PATCH v2 2/2 lib: net_utils: enforce '.' as octet separator in string_to_ip
In reply to Chris PackhamQuoted 4 lines · Chris Packham
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 <[email protected]>
Applied to u-boot/master, thanks!
Signature · 8 lines
Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170115/f6cba96f/attachment.sig>
PATCH v2 1/2 lib: net_utils: make string_to_ip stricter
In reply to Chris PackhamDear Chris,
Quoted 5 lines · In message <[email protected]> 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
Signature · 4 lines
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de Listen to the local yokel yodel.
PATCH v2 1/2 lib: net_utils: make string_to_ip stricter
Hi Wolfgang,
Quoted 4 lines · Wolfgang Denk
Dear Chris,
Quoted 1 line · In message <[email protected]> you wrote:
Quoted 8 lines
Quoted 4 lines
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.
PATCH v2 1/2 lib: net_utils: make string_to_ip stricter
In reply to Chris PackhamQuoted 9 lines · Chris Packham
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 <[email protected]>
Applied to u-boot/master, thanks!
Signature · 8 lines
Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170115/7a1ae904/attachment.sig>