From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 31952 invoked by alias); 23 Sep 2015 11:41:16 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 31930 invoked by uid 89); 23 Sep 2015 11:41:15 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=0.1 required=5.0 tests=AWL,BAYES_50,KAM_LAZY_DOMAIN_SECURITY,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=no version=3.3.2 X-Spam-User: qpsmtpd, 2 recipients X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Wed, 23 Sep 2015 11:41:14 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (Postfix) with ESMTPS id D6C19A443E; Wed, 23 Sep 2015 11:41:12 +0000 (UTC) Received: from localhost (ovpn-116-78.ams2.redhat.com [10.36.116.78]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t8NBfCBB030784; Wed, 23 Sep 2015 07:41:12 -0400 Date: Wed, 23 Sep 2015 12:14:00 -0000 From: Jonathan Wakely To: Martin Sebor Cc: libstdc++ , gcc-patches Subject: Re: [patch] libstdc++/67173 Fix filesystem::canonical for Solaris 10. Message-ID: <20150923114111.GX2969@redhat.com> References: <55F469CF.9010503@gmail.com> <20150916144207.GY2631@redhat.com> <55F9A8A8.3060502@gmail.com> <20150916185844.GD2631@redhat.com> <55F9E75F.10602@gmail.com> <20150916221727.GF2631@redhat.com> <55F9FE71.1060901@gmail.com> <20150917111615.GH2631@redhat.com> <55FADE4C.5000404@gmail.com> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="RXc6EO4W1yUvSQ0X" Content-Disposition: inline In-Reply-To: <55FADE4C.5000404@gmail.com> X-Clacks-Overhead: GNU Terry Pratchett User-Agent: Mutt/1.5.23 (2014-03-12) X-SW-Source: 2015-09/txt/msg01734.txt.bz2 --RXc6EO4W1yUvSQ0X Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline Content-length: 2817 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 >> 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. --RXc6EO4W1yUvSQ0X Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="patch-fs-3.txt" Content-length: 2925 commit 8128173a00c234ccf34e258115747fa0e3b4457a Author: Jonathan Wakely 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; } --RXc6EO4W1yUvSQ0X--