Buffer overflows in libuboot_env_store

[kevincathcart-cas]

It looks like there are a few buffer overflows possible in libuboot_env_store.

Some effort has been made to avoid this. For example, while appending variables into the buffer, it checks if there there is enough space left in the buffer for the variable name, the variable value, and two more bytes (the equal and the null terminator for this variable), if there is not enough space, it returns -ENOMEM. So far so good.

Unfortunately, that is not the only place where data gets appended to the buffer. There are two more areas where that happens, and both look problematic to me.

For the first, let us assume that saveflags remains zero. In that scenario, right after writing out the variables into the buffer, the code will execute *buf++ = '\0';. It is not clear to me that is safe. I'm pretty sure that the variable populating logic would allow the null byte for the final variable to land in the very last byte of the buffer. Then the *buf++ = '\0'; line for terminating the list would be writing one past the end of the buffer.

Even worse is if execution enters into the saveflags branch. That branch does not check before writing with snprintf. For the very first such write, snprintf will (by design) ensure the buffer cannot overflow. However, snprintf will in such a scenario return a value larger than size, which will increment the buffer pointer past the end of the buffer. Then the next size calculation will underflow and wrap around (because size_t is unsigned). Because of that wrap-around the next snprintf would think there is tons of space left and could write outside the bounds of the buffer.

Calculating the sizes in that branch seems more finicky to do up front. An alternative could be to store the return value of each snprintf call into ret, and then check for truncation which is defined as a return value greater than or equal to the size parameter. If truncation occurred, return -ENOMEM. Otherwise we could proceed with buf += ret.

Lastly since we are already discussing security adjacent considerations, the use of malloc instead of calloc for this buffer (which gets written in full to disk, but is not always fully initialized by the function) is a potential information leakage channel. While hopefully swupdate is not just freeing memory that contains sensitive stuff, it is plausible that some other applications that use this as a library might.

Hope this helps.