netdev - Re: [iproute2 2/2] ss: fix NULL pointer access when parsing unix sockets with oldformat

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] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 7 Jan 2018 03:51:36 +0800
From:   Antonio Quartulli <a@...table.cc>
To:     Stefano Brivio <sbrivio@...hat.com>
Cc:     netdev@...r.kernel.org,
        Stephen Hemminger <stephen@...workplumber.org>,
        Phil Sutter <phil@....cc>
Subject: Re: [iproute2 2/2] ss: fix NULL pointer access when parsing unix
 sockets with oldformat

Hi,

On 07/01/18 03:28, Stefano Brivio wrote:
> On Sun,  7 Jan 2018 02:31:50 +0800
> Antonio Quartulli <a@...table.cc> wrote:
> 
>> When parsing and printing the unix sockets in unix_show(),
>> if the oldformat is detected, the peer_name member of the sockstat
>> object is left uninitialized (NULL).
> 
> Luckily, it is initialized. I'd rather say:
> 
> 	[...]
> 	object is not set (NULL).

ops, you are right! "is not set" makes more sense.

> 
>> For this reason, if a filter has been specified on the command line,
>> a strcmp() will crash when trying to access it.
>>
>> Avoid crash by checking that peer_name is not NULL before
>> passing it to strcmp().
>>
>> Cc: Stefano Brivio <sbrivio@...hat.com>
>> Cc: Stephen Hemminger <stephen@...workplumber.org>
>> Signed-off-by: Antonio Quartulli <a@...table.cc>
> 
> Fixes: 2d0e538f3e1c ("ss: Drop list traversal from unix_stats_print()")
> 
>> [...]
>>
>> diff --git a/misc/ss.c b/misc/ss.c
>> index b35859dc..29a25070 100644
>> --- a/misc/ss.c
>> +++ b/misc/ss.c
>> @@ -3711,7 +3711,10 @@ static int unix_show(struct filter *f)
>>  			};
>>  
>>  			memcpy(st.local.data, &u->name, sizeof(u->name));
>> -			if (strcmp(u->peer_name, "*"))
>> +			/* when parsing the old format rport is set to 0 and
>> +			 * therefore peer_name remains NULL
>> +			 */
> 
> Maybe this comment is a bit redundant, but I don't have a strong
> preference either.
> 

I thought explaining "why" it could be NULL would help the casual reader
understand why we do check it.
But now that we have the check, I think it's quick to understand why we
need it.

I'd leave to whoever is going to merge the match to decide to keep or
not the comment.

>> +			if (u->peer_name && strcmp(u->peer_name, "*"))
>>  				memcpy(st.remote.data, &u->peer_name,
>>  				       sizeof(u->peer_name));
>>  			if (run_ssfilter(f->f, &st) == 0) {
> 
> FWIW:
> Reviewed-by: Stefano Brivio <sbrivio@...hat.com>
> 


Thanks a lot, Stefano!

Should I send v2 with the commit message changed? Or can we leave this
to who will merge the change?


Regards,

-- 
Antonio Quartulli



Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)