| Message ID | 20190111081909.11896-1-ppandit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | slirp: check data length while emulating ident function | expand |
Hi On Fri, Jan 11, 2019 at 12:31 PM P J P <ppandit@redhat.com> wrote: > > From: Prasad J Pandit <pjp@fedoraproject.org> > > While emulating identification protocol, tcp_emu() does not check > available space in the 'sc_rcv->sb_data' buffer. It could lead to > heap buffer overflow issue. Add check to avoid it. > > Reported-by: Kira <864786842@qq.com> > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org> > --- > slirp/tcp_subr.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/slirp/tcp_subr.c b/slirp/tcp_subr.c > index fa61349cbb..4415801fbb 100644 > --- a/slirp/tcp_subr.c > +++ b/slirp/tcp_subr.c > @@ -635,6 +635,11 @@ tcp_emu(struct socket *so, struct mbuf *m) > socklen_t addrlen = sizeof(struct sockaddr_in); > struct sbuf *so_rcv = &so->so_rcv; > > + if (m->m_len > so_rcv->sb_datalen > + - (so_rcv->sb_wptr - so_rcv->sb_data)) { > + m_free(m); > + return 0; > + } Check looks correct, it should probably return 1. Is there a reproducer? > memcpy(so_rcv->sb_wptr, m->m_data, m->m_len); > so_rcv->sb_wptr += m->m_len; > so_rcv->sb_rptr += m->m_len; > -- > 2.20.1 > >
+-- On Fri, 11 Jan 2019, Marc-André Lureau wrote --+
| > + if (m->m_len > so_rcv->sb_datalen
| > + - (so_rcv->sb_wptr - so_rcv->sb_data)) {
| > + m_free(m);
| > + return 0;
| > + }
|
| Check looks correct, it should probably return 1.
Function comment says return 1 if 'm' is valid and should be appended via
sbappend(). Not sure if unprocessed 'm' should go to sbappend().
| Is there a reproducer?
Yes, I have one.
Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
Hi On Fri, Jan 11, 2019 at 1:18 PM P J P <ppandit@redhat.com> wrote: > > +-- On Fri, 11 Jan 2019, Marc-André Lureau wrote --+ > | > + if (m->m_len > so_rcv->sb_datalen > | > + - (so_rcv->sb_wptr - so_rcv->sb_data)) { > | > + m_free(m); > | > + return 0; > | > + } > | > | Check looks correct, it should probably return 1. > > Function comment says return 1 if 'm' is valid and should be appended via > sbappend(). Not sure if unprocessed 'm' should go to sbappend(). If you look at the rest of the function, many similar error cases return 1. > | Is there a reproducer? > > Yes, I have one. Ok, could you add it to the commit message ? :) > > Thank you. > -- > Prasad J Pandit / Red Hat Product Security Team > 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
+-- On Fri, 11 Jan 2019, Marc-André Lureau wrote --+ | > | Check looks correct, it should probably return 1. | > | > Function comment says return 1 if 'm' is valid and should be appended via | > sbappend(). Not sure if unprocessed 'm' should go to sbappend(). | | If you look at the rest of the function, many similar error cases return 1. True, not sure if that's right in this case. Nonetheless, I have sent a revised patch to return 1. | Ok, could you add it to the commit message ? :) No, I'm afraid not. Thank you. -- Prasad J Pandit / Red Hat Product Security Team 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
diff --git a/slirp/tcp_subr.c b/slirp/tcp_subr.c index fa61349cbb..4415801fbb 100644 --- a/slirp/tcp_subr.c +++ b/slirp/tcp_subr.c @@ -635,6 +635,11 @@ tcp_emu(struct socket *so, struct mbuf *m) socklen_t addrlen = sizeof(struct sockaddr_in); struct sbuf *so_rcv = &so->so_rcv; + if (m->m_len > so_rcv->sb_datalen + - (so_rcv->sb_wptr - so_rcv->sb_data)) { + m_free(m); + return 0; + } memcpy(so_rcv->sb_wptr, m->m_data, m->m_len); so_rcv->sb_wptr += m->m_len; so_rcv->sb_rptr += m->m_len;