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 <jwak...@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 --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;
}
