Uninitialized Memory Usage in ipp_read_io
Closed this issue · 1 comments
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.
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".