Re: Accepted ethereal 0.10.10-2sarge1 (i386 source)
Steve Langasek 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.
>> [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) {
> 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.
> 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
I caught some of the changes.
> 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 ?
Frederic
Reply to: