* [Qemu-devel] [PATCH] slirp: check sscanf result when emulating ident
@ 2019-03-01 21:45 William Bowling
2019-03-02 2:21 ` Samuel Thibault
2019-03-02 17:42 ` Philippe Mathieu-Daudé
0 siblings, 2 replies; 5+ messages in thread
From: William Bowling @ 2019-03-01 21:45 UTC (permalink / raw)
To: qemu-devel; +Cc: secalert, William Bowling
When emulating ident in tcp_emu, if the strchr checks passed but the
sscanf check failed, two uninitialized variables would be copied and
sent in the reply.
Signed-off-by: William Bowling <will@wbowling.info>
---
slirp/tcp_subr.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/slirp/tcp_subr.c b/slirp/tcp_subr.c
index 262a42d6c8..73a160ba16 100644
--- a/slirp/tcp_subr.c
+++ b/slirp/tcp_subr.c
@@ -664,12 +664,12 @@ tcp_emu(struct socket *so, struct mbuf *m)
break;
}
}
- }
- so_rcv->sb_cc = snprintf(so_rcv->sb_data,
- so_rcv->sb_datalen,
- "%d,%d\r\n", n1, n2);
- so_rcv->sb_rptr = so_rcv->sb_data;
- so_rcv->sb_wptr = so_rcv->sb_data + so_rcv->sb_cc;
+ so_rcv->sb_cc = snprintf(so_rcv->sb_data,
+ so_rcv->sb_datalen,
+ "%d,%d\r\n", n1, n2);
+ so_rcv->sb_rptr = so_rcv->sb_data;
+ so_rcv->sb_wptr = so_rcv->sb_data + so_rcv->sb_cc;
+ }
}
m_free(m);
return 0;
--
2.15.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] slirp: check sscanf result when emulating ident
2019-03-01 21:45 [Qemu-devel] [PATCH] slirp: check sscanf result when emulating ident William Bowling
@ 2019-03-02 2:21 ` Samuel Thibault
2019-03-02 17:42 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 5+ messages in thread
From: Samuel Thibault @ 2019-03-02 2:21 UTC (permalink / raw)
To: William Bowling; +Cc: qemu-devel, secalert
William Bowling, le ven. 01 mars 2019 21:45:56 +0000, a ecrit:
> When emulating ident in tcp_emu, if the strchr checks passed but the
> sscanf check failed, two uninitialized variables would be copied and
> sent in the reply.
>
> Signed-off-by: William Bowling <will@wbowling.info>
Applied to my tree, thanks!
> ---
> slirp/tcp_subr.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/slirp/tcp_subr.c b/slirp/tcp_subr.c
> index 262a42d6c8..73a160ba16 100644
> --- a/slirp/tcp_subr.c
> +++ b/slirp/tcp_subr.c
> @@ -664,12 +664,12 @@ tcp_emu(struct socket *so, struct mbuf *m)
> break;
> }
> }
> - }
> - so_rcv->sb_cc = snprintf(so_rcv->sb_data,
> - so_rcv->sb_datalen,
> - "%d,%d\r\n", n1, n2);
> - so_rcv->sb_rptr = so_rcv->sb_data;
> - so_rcv->sb_wptr = so_rcv->sb_data + so_rcv->sb_cc;
> + so_rcv->sb_cc = snprintf(so_rcv->sb_data,
> + so_rcv->sb_datalen,
> + "%d,%d\r\n", n1, n2);
> + so_rcv->sb_rptr = so_rcv->sb_data;
> + so_rcv->sb_wptr = so_rcv->sb_data + so_rcv->sb_cc;
> + }
> }
> m_free(m);
> return 0;
> --
> 2.15.1
>
>
--
Samuel
What's this script do?
unzip ; touch ; finger ; mount ; gasp ; yes ; umount ; sleep
Hint for the answer: not everything is computer-oriented. Sometimes you're
in a sleeping bag, camping out.
(Contributed by Frans van der Zande.)
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] slirp: check sscanf result when emulating ident
2019-03-01 21:45 [Qemu-devel] [PATCH] slirp: check sscanf result when emulating ident William Bowling
2019-03-02 2:21 ` Samuel Thibault
@ 2019-03-02 17:42 ` Philippe Mathieu-Daudé
2019-03-02 17:49 ` Samuel Thibault
2019-03-02 23:28 ` William Bowling
1 sibling, 2 replies; 5+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-03-02 17:42 UTC (permalink / raw)
To: William Bowling, qemu-devel; +Cc: secalert, qemu-stable
Hi William, Samuel,
On 3/1/19 10:45 PM, William Bowling wrote:
> When emulating ident in tcp_emu, if the strchr checks passed but the
> sscanf check failed, two uninitialized variables would be copied and
> sent in the reply.
William: How did you notice that? Using a static analyzer?
Samuel: since this diff is not obvious without looking at the context
(also due to the code re-indent), can you improve the commit
description, such (or better):
"Move this code inside the if(sscanf()) clause".
We have a data leak, Cc'ing qemu-stable.
(Adding the address I noticed you Cc'ed secalert@redhat.com, so that
confirms my guess).
>
> Signed-off-by: William Bowling <will@wbowling.info>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Thanks,
Phil.
> ---
> slirp/tcp_subr.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/slirp/tcp_subr.c b/slirp/tcp_subr.c
> index 262a42d6c8..73a160ba16 100644
> --- a/slirp/tcp_subr.c
> +++ b/slirp/tcp_subr.c
> @@ -664,12 +664,12 @@ tcp_emu(struct socket *so, struct mbuf *m)
> break;
> }
> }
> - }
> - so_rcv->sb_cc = snprintf(so_rcv->sb_data,
> - so_rcv->sb_datalen,
> - "%d,%d\r\n", n1, n2);
> - so_rcv->sb_rptr = so_rcv->sb_data;
> - so_rcv->sb_wptr = so_rcv->sb_data + so_rcv->sb_cc;
> + so_rcv->sb_cc = snprintf(so_rcv->sb_data,
> + so_rcv->sb_datalen,
> + "%d,%d\r\n", n1, n2);
> + so_rcv->sb_rptr = so_rcv->sb_data;
> + so_rcv->sb_wptr = so_rcv->sb_data + so_rcv->sb_cc;
> + }
> }
> m_free(m);
> return 0;
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] slirp: check sscanf result when emulating ident
2019-03-02 17:42 ` Philippe Mathieu-Daudé
@ 2019-03-02 17:49 ` Samuel Thibault
2019-03-02 23:28 ` William Bowling
1 sibling, 0 replies; 5+ messages in thread
From: Samuel Thibault @ 2019-03-02 17:49 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: William Bowling, qemu-devel, qemu-stable, secalert
Hello,
Philippe Mathieu-Daudé, le sam. 02 mars 2019 18:42:42 +0100, a ecrit:
> Samuel: since this diff is not obvious without looking at the context
> (also due to the code re-indent),
I dropped the code re-indent to make the change obvious. I still added
the commit description, always better goes with saying it :)
Thanks!
Samuel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] slirp: check sscanf result when emulating ident
2019-03-02 17:42 ` Philippe Mathieu-Daudé
2019-03-02 17:49 ` Samuel Thibault
@ 2019-03-02 23:28 ` William Bowling
1 sibling, 0 replies; 5+ messages in thread
From: William Bowling @ 2019-03-02 23:28 UTC (permalink / raw)
To: Philippe Mathieu-Daudé; +Cc: qemu-devel, secalert, qemu-stable
Hi Phil,
William: How did you notice that? Using a static analyzer?
It was while looking into a previous CVE in tcp_emu, just with a manual
code review.
We have a data leak, Cc'ing qemu-stable.
> (Adding the address I noticed you Cc'ed secalert@redhat.com, so that
> confirms my guess).
Yeah the report and patch went via the security list initially due to the
info leak.
Cheers,
Will
On Sun, Mar 3, 2019 at 4:42 AM Philippe Mathieu-Daudé <philmd@redhat.com>
wrote:
> Hi William, Samuel,
>
> On 3/1/19 10:45 PM, William Bowling wrote:
> > When emulating ident in tcp_emu, if the strchr checks passed but the
> > sscanf check failed, two uninitialized variables would be copied and
> > sent in the reply.
>
> William: How did you notice that? Using a static analyzer?
>
> Samuel: since this diff is not obvious without looking at the context
> (also due to the code re-indent), can you improve the commit
> description, such (or better):
>
> "Move this code inside the if(sscanf()) clause".
>
> We have a data leak, Cc'ing qemu-stable.
> (Adding the address I noticed you Cc'ed secalert@redhat.com, so that
> confirms my guess).
>
> >
> > Signed-off-by: William Bowling <will@wbowling.info>
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>
> Thanks,
>
> Phil.
>
> > ---
> > slirp/tcp_subr.c | 12 ++++++------
> > 1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/slirp/tcp_subr.c b/slirp/tcp_subr.c
> > index 262a42d6c8..73a160ba16 100644
> > --- a/slirp/tcp_subr.c
> > +++ b/slirp/tcp_subr.c
> > @@ -664,12 +664,12 @@ tcp_emu(struct socket *so, struct mbuf *m)
> > break;
> > }
> > }
> > - }
> > - so_rcv->sb_cc =
> snprintf(so_rcv->sb_data,
> > -
> so_rcv->sb_datalen,
> > - "%d,%d\r\n",
> n1, n2);
> > - so_rcv->sb_rptr = so_rcv->sb_data;
> > - so_rcv->sb_wptr = so_rcv->sb_data +
> so_rcv->sb_cc;
> > + so_rcv->sb_cc = snprintf(so_rcv->sb_data,
> > + so_rcv->sb_datalen,
> > + "%d,%d\r\n", n1, n2);
> > + so_rcv->sb_rptr = so_rcv->sb_data;
> > + so_rcv->sb_wptr = so_rcv->sb_data + so_rcv->sb_cc;
> > + }
> > }
> > m_free(m);
> > return 0;
> >
>
--
GPG Key ID: 0x980F711A
GPG Key Fingerprint: AA38 2A0E 7D22 18A9 6086 0289 41DC E04B 980F 711A
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-03-02 23:28 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-03-01 21:45 [Qemu-devel] [PATCH] slirp: check sscanf result when emulating ident William Bowling
2019-03-02 2:21 ` Samuel Thibault
2019-03-02 17:42 ` Philippe Mathieu-Daudé
2019-03-02 17:49 ` Samuel Thibault
2019-03-02 23:28 ` William Bowling
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).