[cfe-dev] [libc++][RFC] Implementing Directory Entry CachingforFilesystem

[Billy O'Neal via cfe-dev]

> We're in agreement here that eliminating the TOCTOU is the only way to
> eliminate the vulnerability. However, much of security is about taking
> many small steps to mitigate security issues and reduce attack vectors
> as part of defense-in-depth.

Defense in depth is about taking a system that you think is secure, and building additional barriers such that compromise of one part of that system does not compromise a machine. Defense in depth is not a thing when the original thing is already full of holes. There is no defense in depth thing you can do to an API like exists to avoid this problem.

Billy3

From: Aaron Ballman
Sent: Wednesday, July 18, 2018 11:03 AM
To: Geoffrey Romer
Cc: Billy O'Neal; Eric Fiselier; Clang Dev; Marshall Clow; Titus Winters
Subject: Re: [cfe-dev] [libc++][RFC] Implementing Directory Entry CachingforFilesystem

On Wed, Jul 18, 2018 at 1:46 PM, Geoffrey Romer <gromer at google.com> wrote:
>
> On Wed, Jul 18, 2018 at 9:52 AM Aaron Ballman <aaron at aaronballman.com>
> wrote:
>>
>> On Tue, Jul 17, 2018 at 5:53 PM, Geoffrey Romer <gromer at google.com> wrote:
>> >
>> >
>> > On Tue, Jul 17, 2018 at 10:40 AM Billy O'Neal <billy.oneal at gmail.com>
>> > wrote:
>> >>
>> >> Does anyone have an example of something that is secure without caching
>> >> that is insecure with caching? All of the examples posted as
>> >> problematic are
>> >> still insecure.
>> >
>> >
>> > +1. By my reading, different caching decisions can only make TOCTOU
>> > problems
>> > more or less acute; caching can't introduce TOCTOU problems that don't
>> > already exist.
>>
>> That is my belief as well. However, I don't find the argument to be
>> persuasive, so it's a -1 from me.
>>
>> > From that point of view, caching could actually be helpful,
>> > by making the TOCTOU problems more obvious.
>>
>> Caching is hidden from the user's view, and I fail to see how
>> something hidden from the user's view will will make anything more
>> obvious. What caching does do is extend the length of time over which
>> TOCTOU bugs can occur, which is worse behavior from a security
>> perspective. Users will definitely introduce exciting TOCTOU bugs into
>> their code with or without the caching mechanism, but I'm worried
>> about worsening the effects in practice.
>
>
> Generally speaking, I would expect "worsening the effects" of the bugs to
> make those bugs more obvious.

I don't think that applies to cases that are timing based. You have to
know to be actively testing that scenario to notice the worsening
effects, at which point you'd likely have already fixed the TOCTOU.

>> > The only change we could
>> > meaningfully make here is to give users some conditions under which
>> > non-modifying operations on the same file are guaranteed to reflect a
>> > single
>> > self-consistent state of the file (which may or may not be the present
>> > state), but the benefit of that seems minimal.
>> >
>> > I think this ship sailed when we decided to standardize a TOCTOU-prone
>> > API;
>> > all we can do now is try to educate users about what not to use
>> > std::filesystem for (e.g. anything remotely related to security).
>>
>> "This doesn't introduce new security issues, it just makes existing
>> ones worse" is not an argument that leaves me with warm, fuzzy
>> feelings. The goal is obviously to prevent the TOCTOU bugs in the
>> first place, but if we cannot achieve that, we shouldn't exacerbate
>> the TOCTOU problems only because we can't achieve the ideal.
>
>
> That's not so obvious to me. If exacerbating the TOCTOU problems makes
> people more aware of those problems, that could well be a net win.

How is it making more people aware of the problem, though? Silently
exacerbating the issue doesn't mean people are going to become aware
of it when they previously were not, unless you consider "there's a
new CVE out today" being the kind of education that raises awareness,
but I'd hardly call that a net win.

> Conversely, to the extent that the caching design makes it easier to use
> std::filesystem in security-sensitive contexts, that is likely to be a net
> loss, because std::filesystem simply should not be used in
> security-sensitive contexts.

Last time I looked into it, Clang and LLVM had quite a few TOCTOU
bugs. No one was compelled to fix them because hey, we're just a
compiler, so who cares -- it's not a security-sensitive context. Well,
until people started running it on web servers... (It's been a number
of years since I looked, and we may have improved this since clangd
came along, but the example still holds.)

The trouble is that "security-sensitive context" is a moving target
for a whole lot of code.

> Keep in mind, security exploits are generally not statistical in nature;
> halving the time window for a TOCTOU attack does not halve your
> vulnerability, because the components of the attack are not occurring at
> random; they're being induced by an attacker. Shortening the time window may
> have some security benefit on the margins in terms of increasing the effort
> required for a successful exploit, but that's highly contingent and
> uncertain. I expect the security benefits of eliminating a TOCTOU
> vulnerability (e.g. by using a safer filesystem API) to dwarf the benefits
> of almost any reduction in the vulnerability time window.

We're in agreement here that eliminating the TOCTOU is the only way to
eliminate the vulnerability. However, much of security is about taking
many small steps to mitigate security issues and reduce attack vectors
as part of defense-in-depth.

I guess my point is: let's not be quick to say "well, we can't solve
this to the ideal, so we can do whatever we want in the name of
performance." As history has unkindly demonstrated, that way lies many
years of CVEs. I think a lit review of TOCTOU papers might be
interesting to see if anyone has explored the ramifications of a
widened time window. I'll try to do that when I have a spare moment,
but if anyone gets to it before me, I'd love to hear the results.

~Aaron

>
>>
>>
>> ~Aaron
>>
>> >
>> >
>> >>
>> >>
>> >>
>> >> Billy3
>> >>
>> >>
>> >>
>> >> From: Aaron Ballman
>> >> Sent: Tuesday, July 17, 2018 4:46 AM
>> >> To: Eric Fiselier
>> >> Cc: clang developer list; Marshall Clow; Titus Winters; Billy O'Neal;
>> >> Geoffrey Romer
>> >> Subject: Re: [cfe-dev] [libc++][RFC] Implementing Directory Entry
>> >> Caching
>> >> forFilesystem
>> >>
>> >>
>> >>
>> >> Thank you for putting this together, I greatly appreciate it!
>> >>
>> >>
>> >>
>> >> Out of curiosity, is the caching behavior something you expect you'll
>> >>
>> >> make configurable for users of libc++ (for instance, a macro to enable
>> >>
>> >> or disable caching), or are there issues preventing such an
>> >>
>> >> implementation? My feeling is that we're going to have the usual push
>> >>
>> >> and pull between security and performance where different users will
>> >>
>> >> have different (and equally valid) use cases in mind justifying the
>> >>
>> >> need for the performance provided by caching or the security red flags
>> >>
>> >> enhanced by not caching. We should obviously pick a good default
>> >>
>> >> behavior (and can argue over the definition of "good" in that
>> >>
>> >> context), but I think no matter what option we pick, users will want
>> >>
>> >> the other option to be available.
>> >>
>> >>
>> >>
>> >> ~Aaron
>> >>
>> >>
>> >>
>> >> On Mon, Jul 16, 2018 at 10:39 PM, Eric Fiselier via cfe-dev
>> >>
>> >> <cfe-dev at lists.llvm.org> wrote:
>> >>
>> >> > Hi All,
>> >>
>> >> >
>> >>
>> >> >
>> >>
>> >> > I have a couple of questions and concerns regarding implementations
>> >> > of
>> >>
>> >> > P0317R1 [1], which I would like help from the community answering.
>> >> > The
>> >> > paper
>> >>
>> >> > changes `directory_entry` to cache a number of attributes, including
>> >> > file
>> >>
>> >> > status,  permissions, file size, number of hard links, last write
>> >> > time,
>> >> > and
>> >>
>> >> > more. For reference, this is the interface of `directory_entry`
>> >> > before
>> >> > this
>> >>
>> >> > paper [2], and this is the interface after [3].
>> >>
>> >> >
>> >>
>> >> >
>> >>
>> >> > The implementation has a lot of latitude over which attributes it
>> >> > caches, if
>> >>
>> >> > any, and how it caches them. As I'll explain below, each different
>> >> > approach
>> >>
>> >> > can cause non-obvious behavior, possibly leading to TOCTOU bugs and
>> >> > security
>> >>
>> >> > vulnerabilities!
>> >>
>> >> >
>> >>
>> >> >
>> >>
>> >> > My question for the community is this:
>> >>
>> >> >
>> >>
>> >> >
>> >>
>> >> > Given the considerations below, what should libc++ choose to do?
>> >>
>> >> >
>> >>
>> >> >
>> >>
>> >> > 1. Cache nothing?
>> >>
>> >> >
>> >>
>> >> > 2. Cache as much as possible, when possible?
>> >>
>> >> >
>> >>
>> >> > 3. Something in between?
>> >>
>> >> >
>> >>
>> >> >
>> >>
>> >> > I would like to land std::filesystem before the 7.0 branch, and doing
>> >> > so
>> >>
>> >> > requires deciding on which implementation to provide now.
>> >>
>> >> >
>> >>
>> >> >
>> >>
>> >> > Note that this paper only considers POSIX based implementations of
>> >>
>> >> > filesystem.
>> >>
>> >> >
>> >>
>> >> > TOCTOU Violations
>> >>
>> >> >
>> >>
>> >> > ============================
>> >>
>> >> >
>> >>
>> >> >
>> >>
>> >> > Time of check to time of use, or TOCTOU, is a class of software bugs
>> >> > caused
>> >>
>> >> > by changes in a system between the time a condition is checked, and
>> >> > the
>> >> > time
>> >>
>> >> > the result is used [4]. If libc++ chooses to do any caching in
>> >>
>> >> > directory_entry, it will be vulnerable to this. If libc++ chooses to
>> >> > do
>> >> > no
>> >>
>> >> > caching at all, it will also be vulnerable to this but in different
>> >>
>> >> > scenarios.
>> >>
>> >> >
>> >>
>> >> >
>> >>
>> >> > Most importantly, whichever approach is taken, libc++ will bare some
>> >>
>> >> > responsibility for the TOCTOU bugs it allows users to write
>> >> > accidently.
>> >> > So
>> >>
>> >> > it’s imperative we consider all approaches carefully.
>> >>
>> >> >
>> >>
>> >> >
>> >>
>> >> > Let’s take a simple example:
>> >>
>> >> >
>> >>
>> >> >
>> >>
>> >> > void remove_symlinks_in_dir(path p) {
>> >>
>> >> >
>> >>
>> >> >  for (directory_entry ent : directory_iterator(p)) {
>> >>
>> >> >
>> >>
>> >> >    if (ent.is_symlink())
>> >>
>> >> >
>> >>
>> >> >      remove(ent);
>> >>
>> >> >
>> >>
>> >> >  }
>> >>
>> >> >
>> >>
>> >> > }
>> >>
>> >> >
>> >>
>> >> >
>> >>
>> >> > Above, a TOCTOU violation may occur if another process acts on the
>> >> > file
>> >>
>> >> > referenced by `ent`, changing the attributes between the point the
>> >>
>> >> > directory_entry was created and the point at which the `is_symlink`
>> >>
>> >> > condition is checked.
>> >>
>> >> >
>> >>
>> >> >
>> >>
>> >> > At this point it's important to note that <filesystem> interface is,
>> >> > in
>> >>
>> >> > general, vulnerable to TOCTOU --- even without directory_entry
>> >> > caching.
>> >> > For
>> >>
>> >> > example:
>> >>
>> >> >
>> >>
>> >> >
>> >>
>> >> > void remove_symlinks_in_dir(path p) {
>> >>
>> >> >
>> >>
>> >> >  for (directory_entry ent : directory_iterator(p)) {
>> >>
>> >> >
>> >>
>> >> >    if (is_symlink(ent))
>> >>
>> >> >
>> >>
>> >> >      remove(ent);
>> >>
>> >> >
>> >>
>> >> >  }
>> >>
>> >> >
>> >>
>> >> > }
>> >>
>> >> >
>> >>
>> >> >
>> >>
>> >> > In the above case, a TOCTOU violation still occurs, since changes to
>> >> > `ent`
>> >>
>> >> > can take place between the check and the call to `remove`. However,
>> >> > at
>> >> > least
>> >>
>> >> > the above example makes it clear *when* the check is occuring.
>> >>
>> >> >
>> >>
>> >> >
>> >>
>> >> > Simply eliminating caching will not prevent TOCTOU violations, but,
>> >> > in
>> >> > this
>> >>
>> >> > particular case, it would make them harder to write and make it more
>> >> > obvious
>> >>
>> >> > where they may occur. Having a cache both extends the period of time
>> >> > between
>> >>
>> >> > the check and the use, as well as hiding when the check actually
>> >> > occurs.
>> >>
>> >> >
>> >>
>> >> >
>> >>
>> >> > Perhaps the above concessions make it worthwhile to support a more
>> >> > efficient
>> >>
>> >> > interface with caching and fewer file system accesses?
>> >>
>> >> >
>> >>
>> >> >
>> >>
>> >> > If we do choose to cache some attributes, then, at minimum, it should
>> >> > be
>> >>
>> >> > obvious to the user which values are cached and which are not. The
>> >> > following
>> >>
>> >> > section will explore the feasibility of this.
>> >>
>> >> >
>> >>
>> >> >
>> >>
>> >> > Directory Iteration VS Refresh
>> >>
>> >> >
>> >>
>> >> > ==================================
>> >>
>> >> >
>> >>
>> >> >
>> >>
>> >> > The paper's intention is to allow attributes returned during
>> >> > directory
>> >>
>> >> > iteration to be accessed by the user without additional calls to the
>> >>
>> >> > underlying filesystem.
>> >>
>> >> >
>> >>
>> >> >
>> >>
>> >> > The cache is populated two different ways:
>> >>
>> >> >
>> >>
>> >> > During directory iteration, the cache is populated with only the
>> >> > attributes
>> >>
>> >> > returned by directory iteration function. With POSIX `readdir`, only
>> >> > the
>> >>
>> >> > file type as returned by `lstat` is available.
>> >>
>> >> >
>> >>
>> >> > Using the `refresh` function, either called directly by the user, or
>> >>
>> >> > implicitly by the constructors or modifying methods (Ex. `assign`).
>> >>
>> >> > `refresh` has the ability to fully populate the cache (but not
>> >> > atomically in
>> >>
>> >> > some cases, see the symlink considerations below).
>> >>
>> >> >
>> >>
>> >> >
>> >>
>> >> > Note that const accessors may not update the cache.
>> >>
>> >> >
>> >>
>> >> >
>> >>
>> >> > Therefore the state of the cache depends on whether the
>> >> > `directory_entry`
>> >>
>> >> > was created during directory iteration or by the
>> >> > `directory_entry(path&)`
>> >>
>> >> > constructor, and whether there have been any intervening calls to
>> >> > `refresh`
>> >>
>> >> > or any other modifying member functions.
>> >>
>> >> >
>> >>
>> >> >
>> >>
>> >> > The following case demonstrates a surprising TOCTOU violation, only
>> >> > when
>> >> > the
>> >>
>> >> > `directory_entry` cache was populated during directory iteration, and
>> >> > not by
>> >>
>> >> > a call to refresh:
>> >>
>> >> >
>> >>
>> >> >
>> >>
>> >> > // Assume the filesystem supports permissions on symlinks.
>> >>
>> >> >
>> >>
>> >> > bool is_readable_symlink(directory_entry ent) {
>> >>
>> >> >
>> >>
>> >> >  if (!ent.is_symlink())
>> >>
>> >> >
>> >>
>> >> >    return false;
>> >>
>> >> >
>> >>
>> >> >  // We should have the file_status for the symlink cached, right?
>> >>
>> >> >
>> >>
>> >> >  file_status st = ent.symlink_status();
>> >>
>> >> >
>> >>
>> >> >  // Nope! Only when `refresh` has been called are the perms cached.
>> >>
>> >> >
>> >>
>> >> >  assert(is_symlink(st)); // May fail!
>> >>
>> >> >
>> >>
>> >> >  return (st.permissions() & perms::others_read) != perms::none;
>> >>
>> >> >
>> >>
>> >> > }
>> >>
>> >> >
>> >>
>> >> >
>> >>
>> >> > The above example is intended to demonstrate that the different
>> >> > accessors
>> >>
>> >> > may return a mix of cached and uncached values. Here `is_symlink()`
>> >> > is
>> >>
>> >> > cached, but `symlink_status()` may not be.
>> >>
>> >> >
>> >>
>> >> > If the symlink has been replaced with a regular file, then the result
>> >> > of
>> >>
>> >> > this function is potentially dangerous nonsense.
>> >>
>> >> >
>> >>
>> >> >
>> >>
>> >> > So what happens when the user calls `refresh()` before accessing the
>> >>
>> >> > attributes? Does this make it safer?
>> >>
>> >> >
>> >>
>> >> >
>> >>
>> >> > The answer depends on the caching behavior of the implementation. and
>> >>
>> >> > whether that behavior provides "atomic" or "non-atomic" attribute
>> >> > access
>> >> > (as
>> >>
>> >> > defined in the next section).
>> >>
>> >> >
>> >>
>> >> >
>> >>
>> >> > Atomic VS Non-Atomic Attribute Access
>> >>
>> >> >
>> >>
>> >> > =============================================
>> >>
>> >> >
>> >>
>> >> >
>> >>
>> >> > As previously described, TOCTOU violations require there to be
>> >> > intervening
>> >>
>> >> > time between the check and the use. We'll call any filesystem
>> >> > function,
>> >> > or
>> >>
>> >> > set of functions, which don’t allow for TOCTOU bugs "atomic". All
>> >> > other
>> >>
>> >> > functions or set of functions are considered "non-atomic".
>> >>
>> >> >
>> >>
>> >> >
>> >>
>> >> > Therefore, a `directory_entry` object can be said to provide "atomic
>> >>
>> >> > attribute access" across all accessors if and only if (A) all of the
>> >>
>> >> > attributes are cached, and (B) the cache was populated atomically. A
>> >>
>> >> > non-caching implementation of `directory_entry` never provides atomic
>> >>
>> >> > attribute access.
>> >>
>> >> >
>> >>
>> >> >
>> >>
>> >> > Let’s consider the consequences using the following example:
>> >>
>> >> >
>> >>
>> >> >
>> >>
>> >> > bool  is_empty_regular_file(path p) {
>> >>
>> >> >
>> >>
>> >> >  directory_entry ent(p);   // Calls refresh()
>> >>
>> >> >
>> >>
>> >> >  return ent.is_regular_file() && ent.file_size() == 0;
>> >>
>> >> >
>> >>
>> >> > }
>> >>
>> >> >
>> >>
>> >> >
>> >>
>> >> > In a non-caching implementation `refresh()` is a nop, and therefore
>> >> > opens
>> >>
>> >> > the user up to TOCTOU issues occuring between the call to
>> >> > `is_symlink()`
>> >> > and
>> >>
>> >> > `is_regular_file()`.
>> >>
>> >> >
>> >>
>> >> >
>> >>
>> >> > In a fully-caching implementation, which provides atomic attribute
>> >> > access,
>> >>
>> >> > no such problem occurs.
>> >>
>> >> >
>> >>
>> >> >
>> >>
>> >> > In this case it seems preferable for libc++ to provide a
>> >> > fully-caching
>> >>
>> >> > implementation with "atomic attribute access". But is this possible?
>> >> > The
>> >>
>> >> > answer is "sometimes, but not always" as we'll see below.
>> >>
>> >> >
>> >>
>> >> >
>> >>
>> >> > Problems with Symlinks And Refresh()
>> >>
>> >> >
>> >>
>> >> > ===========================================
>> >>
>> >> >
>> >>
>> >> >
>> >>
>> >> > Above we established that if refresh caches at all, then it should do
>> >> > so
>> >>
>> >> > "atomically", retrieving all attributes from the filesystem in a
>> >> > atomic
>> >>
>> >> > manner. Note that for symlinks, some of the cached attributes refer
>> >> > to
>> >> > the
>> >>
>> >> > symlink (is_symlink() and `symlink_status()), while the rest refer to
>> >> > the
>> >>
>> >> > linked entity (is_regular_file(), file_size(), etc).
>> >>
>> >> >
>> >>
>> >> >
>> >>
>> >> > When the directory entry does not refer to a symlink, a single call
>> >> > to
>> >>
>> >> > `lstat` provides enough information to fully populate the cache
>> >> > atomically.
>> >>
>> >> > However, when the entity is a symlink, we would need a second call to
>> >> > `stat`
>> >>
>> >> > to determine the properties of the linked file.
>> >>
>> >> >
>> >>
>> >> >
>> >>
>> >> > Therefore, "atomic attribute access" is not possible to guarantee.
>> >>
>> >> >
>> >>
>> >> >
>> >>
>> >> > An implementation may choose to partially populate the caches,
>> >> > omitting
>> >>
>> >> > attributes about the linked entity; or it may choose to fully
>> >> > populate
>> >> > the
>> >>
>> >> > cache non-atomically using a non-atomic series of calls to `lstat`
>> >> > and
>> >>
>> >> > `stat`.
>> >>
>> >> >
>> >>
>> >> >
>> >>
>> >> > The former case obfuscates from the user which attributes are cached
>> >> > and
>> >>
>> >> > which are not, opening them up to TOCTOU violations.
>> >>
>> >> >
>> >>
>> >> >
>> >>
>> >> > Worse yet, the latter case, which fully populates the cache
>> >> > non-atomically,
>> >>
>> >> > potentially commits a TOCTOU violation itself, and this violation is
>> >> > hidden
>> >>
>> >> > from the user, preventing them from knowing it exists or being able
>> >> > to
>> >> > do
>> >>
>> >> > anything about it.
>> >>
>> >> >
>> >>
>> >> >
>> >>
>> >> > Both solutions seem fraught with problems.
>> >>
>> >> >
>> >>
>> >> > Conclusion
>> >>
>> >> >
>> >>
>> >> > ===============
>> >>
>> >> >
>> >>
>> >> >
>> >>
>> >> > I hope to have shown the pros and cons of different directory entry
>> >> > caching
>> >>
>> >> > implementations. Now I need your help deciding what's best for
>> >> > Libc++!
>> >>
>> >> >
>> >>
>> >> >
>> >>
>> >> > Personally, I think the best solution is to revert these changes to
>> >> > the
>> >>
>> >> > standard, since each possible implementation seems broken in one way
>> >> > or
>> >>
>> >> > another. Directory entries should not cache, and directory iterators
>> >> > should
>> >>
>> >> > not attempt to populate the cache incompletely. Afterwards, a better
>> >> > and
>> >>
>> >> > safer proposal can be put forward  which to provides efficient and
>> >> > atomic
>> >>
>> >> > access to multiple attributes for the same entity can be proposed.
>> >>
>> >> >
>> >>
>> >> >
>> >>
>> >> > Unfortunately, it might be to late at this point to revert the paper,
>> >> > even
>> >>
>> >> > if the committee generally agrees with these problems.
>> >>
>> >> >
>> >>
>> >> >
>> >>
>> >> > Thoughts?
>> >>
>> >> >
>> >>
>> >> >
>> >>
>> >> >
>> >>
>> >> > [1]
>> >> > http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2016/p0317r1.html
>> >>
>> >> >
>> >>
>> >> > [2] https://en.cppreference.com/w/cpp/experimental/fs/directory_entry
>> >>
>> >> >
>> >>
>> >> > [3] https://en.cppreference.com/w/cpp/filesystem/directory_entry
>> >>
>> >> >
>> >>
>> >> > [4] https://en.wikipedia.org/wiki/Time_of_check_to_time_of_use
>> >>
>> >> >
>> >>
>> >> >
>> >>
>> >> >
>> >>
>> >> >
>> >>
>> >> >
>> >>
>> >> > _______________________________________________
>> >>
>> >> > cfe-dev mailing list
>> >>
>> >> > cfe-dev at lists.llvm.org
>> >>
>> >> > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>> >>
>> >> >
>> >>
>> >>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20180718/9035c3d5/attachment.html>