[sword-devel] crashes with ciphering code

[Joachim Ansorg]

Hi,
I found a fix, so it's working for me now.

Change line 261 of src/modules/common/zverse.cpp to this:

        if (size > 0 && (start < strlen(cacheBuf))) {

Line 263 accessed the cacheBuf char array without checking for the size 
before. So if the uncompressing failed (strlen(cacheBuf) is 0 or too small) 
the bounds were exceeded and a crash occured.

I'm not sure if there are more parts like this which need to be fixed (e.g. in 
the lex driver).
I also know that this is not a nice way to fix it, probably a member to store 
the size of cacheBuf would be nice to work around this repeated call of 
strlen in my fix.

A compressed, locked module now is working with mod2imp with and without the 
right key set. At least for me ;)

Regards,
Joachim

> Hi DM and everybody.
>
> I changed the line to:
> 	strncpy(*buf, cacheBlock->getEntry(entry), size);
> 	(*buf)[size]='\0';
>
> But it didn't work. Still crashes.
>
> I believe I found the reason though. As I assumed many months ago, the
> reading process works like this:
> decryption -> decompression -> display.
>
> Proof (zstr.cpp):
> 	unsigned long len = size;
> 	buf.setSize(size);
> 	rawZFilter(buf, 0); // 0 = decipher
>
> 	compressor->zBuf(&len, buf.getRawData());
> 	char *rawBuf = compressor->Buf(0, &len);
> 	cacheBlock = new EntriesBlock(rawBuf, len);
> 	cacheBlockIndex = block;
>
> That means we're trying to decompress faulty data! Can we at least check
> for a zip signature or something in the decompressor?
> This must also be the reason for the frequent warnings
> "no room in outbuffer to during decompression. see zipcomp.cpp".
>
> If I have the "flow of events correct" this time, then we have a design
> problem. It would be best to change the flow of events and rebuild the
> modules and release a fixed and incompatible sword version.
>
> Or am I wrong? Please advise.
>
> Martin
>
> Am Montag, 27. November 2006 22:51 schrieb DM Smith:
> > Joachim Ansorg wrote:
> > >>   zStr::getCompressedText calls
> > >>  strcpy(*buf, cacheBlock->getEntry(entry));
> > >
> > > My fix for this would be (without digging deep into the sources) in
> > > line 438 of zstr.cpp:
> > > 	strncpy(*buf, cacheBlock->getEntry(entry), size);
> >
> > This takes me way back. I haven't done serious C coding in years! But I
> > remember something about this one. I had been bitten by it several times.
> >
> > IIRC:
> > strncpy will not null terminate the string. Generally its good to follow
> > it with buf[size] = '\0';
> >
> > strncpy will stop copying if it sees '\0' before it reaches the end. In
> > that case, strncpy stops after it copies the '\0'.
> > It may be good to compare strlen(buf) == size and throw an error if it
> > is not.
> >
> > Putting this together:
> > strncpy(*buf, cacheBlock->getEntry(entry), size);
> > buf[size] = '\0';
> > if (strlen(buf) != size) an error has occurred.
> >
> > Also, since we expect that the string is copied in its entirety, it is
> > probably better and faster to use memcpy. Memmove would also work but
> > would be slightly slower than memcpy.
> >
> > > strcpy expects a \0-terminated string. If the deciphering with the
> > > wrong key creates a char* without a proper \0 this would result in an
> > > address out of bounds. So the fix is to make sure we just copy the
> > > number of bytes which are available in the cacheBlock.
> > > I did not yet think whether a \0 has to explicitely be set at the end
> > > of *buf.
> > >
> > > Does this make sense?
> > > Does somebody have the setup to test this?
> > >
> > > Thanks,
> > > Joachim
> >
> > _______________________________________________
> > sword-devel mailing list: sword-devel at crosswire.org
> > http://www.crosswire.org/mailman/listinfo/sword-devel
> > Instructions to unsubscribe/change your settings at above page
>
> _______________________________________________
> sword-devel mailing list: sword-devel at crosswire.org
> http://www.crosswire.org/mailman/listinfo/sword-devel
> Instructions to unsubscribe/change your settings at above page

-- 
<>< Re: deemed
www.bibletime.info