Ethereal-dev: Re: [Ethereal-dev] Re: [Coverity] Possible Format String Vulnerabilites
Are you sure you want to use g_assert. I think it would be better for
the dissector to record that packet as having an error by throwing an
exception, rather than having ethereal abort on bad data. Otherwise,
it would be easy for a packet to be created that would crash Ethereal.
--gilbert
On Thu, 17 Mar 2005 00:41:17 +1100, ronnie sahlberg
<ronniesahlberg@xxxxxxxxx> wrote:
> i have checked in a fix for bug 3,
> just a simple g_assert(length<0xff) which should be enough here.
>
> thankyou for your input and analysis of culnerabilities.
> i will, unless someone beats me to it, look into bug 1 and 2 tomorrow.
>
> thanks.
>
>
> On Mon, 14 Mar 2005 18:54:53 -0800, Bryan Fulton <bryan@xxxxxxxxxxxx> wrote:
> > I didn't mean to tease; here's an example of one of the misused scalar
> > bugs. Again take a peek and let me know if this seems valid, and we can
> > send over some more for your inspection.
> >
> > Thanks again,
> > .:bryan
> >
> > Bug 3:
> > /ethereal-0.10.10/epan/dissectors/packet-manolito.c:dissect_manolito
> > - length pulled off a tvb via tvb_get_guint8(), and length+1 is
> > passed to malloc without proper bounds checking. Possible integer
> > overflow on the allocation, and buffer overflow on the tvb_memcpy().
> >
> > Function "tvb_get_guint8" returns TAINTED data
> > Variable "length" TAINTED from assignment to tainted return value of
> > "tvb_get_guint8"
> >
> > 184 length = tvb_get_guint8(tvb, ++offset)
> >
> > TAINTED variable "(length + 1)" was passed to a tainted sink.
> >
> > 187 data = malloc(length + 1);
> >
> > TAINTED variable "length" was passed to a tainted sink.
> >
> > 188 tvb_memcpy(tvb, (guint8*)data, ++offset, length);
> >
> > ...
> >
> > 196
> > 197 if (dtype == MANOLITO_STRING)
> > 198 {
> >
> > TAINTED variable "length" used as index to pointer "data"
> >
> > 199 data[length] = 0;
> >
> >
> > On Mon, 2005-03-14 at 18:38, Bryan Fulton wrote:
> > > Hi,
> > >
> > > While testing our security checkers here at Coverity, we discovered a
> > > handful of possible flaws in Ethereal-10.10. Most are improper usage of
> > > scalars pulled off a packet (similar to the recent advisories) and we'll
> > > send you more detailed information on those soon. Right now, I thought
> > > you would be interested in a couple of possible format string
> > > vulnerabilities.
> > >
> > > Let me know if these look valid, and if so, when a possible patch would
> > > be made available. We appreciate your feedback as a method to improve
> > > and expand our security checkers.
> > >
> > > Thanks,
> > > .:Bryan Fulton of Coverity, Inc.
> > >
> > >
> > >
> > > Bug 1:
> > > /ethereal-0.10.10/epan/dissectors/packet-mq.c:dissect_mq_pdu
> > > - sStructId pulled off of tvb via tvb_get_string() and passed to
> > > proto_tree_add_text() as format argument.
> > >
> > > 1608 guint8* sStructId;
> > >
> > > Function "tvb_get_string" returns TAINTED string content
> > > Variable "sStructId" TAINTED from assignment to tainted return value of
> > > "tvb_get_string"
> > >
> > > 1609 sStructId = tvb_get_string(tvb, offset, 4);
> > >
> > > TAINTED string "sStructId" was passed to a format string sink.
> > >
> > > 1610 ti = proto_tree_add_text(mqroot_tree, tvb, offset, -1,
> > > (const char*)sStructId);
> > >
> > >
> > >
> > > Bug 2:
> > > /ethereal-0.10.10/epan/dissectors/packet-ansi_a.c:elem_cld_party_ascii_num
> > > - poctets pulled of of tcv via tvb_get_string() and passed to
> > > proto_tree_add_string_function() as format argument.
> > > - This one is fairly subtle to notice as first glance. It looks like the
> > > value argument is missing, as the string "Digits: %s" is used as value
> > > in the call to proto_tree_add_string_format(). The user-controlled
> > > poctets is then unsafely passed as the FS argument.
> > >
> > > Function "tvb_get_string" returns TAINTED string content
> > > Variable "poctets" TAINTED from assignment to tainted return value of
> > > "tvb_get_string"
> > >
> > > 5307 poctets = tvb_get_string(tvb, curr_offset, len - (curr_offset
> > > - offset));
> > >
> > > TAINTED string "poctets" was passed to a format string sink.
> > > 5309 proto_tree_add_string_format(tree,
> > > hf_ansi_a_cld_party_ascii_num,
> > > 5310 tvb, curr_offset, len - (curr_offset - offset),
> > > 5311 "Digits: %s",
> > > 5312 poctets);
> > --
> > Bryan J Fulton
> > Coverity, Inc.
> > 185 Berry St, Suite 3600
> > San Francisco, CA 94107
> > www.coverity.com
> >
> > Phone: 415-321-5206
> > Fax: 415-541-9521
> > Email: bryan@xxxxxxxxxxxx
> >
> > _______________________________________________
> > Ethereal-dev mailing list
> > Ethereal-dev@xxxxxxxxxxxx
> > http://www.ethereal.com/mailman/listinfo/ethereal-dev
> >
>
> _______________________________________________
> Ethereal-dev mailing list
> Ethereal-dev@xxxxxxxxxxxx
> http://www.ethereal.com/mailman/listinfo/ethereal-dev
>
>
