On Mon, May 09, 2005 at 10:55:49AM +0200, Frederic Peters wrote:
> > Hmm. AFAICT, the packet-bacnet.c changes are all new features, adding
> > support for a new subprotocol; with no security fix implications.
> > Likewise, packet-cdp.c and packet-chdlc.c seem to be cosmetic changes only,
> > since tvb_get_foo() includes exception handling and is supposed to already
> > prevent any problems with reading from beyond the end of a packet; the new
> > code should be *safe*, but I have no idea if it's actually more correct than
> > the old code.
> I reverted packet-bacnet.c, packet-cdp.c and packet-chdlc.c changes.
Thanks.
> >> [packet-dcm.c]
> > Hrm, I do see the changed comment at the top of the file mentioning the
> > memory leak fix, but the rest, again, looks like new features. Is there any
> > hope that the memory leak change can be split out fairly easily?
> Done; but there is also the following patch that may be security
> related, checking proper boundary:
> - while (len + nlen <= dcm_data->clen) {
> + while (len + nlen <= dcm_data->tlen && len + nlen <= dcm_data->clen) {
Yes, that looks like it should be included.
> > They seem to be enhancements to the GSSAPI dissector which *add* support for
> > additional NTLMSSP dissection, so which is unrelated to the security fixes
> > in the NTLMSSP dissector.
> > > I believe packet-kerberos.c and packet-spnego.c are somewhat related
> > > to the ntlmssp changes.
> > NTLMSSP is used as a GSS mechanism with Microsoft servers (and those which
> > interoperate), but it is not related to Kerberos; and the packet-kerberos.c
> > changes look suspiciously like a high-level rewrite to me, not a security
> > fix.
> > The packet-spnego.c changes may be what was meant by NTLMSSP; the protocols
> > are related, but distinct. In any case, the affected code here was
> > previously bounded by #ifdef HAVE_HEIMDAL_KERBEROS, and ethereal is linked
> > against libkrb53 (i.e., MIT Kerberos), so this is not a security issue for
> > Debian. Since the security fix to packet-spnego.c seems to additionally be
> > tied up with the API changes to packet-kerberos.c, I would really suggest
> > dropping both groups of changes if possible.
> Isolated a change in packet-spnego.c, I'm reading more of the code to
> see what else can be isolated.
Glad to hear it.
> > top that it's supposed to be auto-generated. Things like packet-mgcp.c,
> > though, can hide new bugs within the "indentation changes", so we really
> > want to avoid such things in changes that are introduced via t-p-u. I would
> > hope, given reasonable commit practices on the part of upstream, that it
> > would take you much less time to extract just the security changes for these
> > files and apply them than it would take me to review the much larger changes
> > currently there and ensure that I'm comfortable with their implications.
> ""reasonable commit practices"" :)
> | Revision 14181 - (view) (download) - [select for diffs]
> | Modified Mon Apr 25 00:51:11 2005 UTC (2 weeks ago) by lroland
> | File length: 83127 byte(s)
> | Diff to previous 14121
> |
> | From Martin Mathieson:
> | a first step of cleaning up the mgcp dissector:
> |
> | - re-indenting/formatting the file as it was tricky to follow
> | - some code simplification and commenting (more to do)
> | - losing some unnecessary includes (not needed under linux at least...)
> | - show duplicate request and response fields in tree (previously hidden)
> | - improved duplicate response detection
> | - fix a couple of problems shown by fuzz testing
Oh, ugh. Sorry to hear that. :)
> > Ok, thanks for taking care of the gsm_a changes. Please also consider my
> > other comments above.
> Done for the most part, a few issues remains, I'm taking care of them
> this afternoon. Should a new upload be 0.10.10-2sarge2 or should I
> keep sarge1 ?
Yes, I would greatly appreciate a sarge2.
Thanks again,
--
Steve Langasek
postmodern programmer
Attachment:
signature.asc
Description: Digital signature