POSIX: CHECK() that file_util::ScopedFD fulfills promise.

POSIX: CHECK() that file_util::ScopedFD fulfills promise.

CHECK() that file_util::ScopedFD will actually close the file descriptors.
There are security implications to not doing so, and logging an error is not
enough: file descriptor are security capabilities. Failing to close them is a
failure to revoke access to certain resources, which is heavily relied on in the
code base.

This CL also adds unit tests to file_util::ScopedFD.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=254129

[jln (very slow on Chromium), 2014-02-28 01:41:39]

Will, do you mind taking a look at this small CL?

[willchan no longer on Chromium, 2014-02-28 01:48:47]

lgtm

https://chromiumcodereview.appspot.com/183953004/diff/1/base/file_util.h
File base/file_util.h (right):

https://chromiumcodereview.appspot.com/183953004/diff/1/base/file_util.h#newc...
base/file_util.h:419: // There are security implications to not closing a file
descriptor
If it's not private and doesn't take forever to explain, can you update this and
the CL description to explain the security implications? They're not obvious to
me (but I trust you).

https://chromiumcodereview.appspot.com/183953004/diff/1/base/file_util_unitte...
File base/file_util_unittest.cc (right):

https://chromiumcodereview.appspot.com/183953004/diff/1/base/file_util_unitte...
base/file_util_unittest.cc:2499: CHECK_EQ(0, IGNORE_EINTR(close(fds[1])));
Shouldn't this be EXPECT_EQ? Is there an advantage to crashing the test suite
when this fails?

[jln (very slow on Chromium), 2014-02-28 02:01:21]

Thanks Will, all done!

https://chromiumcodereview.appspot.com/183953004/diff/1/base/file_util.h
File base/file_util.h (right):

https://chromiumcodereview.appspot.com/183953004/diff/1/base/file_util.h#newc...
base/file_util.h:419: // There are security implications to not closing a file
descriptor
On 2014/02/28 01:48:48, willchan wrote:
> If it's not private and doesn't take forever to explain, can you update this
and
> the CL description to explain the security implications? They're not obvious
to
> me (but I trust you).

Done.

We often have schemes such as: 1. open privileged resources 2. Enable sandbox 3.
Drop access to privilege resources.
(3) failing can be very problematic, especially for directory fd and the setuid
sandbox (since openat(dir_fd, ...) can bypass a chroot).

A lot of the code in sandbox/ is paranoid and redefines its own *Closer because
base/ is not reliable. It looks like patching base is the right thing to do
instead.

https://chromiumcodereview.appspot.com/183953004/diff/1/base/file_util_unitte...
File base/file_util_unittest.cc (right):

https://chromiumcodereview.appspot.com/183953004/diff/1/base/file_util_unitte...
base/file_util_unittest.cc:2499: CHECK_EQ(0, IGNORE_EINTR(close(fds[1])));
On 2014/02/28 01:48:48, willchan wrote:
> Shouldn't this be EXPECT_EQ? Is there an advantage to crashing the test suite
> when this fails?

You're right, a fd leak is probably not worth a crash. Done.

[jln (very slow on Chromium), 2014-02-28 06:45:37]

The CQ bit was checked by jln@chromium.org

[commit-bot: I haz the power, 2014-02-28 06:46:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jln@chromium.org/183953004/40001

[commit-bot: I haz the power, 2014-02-28 15:35:37]

Change committed as 254129