Re: [Qemu-devel] [PATCH] slirp: check sscanf result when emulating ident
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 secal...@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é 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 secal...@redhat.com, so that > confirms my guess). > > > > > Signed-off-by: William Bowling > > Reviewed-by: Philippe Mathieu-Daudé > > 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
Re: [Qemu-devel] [PATCH] slirp: check sscanf result when emulating ident
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
Re: [Qemu-devel] [PATCH] slirp: check sscanf result when emulating ident
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 secal...@redhat.com, so that confirms my guess). > > Signed-off-by: William Bowling Reviewed-by: Philippe Mathieu-Daudé 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; >
Re: [Qemu-devel] [PATCH] slirp: check sscanf result when emulating ident
William Bowling, le ven. 01 mars 2019 21:45:56 +, 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 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.)
[Qemu-devel] [PATCH] slirp: check sscanf result when emulating ident
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 --- 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
