Frederic, On Sun, May 08, 2005 at 05:46:04PM +0200, Frederic Peters wrote: > > The changelog claims to fix infinite loop problems with the DSLw dissector > > and a double-free in the ICEP dissector, but here is the entire diff for > > those two source files: > > diff -u ethereal-0.10.10/epan/dissectors/packet-dlsw.c ethereal-0.10.10/epan/dissectors/packet-dlsw.c > > --- ethereal-0.10.10/epan/dissectors/packet-dlsw.c > > +++ ethereal-0.10.10/epan/dissectors/packet-dlsw.c > > @@ -2,7 +2,7 @@ > > * Routines for DLSw packet dissection (Data Link Switching) > > * Copyright 2001, Paul Ionescu <paul@acorp.ro> > > * > > - * $Id: packet-dlsw.c 13019 2005-01-13 17:26:10Z guy $ > > + * $Id: packet-dlsw.c 14178 2005-04-24 01:15:53Z gerald $ > > * > > * Ethereal - Network traffic analyzer > > * By Gerald Combs <gerald@ethereal.com> > This is not from ethereal_0.10.10-2sarge1.diff.gz; is this a diff > between -2 and -2sarge1 ? Yes. This change is present in the -2sarge1 diff that's in the archive, and was not part of -2. > Those fixes (both ICEP and DLSw) were backported in -2. > > Does this mean these problems were actually fixed in -2, where they are also > > mentioned in the changelog (without the CVE #)? > That's it. Ok. > > Also, there are changes to packet-bacnet.c, packet-cdp.c, packet-chdlc.c, > > packet-dcm.c, packet-gsm_a.c, packet-gssapi.c, packet-h245.c, > > packet-kerberos.c, packet-q931.c, packet-slowprotocols.c, packet-spnego.c > > that don't seem to correspond to anything in the changelog. Can you > > explain? > Since the advisory had some unclear parts (take CAN-2005-1460 for > example, "denial of service (assert) in misc dissectors) and ethereal > has a bad history I backported dissectors that checked lengths in a > more precise way. This applies to packet-bacnet.c, packet-cdp.c, > packet-chdlc.c, 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. > This also applies to packet-dcm.c (as well as memory leaks). 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? > packet-gssapi.c changes are the ntlmssp changes. 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. > packet-h245.c change fixes a possible segfault. > packet-q931.c change fixes possible segfaults (and double-free). Ah, yes, noted. "H.245" and "Q.931" didn't match my regexps. :) > 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. > packet-slowprotocols.c is mentioned in the advisory: " The 802.3 Slow > protocols dissector could throw an assertion." (probably caused by > a wrong buffer size). Right, another case of failing to catch this mention in the changelog using grep alone. :) > > The files packet-dcerpc.c, packet-rsvp.c, packet-acse.c, and packet-mgcp.c > > also contain at least some changes that appear to be unrelated to the > > security fixes, but are instead protocol dissection enhancements. I'm > > really not keen on allowing such changes in via t-p-u. > I don't know for packet-dcerpc.c (and packet-dcerpc-atsvc.c), > packet-rsvp.c only has some more proto_item_append_text, packet-acse.c > mostly removes code, packet-mgcp.c changes are indention changes (but > I didn't check them line by line). I'm not too worried about packet-dcerpc-atsvc.c, given the comment at the 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. > I'll have a closer look to how kerberos and spnego are related to > gssapi, as well as a closer look to dcerpc; I'll also cancel > packet-gsm_a.c changes; I believe the other ones are ok. Ok, thanks for taking care of the gsm_a changes. Please also consider my other comments above. And thanks for your efforts in general for preparing this backport. I know it's a lot of work to track this stuff (especially for a package like ethereal), and I'm sorry I have to be a pain about the contents of the diff. :) -- Steve Langasek postmodern programmer
Attachment:
signature.asc
Description: Digital signature