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)".
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
| 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.
This is my interpretation of this change, please hit me (hard) with a
cluebat if I'm wrong.
Cheers,
Flo
signature.asc
Description: Digital signature
