Florian Ernst wrote:
> Hello there,
>
> On Thu, Jun 02, 2005 at 05:53:19PM +0200, Martin Schulze wrote:
> > Florian Ernst wrote:
> > > On Sat, May 28, 2005 at 12:32:39AM +0200, Florian Ernst wrote:
> > > > Find attached the backported patch I sent to the security team.
> > >
> > > Well, now, really, that is.
> >
> > I may be stupid, but how can this prevent an integer overflow:
> >
> > - thunk_table=(PE_THUNK_DATA*)malloc(sizeof *thunk_table *
> > thunk_count);
> > + if (thunk_count) {
> > + thunk_table=(PE_THUNK_DATA*)malloc(sizeof
> > *thunk_table * thunk_count);
> >
> > Just set thunk_count to MAX_UINT-1 and see what the result of
> > the multiplication is.
>
> I believe this change wasn't aimed at preventing an integer overflow
> at all, but rather at preventing a "malloc(0)".
In that case the patch is not needed in a security update since a
malloc (0) for a structure that is always referenced by the factor
it caused the 0, does not hurt.
I've checked this for the one location only which said/says:
thunk_table=(PE_THUNK_DATA*)malloc(sizeof *thunk_table * thunk_count);
...
for (UINT i=0; i<thunk_count; i++) {
^^^^^^^^^^^
...
for (dword i=0; i<thunk_count; i++) {
^^^^^^^^^^^
Both loops won't be entered if thunk_count is zero anyway
--> superflous crap for a security update.
> The preceding part of the code tries to limit the maximum size
> "thunk_count" could reach via the "if (!thunk.ordinal) break;"
> (all code taken from 0.5.0):
> | PE_THUNK_DATA thunk;
> | dword thunk_count = 0;
> | file->seek(thunk_ofs);
> | while (1) {
> | file->read(&thunk, sizeof thunk);
> | create_host_struct(&thunk, PE_THUNK_DATA_struct,
> little_endian);
> | if (!thunk.ordinal) break;
> | thunk_count++;
> | }
> |
> | PE_THUNK_DATA *thunk_table;
>
> But if "!thunk.ordinal" is (for whatever reasons) met during the first
> cycle "thunk_count" will remain zero and thus the following code
It's ok to fix or change that upstream but it must not clutter
a security update.
> | thunk_table=(PE_THUNK_DATA*)malloc(sizeof *thunk_table *
> thunk_count);
> | file->seek(thunk_ofs);
> | file->read(thunk_table, sizeof *thunk_table * thunk_count);
> | // FIXME: ?
> | for (UINT i=0; i<thunk_count; i++) {
> would effectively read
> | thunk_table=(PE_THUNK_DATA*)malloc(0);
> | [...]
> | file->read(thunk_table, 0);
> | [...]
> | for (UINT i=0; i<0; i++) {
> which is, err, not that good. OTOH, as Debian utilizes glibc which
> AFAIK supports malloc(0), this particular change might not be strictly
> necessary for Debian security purposes. I have to admit my changelog
> entry is quite misleading wrt this change, I just did a simple copy
> and paste of the description as it appeared sufficient to me.
Yes.
> This is my interpretation of this change, please hit me (hard) with a
> cluebat if I'm wrong.
I had hoped you would have come up with a different explanation, since
this is what I thought as well. :-%
Regards,
Joey
--
Long noun chains don't automatically imply security. -- Bruce Schneier
Please always Cc to me when replying to me on the lists.
--
To UNSUBSCRIBE, email to [EMAIL PROTECTED]
with a subject of "unsubscribe". Trouble? Contact [EMAIL PROTECTED]
