Patchwork [4,of,5] context: use a the nofsauditor when matching file in history (issue4749)

login
register
mail settings
Submitter Pierre-Yves David
Date Dec. 3, 2015, 10:01 p.m.
Message ID <65797f84a2cc0317f132.1449180088@marginatus.alto.octopoid.net>
Download mbox | patch
Permalink /patch/11794/
State Accepted
Headers show

Comments

Pierre-Yves David - Dec. 3, 2015, 10:01 p.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@fb.com>
# Date 1449177826 28800
#      Thu Dec 03 13:23:46 2015 -0800
# Node ID 65797f84a2cc0317f1326215701befd382d04299
# Parent  64a5cdedfc87c8c3d47d8147612e62186a5228c4
# EXP-Topic symlink.issue4749
# Available At http://hg.netv6.net/marmoute-wip/mercurial/
#              hg pull http://hg.netv6.net/marmoute-wip/mercurial/ -r 65797f84a2cc
context: use a the nofsauditor when matching file in history (issue4749)

Before this change, asking for file from history (eg: 'hg cat -r 42 foo/bar')
could fail because of the current content of the working copy (eg: current
"foo" being a symlink). As the working copy state have no influence on the
content of the history, we can safely skip these checks.
Matt Mackall - Dec. 5, 2015, 10:35 p.m.
On Thu, 2015-12-03 at 14:01 -0800, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@fb.com>
> # Date 1449177826 28800
> #      Thu Dec 03 13:23:46 2015 -0800
> # Node ID 65797f84a2cc0317f1326215701befd382d04299
> # Parent  64a5cdedfc87c8c3d47d8147612e62186a5228c4
> # EXP-Topic symlink.issue4749
> # Available At http://hg.netv6.net/marmoute-wip/mercurial/
> #              hg pull http://hg.netv6.net/marmoute-wip/mercurial/ -r
> 65797f84a2cc
> context: use a the nofsauditor when matching file in history
> (issue4749)

I would be MUCH happier if the commit message addressed the following
obvious security question:

How are we still confident that we never accidentally use the
nofsauditor with on-disk paths?

-- 
Mathematics is the supreme nostalgia of our time.
Pierre-Yves David - Dec. 6, 2015, 5:33 a.m.
On 12/05/2015 02:35 PM, Matt Mackall wrote:
> On Thu, 2015-12-03 at 14:01 -0800, Pierre-Yves David wrote:
>> # HG changeset patch
>> # User Pierre-Yves David <pierre-yves.david@fb.com>
>> # Date 1449177826 28800
>> #      Thu Dec 03 13:23:46 2015 -0800
>> # Node ID 65797f84a2cc0317f1326215701befd382d04299
>> # Parent  64a5cdedfc87c8c3d47d8147612e62186a5228c4
>> # EXP-Topic symlink.issue4749
>> # Available At http://hg.netv6.net/marmoute-wip/mercurial/
>> #              hg pull http://hg.netv6.net/marmoute-wip/mercurial/ -r
>> 65797f84a2cc
>> context: use a the nofsauditor when matching file in history
>> (issue4749)
>
> I would be MUCH happier if the commit message addressed the following
> obvious security question:
>
> How are we still confident that we never accidentally use the
> nofsauditor with on-disk paths?

woops I did double check, but that data got lost somewhere in my patch 
gardening.

The working copy context class have a different 'match' implementation. 
That implementation still use the repo.auditor will still catch symlink 
traversal.

I've audited all stuff calling "match" and they all go through a ctx in 
a sensible way. The most unclear case was diff which still seemed okay. 
You raised my paranoid level today and I double checked through tests. 
They behave properly.

The odd of someone using the wrong (matching with a changectx for 
operation that will eventually touch the file system) is non-zero 
because you are never sure of what people will do. But I dunno if we can 
fight against that. So I would not commit to "never" for "at this level" 
and "in the future" if someone write especially bad code.

However, as a last defense, the vfs itself is running path auditor in 
all cases outside of .hg/. So I think anything passing the 'matcher' for 
buggy reason would growl at the vfs layer.

Feel free to update the commit message with this above statement.



Have also the tests for `hg diff` to send to the list. I can email a V2 
with the test and an updated message if you want me to.

Patch

diff --git a/mercurial/context.py b/mercurial/context.py
--- a/mercurial/context.py
+++ b/mercurial/context.py
@@ -269,11 +269,11 @@  class basectx(object):
     def match(self, pats=[], include=None, exclude=None, default='glob',
               listsubrepos=False, badfn=None):
         r = self._repo
         return matchmod.match(r.root, r.getcwd(), pats,
                               include, exclude, default,
-                              auditor=r.auditor, ctx=self,
+                              auditor=r.nofsauditor, ctx=self,
                               listsubrepos=listsubrepos, badfn=badfn)
 
     def diff(self, ctx2=None, match=None, **opts):
         """Returns a diff generator for the given contexts and matcher"""
         if ctx2 is None:
diff --git a/tests/test-audit-path.t b/tests/test-audit-path.t
--- a/tests/test-audit-path.t
+++ b/tests/test-audit-path.t
@@ -25,10 +25,49 @@  should still fail - maybe
 
   $ hg add b/b
   abort: path 'b/b' traverses symbolic link 'b' (glob)
   [255]
 
+  $ hg commit -m 'add symlink b'
+
+
+Test symlink traversing when accessing history:
+-----------------------------------------------
+
+(build a changeset where the path exists as a directory)
+
+  $ hg up 0
+  0 files updated, 0 files merged, 1 files removed, 0 files unresolved
+  $ mkdir b
+  $ echo c > b/a
+  $ hg add b/a
+  $ hg ci -m 'add directory b'
+  created new head
+
+Test that hg cat does not do anything wrong the working copy has 'b' as directory
+
+  $ hg cat b/a
+  c
+  $ hg cat -r "desc(directory)" b/a
+  c
+  $ hg cat -r "desc(symlink)" b/a
+  b/a: no such file in rev bc151a1f53bd
+  [1]
+
+Test that hg cat does not do anything wrong the working copy has 'b' as a symlink (issue4749)
+
+  $ hg up 'desc(symlink)'
+  1 files updated, 0 files merged, 1 files removed, 0 files unresolved
+  $ hg cat b/a
+  b/a: no such file in rev bc151a1f53bd
+  [1]
+  $ hg cat -r "desc(directory)" b/a
+  c
+  $ hg cat -r "desc(symlink)" b/a
+  b/a: no such file in rev bc151a1f53bd
+  [1]
+
 #endif
 
 
 unbundle tampered bundle