diff mbox

libstdc++/67173 Fix filesystem::canonical for Solaris 10.

Message ID 20150923114111.GX2969@redhat.com
State New
Headers show

Commit Message

Jonathan Wakely Sept. 23, 2015, 11:41 a.m. UTC 
On 17/09/15 09:37 -0600, Martin Sebor wrote:
>On 09/17/2015 05:16 AM, Jonathan Wakely wrote:
>>On 16/09/15 17:42 -0600, Martin Sebor wrote:
>>>I see now the first exists test will detect symlink loops in
>>>the original path. But I'm not convinced there isn't a corner
>>>case that's subject to a TOCTOU race condition between the first
>>>exists test and the while loop during which a symlink loop can
>>>be introduced.
>>>
>>>Suppose we call the function with /foo/bar as an argument and
>>>the path exists and contains no symlinks. result is / and cmpts
>>>is set to { foo, bar }. Just as the loop is entered, /foo/bar
>>>is replaced with a symlink containing /foo/bar. The loop then
>>>proceeds like so:
>>>
>>>1. The first iteration removes foo from cmpts and sets result
>>>to /foo. cmpts is { bar }.
>>>
>>>2. The second iteration removes bar from cmpts, sets result to
>>>/foo/bar, determines it's a symlink, reads its contents, sees
>>>it's an absolute pathname and replaces result with /. It then
>>>inserts the symlink's components { foo, bar } into cmpts. cmpts
>>>becomes { foo, bar }. exists(result) succeeds.
>>>
>>>3. The next iteration of the loop has the same initial state
>>>as the first.
>>>
>>>But I could have very easily missed something that takes care
>>>of this corner case. If I did, sorry for the false alarm!
>>
>>No, you're right. The TS says such filesystem races are undefined:
>>http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4099.html#fs.race.behavior
>>
>>but it would be nice to fail gracefully rather than DOS the
>>application.
>>
>>The simplest approach would be to increment a counter every time we
>>follow a symlink, and if it reaches some limit decide something is
>>wrong and fail with ELOOP.
>>
>>I don't see how anything else can be 100% bulletproof, because a truly
>>evil attacker could just keep altering the contents of symlinks so we
>>keep ping-ponging between two or more paths. If we keep track of paths
>>we've seen before the attacker could just keep changing the contents
>>to a unique path each time, that initially exists as a file, but by
>>the time we get to is_symlink() its become a symlink to a new path.
>>
>>So if we use a counter, what's a sane maximum? Is MAXSYMLINKS in
>><sys/param.h> the value the kernel uses? 20 seems quite low, I was
>>thinking of a much higher number.
>
>Yes, it is a corner case, and it's not really avoidable in the case
>of hard links. For symlinks, POSIX defines the SYMLOOP_MAX constant
>as the maximum, with the _SC_SYMLOOP_MAX and _PC_SYMLOOP_MAX
>sysconf and pathconf variables. Otherwise 40 seems reasonable.
>
>With this, I'll let you get back to work -- I think we've beat this
>function to death ;)

Here's what I committed. Similar to the last patch, but using the new
is_dot and is_dotdot helpers.
commit 8128173a00c234ccf34e258115747fa0e3b4457a
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Wed Sep 23 02:00:57 2015 +0100

    Limit number of symlinks that canonical() will resolve
    
    	* src/filesystem/ops.cc (canonical): Simplify error handling and
    	limit number of symlinks that can be resolved.
diff mbox

Patch

diff --git a/libstdc++-v3/src/filesystem/ops.cc b/libstdc++-v3/src/filesystem/ops.cc
index 5ff8120..7b261fb 100644
--- a/libstdc++-v3/src/filesystem/ops.cc
+++ b/libstdc++-v3/src/filesystem/ops.cc
@@ -116,6 +116,7 @@  fs::canonical(const path& p, const path& base, error_code& ec)
 {
   const path pa = absolute(p, base);
   path result;
+
 #ifdef _GLIBCXX_USE_REALPATH
   char_ptr buf{ nullptr };
 # if _XOPEN_VERSION < 700
@@ -137,18 +138,9 @@  fs::canonical(const path& p, const path& base, error_code& ec)
     }
 #endif
 
-  auto fail = [&ec, &result](int e) mutable {
-      if (!ec.value())
-	ec.assign(e, std::generic_category());
-      result.clear();
-  };
-
   if (!exists(pa, ec))
-    {
-      fail(ENOENT);
-      return result;
-    }
-  // else we can assume no unresolvable symlink loops
+    return result;
+  // else: we know there are (currently) no unresolvable symlink loops
 
   result = pa.root_path();
 
@@ -156,20 +148,19 @@  fs::canonical(const path& p, const path& base, error_code& ec)
   for (auto& f : pa.relative_path())
     cmpts.push_back(f);
 
-  while (!cmpts.empty())
+  int max_allowed_symlinks = 40;
+
+  while (!cmpts.empty() && !ec)
     {
       path f = std::move(cmpts.front());
       cmpts.pop_front();
 
-      if (f.compare(".") == 0)
+      if (is_dot(f))
 	{
-	  if (!is_directory(result, ec))
-	    {
-	      fail(ENOTDIR);
-	      break;
-	    }
+	  if (!is_directory(result, ec) && !ec)
+	    ec.assign(ENOTDIR, std::generic_category());
 	}
-      else if (f.compare("..") == 0)
+      else if (is_dotdot(f))
 	{
 	  auto parent = result.parent_path();
 	  if (parent.empty())
@@ -184,27 +175,30 @@  fs::canonical(const path& p, const path& base, error_code& ec)
 	  if (is_symlink(result, ec))
 	    {
 	      path link = read_symlink(result, ec);
-	      if (!ec.value())
+	      if (!ec)
 		{
-		  if (link.is_absolute())
-		    {
-		      result = link.root_path();
-		      link = link.relative_path();
-		    }
+		  if (--max_allowed_symlinks == 0)
+		    ec.assign(ELOOP, std::generic_category());
 		  else
-		    result.remove_filename();
+		    {
+		      if (link.is_absolute())
+			{
+			  result = link.root_path();
+			  link = link.relative_path();
+			}
+		      else
+			result.remove_filename();
 
-		  cmpts.insert(cmpts.begin(), link.begin(), link.end());
+		      cmpts.insert(cmpts.begin(), link.begin(), link.end());
+		    }
 		}
 	    }
-
-	  if (ec.value() || !exists(result, ec))
-	    {
-	      fail(ENOENT);
-	      break;
-	    }
 	}
     }
+
+  if (ec || !exists(result, ec))
+    result.clear();
+
   return result;
 }