google / upb / 177

70%

Pull #51

travis-ci

Make upb_readfile() safer

upb_readfile() computes the size of the file that it plans to read by opening
it as a binary stream and then using fseek() / ftell(). This is not reliable.
Subclause 7.21.9.2 of the C standard stipulates that "a binary stream need not
meaningfully support fseek calls with a whence value of SEEK_END"; Additionally,
footnote 268 of subclause 7.21.3 says: "Setting the file position indicator to
end-of-file, as with fseek(file, 0, SEEK_END), has undefined behavior for a
binary stream". On non-POSIX systems, using fseek() to set the file position
indicator to end-of-file is not guaranteed to work. If the program tries to
allocate memory based on the result of fseek() / ftell(), the amount may be
incorrect, leading to a potential vulnerability.

To address this issue, we can simply use the file size provided by fstat(),
instead of fseek() / ftell(). Both the diagnosis and the resolution are
supported by recommendation [FIO19-C][] of the SEI CERT C Coding Standard.

udp_readfile() also allocates a buffer that is one byte larger than the file,
presumably with the intention of making sure that the buffer is
null-terminated. However, it does not check the return value of malloc() to
ensure the allocation succeeded, and it does not actually null-terminate the
buffer. Fix that by explicitly null-terminating the buffer.

[FIO19-C]: https://www.securecoding.cert.org/confluence/display/c/FIO19-C.+Do+not+use+fseek%28%29+and+ftell%28%29+to+compute+the+size+of+a+regular+file?src=contextnavpagetreemode

11 of 11 new or added lines in 1 file covered. (100.0%)

5867 of 7251 relevant lines covered (80.91%)

272740.1 hits per line