snmp-supplies: validate percent value to prevent undefined behavior

[dhejyuyr]

Describe the bug

The backendSNMPSupplies() function calculates the percent value using signed integer variables and writes the result into a fixed-sized buffer value[CUPS_MAX_SUPPLIES * 4].

The values level, max_capacity, and others are taken from the cash file (backend_init_supplies() function) or directly from the SNMP responses in backend_walk_cb(). Both functions don't check values.

On the one hand, unexpectedly large supplies level values in the expression 100 * supplies[i].level can lead to signed integer overflow and, as a result, to undefined behavior.

On the other hand, even small values can lead to problem. The results are written to char value[CUPS_MAX_SUPPLIES * 4]. Commas are placed between values: *ptr++ = ',';. The buffer is filled using snprintf()/cupsCopyString() and the pointer is moved on each iteration using ptr += strlen(ptr). The buffer size is CUPS_MAX_SUPPLIES * 4, so with max num_supplies each percent value should be no more than three characters, plus a comma as a delimiter. If the percent strings are longer than the buffer expected, concatenation may cause truncation and potentially lead to out-of-bounds buffer access. Even if the snprintf() limits the number of written bytes, it may truncate the output. The separator insertion doesn't handle truncation and doesn't check whether there is enough space left. This may result overwriting of the null terminator, also calling the strlen(ptr) may caused undefined behavior in the next iteration.

Solution

I believe the solution is to add check directly to the backendSNMPSupplies() function. If the level accidentally exceeds the max_capacity, then the percent is 100. In another case, it is better to expand the type of multiplication operation and then after division cast it to int. So the percent will be from 0 to 100, and as I understand the writing logic will work as well. Also may be is good to check values in the backend_walk_cb() function, but in that case I don't know how to check them in the backend_init_supplies() function.

[michaelrsweet]

Already reported and fixed as a security issue.

[dhejyuyr]

@michaelrsweet But this doesn't consider the possible integer overflow in the earlier expression 100 * supplies[i].level. Do we assume that the behavior is defined?

[michaelrsweet]

We assume that even if we get an undefined result, the subsequent range checks will ensure that "percent" is in the range 0 to 100.