Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make upb_readfile() safer #51

Closed
wants to merge 1 commit into from

Conversation

atdt
Copy link

@atdt atdt commented Mar 4, 2016

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.

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
@haberman
Copy link
Member

haberman commented Mar 4, 2016

Thank you for the PR and the detailed explanation. However I noticed that the new code depends on POSIX interfaces, whereas upb is trying to rely only on C89. Is there a way to fix this that only uses C89 interfaces?

@haberman
Copy link
Member

haberman commented Mar 4, 2016

Also, given how much detail and effort you put into this, I'm wondering if this is part of a larger audit of some sort? Curious to know what your interest is and if there's any way I can facilitate.

@atdt atdt closed this Jan 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants