Security CHECK for FD close failure in ScopedFD.

[Krzysztof Olczyk]

Hi,

In Chromium, we have a "scoped handle" class for Posix file descriptors (ScopedFD) which just makes sure FD is closed when it goes out of scope.

When closing a FD, there is also a PCHECK which verifies that the operation has succeeded

with the adequate comment on the security reasoning which brings to it:

// It's important to crash here. 

// There are security implications to not closing a file descriptor 

// properly. As file descriptors are "capabilities", keeping them open 

// would make the current process keep access to a resource. Much of 

// Chrome relies on being able to "drop" such access. 

// It's especially problematic on Linux with the setuid sandbox, where 

// a single open directory would bypass the entire security model. 

PCHECK(0 == IGNORE_EINTR(close(fd)));

See the original code here:

https://code.google.com/p/chromium/codesearch#chromium/src/base/files/scoped_file.cc&sq=package:chromium&type=cs&l=21

While this sounds reasonable, this check also fires and kills the browser process when some render process crashes or is killed exactly at the moment it does a font operation.

This is so because, on Linux, render processes access font files through the SandboxIPCHandler which they communicate with using sockets.

See SandboxIPCHandler::HandleRequestFromRenderer function:

https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/renderer_host/sandbox_ipc_linux.cc&sq=package:chromium&type=cs&rcl=1439422024&l=125

If renderer dies while this function is being executed, the socket will be invalidated and at the attempt of closing it when going out of the scope (note the |fds| vector of scoped FDs),

close() will fail with EBADF errno and the described check will hit and crash browser process.

However, according to my understanding, this does not have a security implication in this case.

And generally, is there a reason to crash here if close() has failed due to the fact that FD is no longer valid? Are there any situations where it does not mean that FD (socket) has been closed somehow/somewhere else before?

If not, would it make sense to change this check to the following?

PCHECK(0 == IGNORE_EINTR(close(fd)) || errno = EBADF);

-- 

Best regards,

Krzysztof Olczyk

Opera Software ASA

TV & Devices

[Thiemo Nagel]

If renderer dies while this function is being executed, the socket will be invalidated and at the attempt of closing it when going out of the scope (note the |fds| vector of scoped FDs), close() will fail with EBADF errno and the described check will hit and crash browser process. However, according to my understanding, this does not have a security implication in this case.

I agree. Dying on every close() error seems to be throwing out the baby with the bathwater. There seems to be plenty scenarios in which failing to close() has no security impact and crashing the browser causes needless grief. (There are filesystems that report error on close() as part of normal operation, for example AFS iirc.) 

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Mark Mentovai]

Absolutely not. If close() can fail with EBADF, it means the file descriptor was available for reuse. If it was reused, ScopedFD would close an unrelated object upon going out of scope. This sort of bug is especially difficult to track down. Failing on EBADF provides an opportunity to catch the problem when the stars align such that the file descriptor isn't reused.

Receiving EBADF at close() means that you thought you owned a file descriptor but actually didn't. This indicates a serious problem. We do want this to be fatal.

If some other operation closes a file descriptor, we need to be aware of it. We have to avoid the double-close().

--

[David Benjamin]

Put another way, closing a stale fd number is *exactly* the same as calling free() on a stale pointer. It is not okay for a malloc implementation to sanity-check internal data structures and silently do nothing in that situation; every such situation could lose the reuse race with dire consequences.

Have you observed the problem you describe? I'm not very familiar with recvmsg's fd-passing API, but that seems fishy. fd tables are local to a process, so if the kernel gives you fd numbers, they should presumably be in your fd table. If the process that sent them to you then crashes, your fd table entries should still remain.

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.

[Krzysztof Olczyk]

On Thu, Aug 13, 2015 at 3:56 PM, David Benjamin <davi...@chromium.org> wrote:

Have you observed the problem you describe? I'm not very familiar with recvmsg's fd-passing API, but that seems fishy. fd tables are local to a process, so if the kernel gives you fd numbers, they should presumably be in your fd table. If the process that sent them to you then crashes, your fd table entries should still remain.

Yes, we have some automated testing bots here and one of the tests

crashes/kills the renderer and asserts the browser itself remains stable.

We have observed the browser process sometimes crashes as well,

with the following log/callstack:

[8929:8934:0625/205632:4521117885717:FATAL:scoped_file.cc(29)] Check failed: 0 == IGNORE_EINTR(close(fd)). : Bad file descriptor

Received signal 6

#8 0x7f797d454119 base::internal::ScopedFDCloseTraits::Free()

#9 0x7f797cb13dad content::SandboxIPCHandler::HandleRequestFromRenderer()

#10 0x7f797cb140ef content::SandboxIPCHandler::Run()

#11 0x7f797d49810d base::DelegateSimpleThread::Run()

#12 0x7f797d4980a0 base::SimpleThread::ThreadMain()

#13 0x7f797d4943f8 base::(anonymous namespace)::ThreadFunc()

#14 0x7f797baf9182 start_thread

#15 0x7f797b52230d clone

The file descriptors are received over sockets using recvmsg() and accessing ancillary data.

I do not know details on how this works and if it is expected that so transfered FDs can become invalid when original process dies. If so, this would imply we should not use ScopedFD for FDs received in such a way.

Any experts on sending FDs over sockets in Linux?

[David Benjamin]

To be clear, if that's the case, that would imply that Linux and POSIX have a bug, not that this API shouldn't return ScopedFD. For the same reason this kind of API is completely wrong:

  // Returns a pointer to a newly-allocated Foo object. Depending on a race condition

  // with the renderer crashing, the resulting pointer may or may not already be freed.

  Foo* DoStuffAndReturnAFoo();

I think it's much more likely something else is going on.
 

[Markus Gutschke (顧孟勤)]

I agree with early comments in this thread that close() returning EBADF is just about as bad as a double-free. That probably should trigger a fatal CHECK(). We must never call close() with an invalid file descriptor.

It is less obvious what should happen for other errno values, as close()'s behavior is surprisingly underspecified in POSIX. In particular, I believe the standard is silent on whether EINTR means that the file descriptor has been released or whether it is still live, and I believe EINTR is rare for close(), but can in fact happen. I cc'd @jln because he spent a good while reading the Linux sources at one point, trying to answer this question.

If I recall correctly, on Linux (but not necessarily on other POSIX systems), the file descriptor is always closed after returning from close(), even if close() signals an error condition. In other words, close()'s exit code is informative for diagnostic purposes, but there is absolutely nothing a program can do when it receives the error.

In particular, this means, on Linux "IGNORE_EINTR(close(fd))" is actually a dangerous bug. If it ever loops, it is possible the code ends up closing an unrelated file descriptor that had just been opened in a separate thread! On the other hand, close() returning EINTR could very well explain why Krzysztof occasionally sees EBADF; it would just be an otherwise harmless side effect of the bogus IGNORE_EINTR() wrapper. If this theory is true, then removing the wrapper should fix the problem: PCHECK(0 == close(fd) || errno == EINTR);

I don't want to stick out my head and unconditionally make this recommendation without another audit of the kernel source, though, as things are a little more complicated on Linux. Linux can have file descriptors that refer to all sorts of non-standard objects including but not limited to FUSE file systems. I hope @jln remembers whether the statement about close() always closing is generally true, or if it is only true for "normal" file systems.

Furthermore, I vaguely recall that Linux uses close()'s exit code to report on things such as errors resulting from delayed allocation. In other words, write() could return success but not actually flush the data to disk. When close() is called a little while later, data gets flushed and now triggers an error, because the file system filled up in the meantime. It is unclear, whether this should constitute a fatal error that should trigger a CHECK() failure. So, maybe, the code needs to be relaxed even more: PCHECK(0 == close(fd) || errno != EBADF);

Markus

--

[Paweł Hajdan, Jr.]

See https://code.google.com/p/chromium/issues/detail?id=269623 and https://codereview.chromium.org/100253002/ for some context about IGNORE_EINTR. Looks like it indeed shouldn't be used with close.

Paweł

[Krzysztof Olczyk]

Hi all,

Yes, I understand and also see the correspondence between this and double-free case.

And indeed, the case I'm seeing may be explained by getting EINTR and then retrying to close the FD.

Thanks Pawel for pointing me to the issue and CL which address this problem in other places. Looks like this one was not hit by the sed command of CL author. I will post a new CL to fix this up.

-- 

Best regards,

Krzysztof Olczyk

Opera Software ASA

TV & Devices

[Krzysztof Olczyk]

