ss: fix crash when skipping disabled header field
Subsystems (2)
PATCH 1/2 ss: fix crash when skipping disabled header field
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.
--- misc/ss.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
misc/ss.c +5 -2
@@ -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;
Signature · 1 line
2.15.1
Quoted 5 lines · On Sun, 7 Jan 2018 02:31:49 +0800 Antonio Quartulli <[email protected]> wrote:
When the first header field is disabled (i.e. when passing the -t option),
field_flush()is invoked with thebufferglobal variable still zero'd.
However, infield_flush()we try to accessbuffer.cur->lenduring variables initialization, thus leading to a SIGSEGV.
Good catch, thanks for fixing this.
Quoted 4 lines
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.
Quoted 3 lines
Cc Stefano Brivio <[email protected]>Cc Stephen Hemminger <[email protected]>Signed-off-by Antonio Quartulli <[email protected]>
Signature · 1 line
Stefano
PATCH 2/2 ss: fix NULL pointer access when parsing unix sockets with oldformat
In reply to Antonio QuartulliWhen 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().
---
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(-)
misc/ss.c +4 -1
@@ -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 ) {
Signature · 1 line
2.15.1
Re: [iproute2 2/2] ss: fix NULL pointer access when parsing unix sockets with oldformat
Quoted 3 lines · On Sun, 7 Jan 2018 02:31:50 +0800 Antonio Quartulli <[email protected]> wrote:
When parsing and printing the unix sockets in
unix_show(), if the oldformat is detected, thepeer_namemember of the sockstat object is left uninitialized (NULL).
Luckily, it is initialized. I'd rather say:
[...] object is not set (NULL).
Quoted 9 lines
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_nameis not NULL before passing it tostrcmp().Cc Stefano Brivio <[email protected]>Cc Stephen Hemminger <[email protected]>Signed-off-by Antonio Quartulli <[email protected]>
Quoted 7 lines
[...]
misc/ss.c +0
-0
misc/ss.c static int unix_show(struct filter *f) · line 3716
Quoted 7 lines
misc/ss.c +3
-1@@ -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.
misc/ss.c static int unix_show(struct filter *f) · line 3720
Quoted 4 lines
misc/ss.c +1
-0+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:
Signature · 1 line
Stefano
Re: [iproute2 2/2] ss: fix NULL pointer access when parsing unix sockets with oldformat
Hi,
Quoted 11 lines · Stefano Brivio
Quoted 3 lines · On Sun, 7 Jan 2018 02:31:50 +0800 Antonio Quartulli <[email protected]> wrote:
When parsing and printing the unix sockets in
unix_show(), if the oldformat is detected, thepeer_namemember 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.
Quoted 22 lines
Quoted 10 lines
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_nameis not NULL before passing it tostrcmp().Cc Stefano Brivio <[email protected]>Cc Stephen Hemminger <[email protected]>Signed-off-by Antonio Quartulli <[email protected]>Fixes 2d0e538f3e1c ("ss: Drop list traversal from unix_stats_print()")Quoted 9 lines
[...]
misc/ss.c +0
-0@@ -3711,7 +3711,10 @@ static int unix_show(struct filter *f) };
Quoted 9 lines
misc/ss.c static int unix_show(struct filter *f) · line 3716
Quoted 5 lines
misc/ss.c +3
-1memcpy (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.
Quoted 8 lines
misc/ss.c static int unix_show(struct filter *f) · line 3720
Quoted 4 lines
misc/ss.c +1
-0+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 <[email protected]>
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,
Signature · 1 line
Antonio Quartulli
Quoted 6 lines · On Sun, 7 Jan 2018 02:31:49 +0800 Antonio Quartulli <[email protected]> wrote:
When the first header field is disabled (i.e. when passing the -t option),
field_flush()is invoked with thebufferglobal variable still zero'd.
However, infield_flush()we try to accessbuffer.cur->lenduring variables initialization, thus leading to a SIGSEGV.
Quoted 8 lines
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 <[email protected]>Cc Stephen Hemminger <[email protected]>Signed-off-by Antonio Quartulli <[email protected]>
Both applied, thanks.