[U-Boot] [PATCH] lib: net_utils: make string_to_ip stricter

[U-Boot] [PATCH] lib: net_utils: make string_to_ip stricter

[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 <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);
-- 
2.11.0.24.ge6920cf

[U-Boot] [PATCH] lib: net_utils: make string_to_ip stricter

[Simon Glass]

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;
> +               else
> +                       break;

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

[U-Boot] [PATCH] lib: net_utils: make string_to_ip stricter

[Chris Packham]

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;
>> +               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.

> 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

[U-Boot] [PATCH v2 1/2] lib: net_utils: make string_to_ip stricter

[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 <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) {
-- 
2.11.0.24.ge6920cf

[U-Boot] [PATCH v2 2/2] lib: net_utils: enforce '.' as octet separator in string_to_ip

[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 <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) {
-- 
2.11.0.24.ge6920cf

[U-Boot] [PATCH v2 2/2] lib: net_utils: enforce '.' as octet separator in string_to_ip

[Wolfgang Denk]

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

-- 
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_

[U-Boot] [PATCH v2 2/2] lib: net_utils: enforce '.' as octet separator in string_to_ip

[Chris Packham]

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.

[U-Boot] [PATCH v2 2/2] lib: net_utils: enforce '.' as octet separator in string_to_ip

[Wolfgang Denk]

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

-- 
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!

[U-Boot] [PATCH v2 2/2] lib: net_utils: enforce '.' as octet separator in string_to_ip

[Chris Packham]

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.

[U-Boot] [U-Boot, v2, 2/2] lib: net_utils: enforce '.' as octet separator in string_to_ip

[Tom Rini]

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!

-- 
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>

[U-Boot] [PATCH v2 1/2] lib: net_utils: make string_to_ip stricter

[Wolfgang Denk]

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

-- 
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.

[U-Boot] [PATCH v2 1/2] lib: net_utils: make string_to_ip stricter

[Chris Packham]

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.

[U-Boot] [U-Boot, v2, 1/2] lib: net_utils: make string_to_ip stricter

[Tom Rini]

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!

-- 
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>