From: Jonathan Wakely <jwakely@redhat.com>
To: Martin Sebor <msebor@gmail.com>
Cc: libstdc++ <libstdc++@gcc.gnu.org>, gcc-patches <gcc-patches@gcc.gnu.org>
Subject: Re: [patch] libstdc++/67173 Fix filesystem::canonical for Solaris 10.
Date: Thu, 17 Sep 2015 14:38:00 -0000 [thread overview]
Message-ID: <20150917143707.GC2969@redhat.com> (raw)
In-Reply-To: <20150917111615.GH2631@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 2717 bytes --]
On 17/09/15 12:16 +0100, 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.
This patch sets ELOOP after following 40 symlinks.
I can also move the exists(result, ec) check to the end, because the
is_symlink(result, ec) call will already check for existence on every
iteration that adds a component to the result.
I've also simplified the error handling (when exists(p, ec) fails it
sets ENOENT anyway) and moved !ec into the loop condition, rather than
using 'fail(e); break;' on errors.
I'm quite happy with this version now.
[-- Attachment #2: canonical.patch --]
[-- Type: text/plain, Size: 2481 bytes --]
diff --git a/libstdc++-v3/src/filesystem/ops.cc b/libstdc++-v3/src/filesystem/ops.cc
index b5c8eb9..95146bf 100644
--- a/libstdc++-v3/src/filesystem/ops.cc
+++ b/libstdc++-v3/src/filesystem/ops.cc
@@ -98,6 +98,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
@@ -119,18 +120,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();
@@ -138,18 +130,17 @@ 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_directory(result, ec))
- {
- fail(ENOTDIR);
- break;
- }
+ if (!is_directory(result, ec) && !ec)
+ ec.assign(ENOTDIR, std::generic_category());
}
else if (f.compare("..") == 0)
{
@@ -166,27 +157,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())
+ if (--max_allowed_symlinks == 0)
+ ec.assign(ELOOP, std::generic_category());
+ else
{
- result = link.root_path();
- link = link.relative_path();
+ if (link.is_absolute())
+ {
+ result = link.root_path();
+ link = link.relative_path();
+ }
+ else
+ result.remove_filename();
+
+ cmpts.insert(cmpts.begin(), link.begin(), link.end());
}
- else
- result.remove_filename();
-
- 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;
}
next prev parent reply other threads:[~2015-09-17 14:37 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-09-11 14:23 Jonathan Wakely
2015-09-11 18:05 ` Martin Sebor
2015-09-12 10:39 ` Jonathan Wakely
2015-09-12 19:49 ` Martin Sebor
2015-09-12 22:00 ` Martin Sebor
2015-09-16 14:52 ` Jonathan Wakely
2015-09-16 16:05 ` Jonathan Wakely
2015-09-16 16:11 ` Jonathan Wakely
2015-09-16 17:38 ` Martin Sebor
2015-09-16 19:02 ` Jonathan Wakely
2015-09-16 22:17 ` Martin Sebor
2015-09-16 22:23 ` Jonathan Wakely
2015-09-16 23:51 ` Martin Sebor
2015-09-17 11:31 ` Jonathan Wakely
2015-09-17 11:33 ` Jonathan Wakely
2015-09-17 14:38 ` Jonathan Wakely [this message]
2015-09-17 15:40 ` Martin Sebor
2015-09-23 12:14 ` Jonathan Wakely
2015-09-16 23:42 ` Jonathan Wakely
2015-09-17 15:36 ` Jonathan Wakely
2015-09-17 19:27 ` Andreas Schwab
2015-09-17 22:23 ` Jonathan Wakely
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20150917143707.GC2969@redhat.com \
--to=jwakely@redhat.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=libstdc++@gcc.gnu.org \
--cc=msebor@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).