Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Mon, 28 Jan 2013 23:22:24 +0200
From: Milen Rangelov <>
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);

		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 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).


On Mon, Jan 28, 2013 at 11:09 PM, Solar Designer <> 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.