Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use of uninitialized memory of trailer list #91

Closed
fish98 opened this issue Oct 2, 2024 · 1 comment
Closed

Use of uninitialized memory of trailer list #91

fish98 opened this issue Oct 2, 2024 · 1 comment
Assignees
Labels
bug Something isn't working priority-low
Milestone

Comments

@fish98
Copy link

fish98 commented Oct 2, 2024

Description

The use of uninitialized memory of the trailer array is found in function cups_fill of cups/file.c. Detailed code can be found below:

unsigned char	trailer[8];	// Trailer bytes
uLong		tcrc;		// Trailer CRC
ssize_t		tbytes = 0;	// Number of bytes

if (fp->stream.avail_in > 0)
{
  if (fp->stream.avail_in > sizeof(trailer))
    tbytes = (ssize_t)sizeof(trailer);
  else
    tbytes = (ssize_t)fp->stream.avail_in;

  memcpy(trailer, fp->stream.next_in, (size_t)tbytes);
  fp->stream.next_in  += tbytes;
  fp->stream.avail_in -= (size_t)tbytes;
}

      if (tbytes < (ssize_t)sizeof(trailer))
{
  if (read(fp->fd, trailer + tbytes, sizeof(trailer) - (size_t)tbytes) < ((ssize_t)sizeof(trailer) - tbytes))
  {
...
  }
}

tcrc = ((((((uLong)trailer[3] << 8) | (uLong)trailer[2]) << 8) | (uLong)trailer[1]) << 8) | (uLong)trailer[0];

if (tcrc != fp->crc)
{ 
...

when vail_in is less than sizeof(trailer), the operation memcpy(trailer, fp->stream.next_in, (size_t)tbytes); will end up with uninitialized value in trailer array. The subsequent function if (read(fp->fd, trailer + tbytes, sizeof(trailer) - (size_t)tbytes) < ((ssize_t)sizeof(trailer) - tbytes)) may also inroduce unitialized value issue when read() function returns EOF or error.

Suggested Fix

  1. Initialize trailer with zero e.g., unsigned char trailer[8] = {0};
  2. Handle read() error

Postscript

The issue is identified by OSS-Fuzz harness fuzzipp with MSAN. Here is the linked issue.

@michaelrsweet
Copy link
Member

OK, the code handles read errors - if we get less than "sizeof(trailer) - tbytes" bytes, that is an error and we abort.

I think the issue is that this and the CRC failure don't call inflateEnd.

Test this change:

[master 83562f7] Make sure we call inflateEnd when there is an error reading or comparing the stream CRC (Issue #91)

michaelrsweet added a commit that referenced this issue Oct 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working priority-low
Projects
None yet
Development

No branches or pull requests

2 participants