OpenPrinting / cups

OpenPrinting CUPS Sources
https://openprinting.github.io/cups
Apache License 2.0
1.1k stars 195 forks source link

Uninitialized Memory Usage in ipp_read_io #1077

Closed fish98 closed 1 month ago

fish98 commented 1 month ago

Description

After the upgrade of ipp.c in the recent commit, the usage of uninitialized memory of the list array still exists in the function ipp_read_io of cups/ipp.c. Detailed code can be found below:

static ipp_state_t          // O - Current state
ipp_read_io(void        *src,       // I - Data source
            ipp_io_cb_t cb,     // I - Read callback function
        int         blocking,   // I - Use blocking IO?
        ipp_t       *parent,    // I - Parent request, if any
            ipp_t       *ipp,       // I - IPP data
            int         depth)      // I - Depth of collection
{
  int           n;      // Length of data
  unsigned char     *buffer,    // Data buffer
            string[IPP_MAX_TEXT],
                    // Small string buffer
...
  if ((bufptr + 2 + n + 2) > bufend || n >= (int)sizeof(string))
  {
        _cupsSetError(IPP_STATUS_ERROR_INTERNAL, _("IPP language length overflows value."), 1);
        DEBUG_printf("1ipp_read_io: bad language value length %d.", n);
        goto rollback;
  }
  else if (n >= IPP_MAX_LANGUAGE)
  {
        _cupsSetError(IPP_STATUS_ERROR_INTERNAL, _("IPP language length too large."), 1);
        DEBUG_printf("1ipp_read_io: bad language value length %d.", n);
        goto rollback;
  }

  memcpy(string, bufptr + 2, (size_t)n);
  string[n] = '\0';

  value->string.language = _cupsStrAlloc((char *)string);
...

The string array is used to store temporal data from the buffer. Before the execution of memcpy(string, bufptr + 2, (size_t)n);, there lacks proper initialization to ensure that the remaining parts of the string array are properly initialized. If the length n is less than IPP_MAX_TEXT and less than the size of the array, the remaining parts will retain the original data in memory, which could lead to potential leaks or unexpected behavior.

Suggested Fix

Add memset(string, 0, sizeof(string));

Postscript

The related OSS-Fuzz issue.

michaelrsweet commented 1 month ago

OK, so "string" is a temporary buffer. We copy N bytes to string, then nul-terminate the N+1'th byte in the buffer, and then make a copy as needed in the string pool. At no point do we look at byte N+2, N+3, etc., and bytes 0 to N+1 are initialized by the memcpy and assignment.

So honestly I don't see how this has any chance of leaking any information from "string".