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