Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

The zlib library is a venerable piece of C software. The copyright for the decompression routine goes back to 1995, with the last rewrite in 2002. It is used daily by millions.

To be perfect, the decompression routine should not invoke undefined behavior, even if passed a maliciously crafted buffer. I have the opportunity to detect a number of places where it is not obvious that the library does not invoke undefined behavior on some input. Would users of this site help me in confirming these issues? We would improve an already great piece of Open Source software, and there is always something to learn in reading code that has passed the test of time.

Assuming this is okay, for my first post, I would like to suggest looking at function inflate() in version 1.2.7 of the library.

int ZEXPORT inflate(strm, flush)
z_streamp strm;
int flush;
{
    struct inflate_state FAR *state;
    state = (struct inflate_state FAR *)strm->state;
/*** strm->state was previously allocated with malloc(), apparently.
Function malloc(), when it succeeds,
returns an uninitialized block of memory. */
...
    for (;;)
        switch (state->mode) {
...
        case MATCH:
            if (left == 0) goto inf_leave;
            copy = out - left;
            if (state->offset > copy) {         /* copy from window */
/*** It is unclear if state->offset always contains initialized contents 
at this point, line 1125 in inflate.c.
*/
...

So my question is, is it possible to reach line 1125 in inflate.c without having initialized state->offset?

I linked the library with the following main() function to identify the possible issue, so I guess that an input vector, if one exists, could be based on this main(), simply initializing array in[] to values that cause the issue. This main() is using the library correctly, I hope, otherwise the flagged issue may be meaningless.

#include "zlib.h"

#define CHUNK 100

z_stream strm;
unsigned char in[CHUNK];
unsigned char out[CHUNK];
int ret;

main(){
  int i;

  /* allocate inflate state */
  strm.zalloc = Z_NULL;
  strm.zfree = Z_NULL;
  strm.opaque = Z_NULL;
  strm.avail_in = 0;
  strm.next_in = Z_NULL;
  ret = inflateInit(&strm);
  if (ret != Z_OK)
    return ret;

  for (i=0; i<CHUNK; i++)
    in[i] = any();

  strm.next_in = in;
  strm.avail_in = CHUNK;
  strm.next_out = out;
  strm.avail_out = CHUNK;

  ret = inflate(&strm, Z_NO_FLUSH);
}

Of course, if the apparently dangerous access at line 1125 is in fact safe, then a great answer would be an explanation why.

share|improve this question

1 Answer 1

up vote 3 down vote accepted

I am not an expert in the zlib source tree nor am I familiar with the coding style of its authors, but I found this to be an interesting question. I'd say it looks safe and my guess is the original authors are expecting offset to be initialized to zero. Update: OK, I misread an if statement; looking at it again it looks like it might be unsafe without a custom allocator, so we'd have to look at inflateInit et al. and the actual inflate logic to know for sure.

Explanation follows:

It looks like inflate_state is allocated with the ZALLOC macro.

That macro resolves to:

#define ZALLOC(strm, items, size) \
       (*((strm)->zalloc))((strm)->opaque, (items), (size))

According to comments in zlib.h, correct use of the library requires the caller to initialize the zalloc member being called:

The application must initialize zalloc, zfree and opaque before calling the init function.

If strm->zalloc is NULL, which I'd expect most callers to do, the library uses its own default implementation, zcalloc.

zcalloc, with some exceptions for ancient MS-DOS compilers and so long as sizeof(unsigned int) is greater than 1, ends up calling calloc.

calloc by definition zeroes out its data.

So, I'd say it's safe under the following conditions:

  1. If you use a custom allocator, make sure it zeroes out the data.

2. sizeof(unsigned int) must be > 1. (I'm guessing the standard guarantees that but I'm not certain.)

After the update: Now realizing that the allocations are not necessarily zeroed, I'd say it all depends on whether or not the local variable here can be used uninitialized, as the only assignment I see is based on that value.

Note that inflateInit, which calls inflateReset, which calls inflateResetKeep, seems to initialize the members that the assignments to here depends on, notably lenbits. It looks like there are lots of size checks for overflow as well, eg: if ((unsigned)(here.bits) <= bits) break;. I haven't given it a thorough audit but I'd say everything I've seen so far looks safe.

share|improve this answer
    
When I analyzed it, I got this call stack: malloc <- zcalloc <- inflateInit2_ <- inflateInit_ <- main. I had to provide malloc(), and I provided an allocation function that does not initialize (since malloc() doesn't. Let me investigate and come back to you. –  Pascal Cuoq Dec 6 '12 at 22:18
    
@PascalCuoq Set it to NULL and you should be fine. –  asveikau Dec 6 '12 at 22:21
    
I set strm.zalloc to NULL (see the main()) in my question). You must be referring to sizeof(uInt) > 2 ? (voidpf)malloc(items * size) : (voidpf)calloc(items, size) in zutil.c:310. This resolves to a call to malloc() on many architectures, including the one my analysis is tuned for (sizeof(uInt)==4). –  Pascal Cuoq Dec 6 '12 at 22:23
    
@PascalCuoq - Yeah I looked back and it and it was backwards from how I read it the first time. Whoops. –  asveikau Dec 6 '12 at 22:26
    
Yes, the only assignment to state->offset is from here at line inflate.c:1103. The way I see the question is, if it possible to craft an input where case MATCH: is taken before case DIST:? –  Pascal Cuoq Dec 6 '12 at 22:41

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.