netdev - Re: [PATCH iproute2 v2] ipaddress: strengthen check on 'label' input

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 14 Jun 2018 15:46:46 +0200
From:   Patrick Talbert <ptalbert@...hat.com>
To:     Stephen Hemminger <stephen@...workplumber.org>
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH iproute2 v2] ipaddress: strengthen check on 'label' input

On Fri, Jun 1, 2018 at 9:56 PM, Stephen Hemminger
<stephen@...workplumber.org> wrote:
> On Tue, 29 May 2018 16:57:07 +0200
> Patrick Talbert <ptalbert@...hat.com> wrote:
>
>> As mentioned in the ip-address man page, an address label must
>> be equal to the device name or prefixed by the device name
>> followed by a colon. Currently the only check on this input is
>> to see if the device name appears at the beginning of the label
>> string.
>>
>> This commit adds an additional check to ensure label == dev or
>> continues with a colon.
>>
>> Signed-off-by: Patrick Talbert <ptalbert@...hat.com>
>> Suggested-by: Stephen Hemminger <stephen@...workplumber.org>
>
> Yes, this looks better but still have some feedback.
>
>> ---
>>  ip/ipaddress.c | 21 +++++++++++++++++++--
>>  1 file changed, 19 insertions(+), 2 deletions(-)
>>
>> diff --git a/ip/ipaddress.c b/ip/ipaddress.c
>> index 00da14c..fce2008 100644
>> --- a/ip/ipaddress.c
>> +++ b/ip/ipaddress.c
>> @@ -2040,6 +2040,22 @@ static bool ipaddr_is_multicast(inet_prefix *a)
>>               return false;
>>  }
>>
>> +static bool is_valid_label(const char *dev, const char *label)
>> +{
>> +     char alias[strlen(dev) + 1];
>> +
>> +     if (strlen(label) < strlen(dev))
>> +             return false;
>> +
>> +     strcpy(alias, dev);
>> +     strcat(alias, ":");
>> +     if (strncmp(label, dev, strlen(dev)) == 0 ||
>> +         strncmp(label, alias, strlen(alias)) == 0)
>> +             return true;
>> +     else
>> +             return false;
>> +}
>
> This string copying and comparison still is much more overhead than it
> needs to be. The following tests out to be equivalent with a single strncmp
> and strlen.
>
> Why not just:
> diff --git a/ip/ipaddress.c b/ip/ipaddress.c
> index 00da14c6f97c..eac489e94fe4 100644
> --- a/ip/ipaddress.c
> +++ b/ip/ipaddress.c
> @@ -2040,6 +2040,16 @@ static bool ipaddr_is_multicast(inet_prefix *a)
>                 return false;
>  }
>
> +static bool is_valid_label(const char *label, const char *dev)
> +{
> +       size_t len = strlen(dev);
> +
> +       if (strncmp(label, dev, len) != 0)
> +               return false;
> +
> +       return label[len] == '\0' || label[len] == ':';
> +}
> +

Woah. This is way better. v3 coming up....

Thank you for all of your help with this... and by help I mean writing
the patch.

>
>
> Doesn't matter much now, but code seems to get copied.