I took a look at scoped_file.cc code again and it looks like it already uses IGNORE_EINTR (not HANDLE_EINTR), so it should ignore EINTR already and it must be something else I am seeing.

Sorry for confusion.

-- 

Best regards,

Krzysztof Olczyk

Opera Software ASA

TV & Devices

[Thiemo Nagel]

In particular, this means, on Linux "IGNORE_EINTR(close(fd))" is actually a dangerous bug. If it ever loops, it is possible the code ends up closing an unrelated file descriptor that had just been opened in a separate thread!

IGNORE_EINTR() never loops, it just turns [return -1, errno==EINTR] into [return 0]. Probably you're confusing this with HANDLE_EINTR() which indeed does loop.

Furthermore, I vaguely recall that Linux uses close()'s exit code to report on things such as errors resulting from delayed allocation. In other words, write() could return success but not actually flush the data to disk. When close() is called a little while later, data gets flushed and now triggers an error, because the file system filled up in the meantime.

Yeah, and I vaguely remember that close() can fail on AFS, either if the network is flaky or if the token has expired. It's not obvious to me why blowing up Chrome is the right thing to do in these cases. 

On Fri, Aug 14, 2015 at 10:45 PM, Markus Gutschke (顧孟勤) <mar...@chromium.org> wrote:

[Tomasz Mikolajewski]

This seem to be related to crbug.com/507529 which is about a crash
when plugging and unplugging a USB device.
You guys said that dangerous to close an invalid fd. I don't have much
expertise here, but IIUC in case of an unplugged USB device, we may
end up on an invalid fd. It would be reasonable that it returns EIO.
In such case should we confirm that the error is actually EBADF? The
Markus's suggestion sounds like a good candidate: PCHECK(0 ==
close(fd) || errno != EBADF);

[Scott Hess]

On Wed, Oct 28, 2015 at 6:39 PM, 'Tomasz Mikolajewski' via Chromium-dev <chromi...@chromium.org> wrote:

This seem to be related to crbug.com/507529 which is about a crash
when plugging and unplugging a USB device.
You guys said that dangerous to close an invalid fd. I don't have much
expertise here, but IIUC in case of an unplugged USB device, we may
end up on an invalid fd. It would be reasonable that it returns EIO.
In such case should we confirm that the error is actually EBADF? The
Markus's suggestion sounds like a good candidate: PCHECK(0 ==
close(fd) || errno != EBADF);

I don't follow - how does the fd become invalid in this case?  Removing a device out from under an open file descriptor should cause all sorts of errors to happen if you try to do anything with the file, but if the actual file descriptor becomes invalid asynchronously, that is a serious OS bug.  The file descriptor should only become invalid if you make a system call which allows for the file descriptor to become invalid, such as close(2).  I can't think of any calls which could allow that to happen as a side effect, but if there are they should have return codes and errno combinations which appropriately indicate what happened.

Why is this a bug?  Because otherwise it is not possible to write safe code.  Any invalid file descriptor could be replaced by a valid file descriptor referring to some other object.

-scott

[Tomasz Mikolajewski]

Let me clarify. By invalid, I said that close() would fail with an
error. According to manual it may happen:

http://pubs.opengroup.org/onlinepubs/9699919799/functions/close.html
"If close() is interrupted by a signal that is to be caught, it shall
return -1 with errno set to [EINTR] and the state of fildes is
unspecified. If an I/O error occurred while reading from or writing to
the file system during close(), it may return -1 with errno set to
[EIO]; if this error is returned, the state of fildes is unspecified."

It makes a lot of sense for buffered writing on USB devices. write()
will succeed, then close() will fail trying to flush the data. If this
is true, then we should ignore EIO for close(). Does it make any
sense?

[Scott Hess]

My point is that if the file descriptor was valid, then close() should not return an error with errno==EBADF.  If the filedescriptor is invalid, then the code should not call close() in the first place.  Basically there is no way to correctly ignore EBADF from close() in a multi-threaded program (and I'd argue about a single-threaded program, but when multi-threaded, it's simply not workable, any other thread could snipe that file descriptor from under you and you'll be closing their handle).

DAMMIT.  Apologies, I completely read your test as "Either close succeeds, or the error is EBADF".  You're testing "Either close succeeds, or the error is something other than EBADF".  You have my mightily-embarrassed blessing.

-scott