Potential uninitialized access in zlib

Question

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.

Answer 1

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.