[iproute2 1/2] ss: fix crash when skipping disabled header field

[iproute2 1/2] ss: fix crash when skipping disabled header field

[Antonio Quartulli]

When the first header field is disabled (i.e. when passing the -t
option), field_flush() is invoked with the `buffer` global variable
still zero'd.
However, in field_flush() we try to access buffer.cur->len
during variables initialization, thus leading to a SIGSEGV.

It's interesting to note that this bug appears only when the code
is compiled with -O0, because the compiler is smart
enough to immediately jump to the return statement if optimizations
are enabled and skip the faulty instruction.

Cc: Stefano Brivio <sbrivio@redhat.com>
Cc: Stephen Hemminger <stephen@networkplumber.org>
Signed-off-by: Antonio Quartulli <a@unstable.cc>
---
 misc/ss.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/misc/ss.c b/misc/ss.c
index 1abf43d0..b35859dc 100644
--- a/misc/ss.c
+++ b/misc/ss.c
@@ -1018,12 +1018,15 @@ static void print_right_spacing(struct column *f, int printed)
 /* Done with field: update buffer pointer, start new token after current one */
 static void field_flush(struct column *f)
 {
-	struct buf_chunk *chunk = buffer.tail;
-	unsigned int pad = buffer.cur->len % 2;
+	struct buf_chunk *chunk;
+	unsigned int pad;
 
 	if (f->disabled)
 		return;
 
+	chunk = buffer.tail;
+	pad = buffer.cur->len % 2;
+
 	if (buffer.cur->len > f->max_len)
 		f->max_len = buffer.cur->len;
 
-- 
2.15.1

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

[Antonio Quartulli]

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).
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@redhat.com>
Cc: Stephen Hemminger <stephen@networkplumber.org>
Signed-off-by: Antonio Quartulli <a@unstable.cc>
---

To crash ss, simply execute the following on a system using the old
socket format:
ss -x dst 192.168.1.1
or
ss dst 192.168.1.1

(crash reproduced on linux-4.12.12)


 misc/ss.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

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
+			 */
+			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) {
-- 
2.15.1

Re: [iproute2 1/2] ss: fix crash when skipping disabled header field

[Stefano Brivio]

On Sun,  7 Jan 2018 02:31:49 +0800
Antonio Quartulli <a@unstable.cc> wrote:

> When the first header field is disabled (i.e. when passing the -t
> option), field_flush() is invoked with the `buffer` global variable
> still zero'd.
> However, in field_flush() we try to access buffer.cur->len
> during variables initialization, thus leading to a SIGSEGV.

Good catch, thanks for fixing this.

> It's interesting to note that this bug appears only when the code
> is compiled with -O0, because the compiler is smart
> enough to immediately jump to the return statement if optimizations
> are enabled and skip the faulty instruction.

I should really have tested all the options with -O0 as well. :( Done
now.

> Cc: Stefano Brivio <sbrivio@redhat.com>
> Cc: Stephen Hemminger <stephen@networkplumber.org>
> Signed-off-by: Antonio Quartulli <a@unstable.cc>

Fixes: 691bd854bf4a ("ss: Buffer raw fields first, then render them as a table")
Reviewed-by: Stefano Brivio <sbrivio@redhat.com>

-- 
Stefano

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

[Stefano Brivio]

On Sun,  7 Jan 2018 02:31:50 +0800
Antonio Quartulli <a@unstable.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).

> 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@redhat.com>
> Cc: Stephen Hemminger <stephen@networkplumber.org>
> Signed-off-by: Antonio Quartulli <a@unstable.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.

> +			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@redhat.com>

-- 
Stefano

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

[Antonio Quartulli]

[-- Attachment #1.1: Type: text/plain, Size: 2160 bytes --]

Hi,

On 07/01/18 03:28, Stefano Brivio wrote:
> On Sun,  7 Jan 2018 02:31:50 +0800
> Antonio Quartulli <a@unstable.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@redhat.com>
>> Cc: Stephen Hemminger <stephen@networkplumber.org>
>> Signed-off-by: Antonio Quartulli <a@unstable.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@redhat.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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [iproute2 1/2] ss: fix crash when skipping disabled header field

[Stephen Hemminger]

On Sun,  7 Jan 2018 02:31:49 +0800
Antonio Quartulli <a@unstable.cc> wrote:

> When the first header field is disabled (i.e. when passing the -t
> option), field_flush() is invoked with the `buffer` global variable
> still zero'd.
> However, in field_flush() we try to access buffer.cur->len
> during variables initialization, thus leading to a SIGSEGV.
> 
> It's interesting to note that this bug appears only when the code
> is compiled with -O0, because the compiler is smart
> enough to immediately jump to the return statement if optimizations
> are enabled and skip the faulty instruction.
> 
> Cc: Stefano Brivio <sbrivio@redhat.com>
> Cc: Stephen Hemminger <stephen@networkplumber.org>
> Signed-off-by: Antonio Quartulli <a@unstable.cc>

Both applied, thanks.