[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CABh=JRHAYn=uVD4c2oVfMvYDmjtyvfYHggGDW7c23Niai4E+RA@mail.gmail.com>
Date: Mon, 28 Jan 2013 23:22:24 +0200
From: Milen Rangelov <gat3way@...il.com>
To: john-dev@...ts.openwall.com
Subject: Re: dmg2john
I apologize because it came from my poor code.
There are two issues actually. The floating point one (which can be bad,
but I think is relatively innocent as compared to the next one).
You see, both data_size and cno are int. Then we have this:
cno = ceil(header2.datasize / 4096.0) - 2;
chunk = (unsigned char *) malloc(header2.datasize);
data_size = header2.datasize - cno * 4096;
if (data_size < 0) {
fprintf(stderr, "File %s is not a valid DMG file!\n", filename);
return;
}
lseek(fd, header2.dataoffset, SEEK_SET);
read(fd, chunk, header2.datasize);
So what happens with big files with big header2.datasize:
1) cno might be right after the fp division (I guess for 15GB of file it
would most likely still be correct)
2) data_size = header2.datasize - cno*4096 .....here it becomes a bit
interesting because we have a check for data_size < 0 right after that. I
imagine for certain sizes it can overflow in the *correct way* becoming
positive still having bad enough value which later on crashes jtr.
I apologize again, I must have been drunk when writing that (in fact, it
might have come from the vilefault project, need to check - as I reused
lots of their parsing code, fixing and accomodating some of the stuff). But
I definitely should have noticed that - it doesn't look quite normal (going
to double then back to int is not very clever for sure).
Regards
On Mon, Jan 28, 2013 at 11:09 PM, Solar Designer <solar@...nwall.com> wrote:
> Dhiru, Milen -
>
> dmg2john is in bad shape now. Here are some issues:
>
> 1. It's not being built by default. "make dmg2john" builds it, but this
> should be made the default.
>
> 2. It's not integrated into "john", to be similar with other *2john
> tools. It becomes a separate binary executable. Perhaps we need to
> integrate it, since it has no dependencies on extra libs.
>
> 3. The return values from lseek() are not checked. They must be!
>
> 4. The return values from read() are either not checked or are checked
> incorrectly. "<= 0" is not it. read() may also return with partial
> data. We need to use a read_loop() function (see popa3d), or at the
> very least detect the partial reads and refuse to work if so.
> Alternatively, we may switch to using "FILE *" and the f*() functions.
>
> 5. As also spotted by Milen:
>
> <@gat3way> @jmgosney @jeremiahg @DhiruKholia @solardiz Hm I think I found
> the problem....cno = ceil(header2.datasize / 4096.0) - 2; cno is int
>
> We must not do any floating-point math. When header2.datasize is large,
> there may be precision loss here, and the resulting value may be other
> than what we expect. We should express this without resorting to
> floating-point intermediate values:
>
> cno = (header2.datasize + 4095) / 4096 - 2;
>
> Milen - is this what you meant, too?
>
> Thanks,
>
> Alexander
>
Content of type "text/html" skipped
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
