[PATCH stable] findrepo(), walkrepos(): skip repositories not readable with current privileges; skip empty '.hg' directories, e.g. unmounted mountpoint (was: [PATCH] hg root, run-tests.py: existence of an unreadable /.hg directory causes run-tests.py failures)
Roland Eggner
edvx1 at systemanalysen.net
Wed Jun 13 16:14:32 CDT 2012
On 2012-06-12 Tue 00:09 +0200, Mads Kiilerich wrote:
> Roland Eggner wrote, On 06/11/2012 07:18 PM:
> > (1) Bug reproduction
> > ---------------------
> > (i) [ -d /.hg ] || install -d -m 700 -o root -g root /.hg
> > (ii) Run run-tests.py within a mercurial source tree as an unprivileged user.
> > Existence of the unreadable /.hg directory will cause failure of some 13 tests.
> ...
> > # HG changeset patch
> > # User Roland Eggner < odvx1 at systomanalyson.not s/o/e/g >
> > # Date 1339434022 -7200
> > # Node ID 3f22a0580c54328cbc82947bdfc180e955156c7f
> > # Parent 654ad2f8392c501a55d28a7f1f8f9e39148fcaa5
> > hg root: Skip repositories not readable with current privileges. Skip empty .hg directories, e.g. unmounted mountpoints.
> >
> > diff --git a/mercurial-2.0.2/mercurial/cmdutil.py b/mercurial-2.0.2/mercurial/cmdutil.py
> > --- a/mercurial-2.0.2/mercurial/cmdutil.py
> > +++ b/mercurial-2.0.2/mercurial/cmdutil.py
> > @@ -69,7 +69,9 @@ def findcmd(cmd, table, strict=True):
> > raise error.UnknownCommand(cmd)
> >
> > def findrepo(p):
> > - while not os.path.isdir(os.path.join(p, ".hg")):
> > + while not (os.path.isdir(os.path.join(p, ".hg"))
> > + and ((os.path.isfile(os.path.join(p, '.hg', 'requires' )) and os.access(os.path.join(p, '.hg', 'requires' ), os.R_OK))
> > + or (os.path.isfile(os.path.join(p, '.hg', '00changelog.i')) and os.access(os.path.join(p, '.hg', '00changelog.i'), os.R_OK)))):
> > oldp, p = p, os.path.dirname(p)
> > if p == oldp:
> > return None
>
> Thank you for the patch, but ...
>
> One property of Mercurial is that it is very well-defined which repository
> a given file belong to, even when working directories are nested.
Stock mercurial-1.9 (and probably all later versions released so far) lacks this
“property”, as proved by my bugreport.
The “property” you describe is achieved after applying my patch.
Thank you for the selling argument for my patch.
> All users will get the same output from 'hg root' ... or a failure if they
> don't have access.
My bugreport proves: This is true ONLY after applying my patch.
Again: Thank you for the selling argument for my patch.
> The behaviour changed by this patch is intentional - failure to access a .hg
> should not pass silently.
>
> The error message is quite clear and helpful for debugging the problem.
Sorry, you got it completely wrong. Currently findrepo() and walkrepos()
erroneously succeed on hitting an unreadable directory “.hg”. This leads to
secondary errors like shown in my bugreport:
> > # + abort: Permission denied: /.hg/requires
Instead of investigating possible security vulnerabilities related to this bug,
time is better used for simply fixing the bug. If mercurial-2.2.3 will include my
patch, and perhaps a related CVE entry would be filed, it would be nice to say to
the user community “already fixed, just update to our latest version”. Can you
agree?
Additionally your argument “All users will get the same output …” contradicts
totally several obvious design principles of mercurial. Most important: It
wants to be portable to BOTH multi- and single-user systems. Thus it is
designed from the bottom up with multi-user environments in mind. Exhibited
e.g. by careful locking mechanism and careful protections against symlink
attacks. Consequently in multi-user environments different users get
INTENTIONALLY possibly DIFFERENT outputs from hg commands, namely according to
their privileges.
> The test suite should pass if just there is read access to the outer
> repo - I would suggest that as a workaround. If you don't want to change
> the permissions of /.hg then you could create another readable repo in
> /tmp and set TMP to point inside that. The few tests that depend on not
> having a (readable) outer repo is guarded with 'hghave outer-repo'.
>
> If you want a fix in Mercurial then I consider it feasible to have a
> secret 'debug' option for controlling how close to '/' Mercurial should
> go when searching for a surrounding .hg. It could perhaps be a
> HGDEBUGSKIPROOT environment variable that made findrepo stop searching
> at that directory when looking for a .hg.
Why such a huge, complicated und thus error prone effort to fix a part of the
secondary problems, when there is a patch fixing the primary problem COMPLETELY
with a diffstat as small as shown below in my patch with enhanced description?
Note: Fixing the primary problem solves additionally ALL secondary problems at
once.
> ps: The guidelines for contributing changes can be found on
> http://mercurial.selenic.com/wiki/ContributingChanges .
Thank you for the hint. Already tried to follow this guideline. Now I try even
harder, adjust mail subject, and provide my patch enhanced with more description
(repository just set up for getting proper “hg export tip” output, it holds only
the 2 modified files, thus hashes are probably useless):
# HG changeset patch
# User Roland Eggner < odvx1 at systomanalyson.not s/o/e/g >
# Date 1339621084 -7200
# Node ID fe5416b6fb8607c070a2d3ae0303cb2d7ce04354
# Parent 654ad2f8392c501a55d28a7f1f8f9e39148fcaa5
findrepo(), walkrepos(): skip repositories not readable with current privileges; skip empty '.hg' directories, e.g. unmounted mountpoints
mercurial-2.0.2/mercurial/cmdutil.py | 4 +++-
mercurial-2.0.2/mercurial/scmutil.py | 4 +++-
2 files changed, 6 insertions(+), 2 deletions(-)
(1) Bug fixed by this patch
............................
findrepo() and walkrepos() erroneously succeed on hitting an unreadable
directory “.hg”. This leads to secondary errors like
abort: Permission denied: /.hg/requires
and failure of some 13 tests performed by run-tests.py, if there is an
unreadable directory '/.hg':
Failed test-config-case.t: output changed
Failed test-dispatch.t: output changed
Failed test-globalopts.t: output changed
Failed test-glog.t: output changed
Failed test-hgrc.t: output changed
Failed test-hgwebdir.t: output changed
Failed test-hgwebdirsym.t: output changed
Failed test-i18n.t: output changed
Failed test-identify.t: output changed
Failed test-keyword.t: output changed
Failed test-mq-qclone-http.t: output changed
Failed test-mq.t: output changed
Failed test-subrepo-relative-path.t: output changed
# Ran 389 tests, 12 skipped, 13 failed.
These 13 tests succeed after applying this patch.
(2) Mercurial features provided by this patch
..............................................
- Reliability of 'hg root' and other callers of findrepo() and walkrepos() in
case of nested repositories with possibly different access permissions, or
unmounted '.hg' mountpoints.
- Reliable skipping of unreadable or empty '.hg' directories, when searching
for repository root directory. This is desired e.g. in case of unmounted
'.hg' mountpoints.
- Reliable failure when there is no readable repository, even in case of empty
or unreadable '.hg' directories.
(3) Security considerations
............................
The bug fixed by this patch causes many secondary problems, probably including
security relevant problems. Thus the upcoming release mercurial-2.2.3 should
strongly encourage all users to update to this version anyway, because other
important bugfixes have been committed already (runtests.py: "cd .." issues …).
(4) Performance considerations
...............................
Consistently with other places in mercurial sources, protection against symlink
attacks is taken more important than saving the test for '.hg' beeing a
directory.
time python2.7 run-tests.py -j5 --tmpdir=/var/tmp/portage/dev-vcs/mercurial-2.0.2/temp/tests-2.7
# Ran 399 tests, 15 skipped, 0 failed.
| unpatched | patched |
-----+-----------+-----------+--------
real | 1360.423s | 1365.644s | +0.38%
user | 4365.453s | 4365.967s | +0.01%
sys | 416.495s | 418.503s | +0.48%
-----+-----------+-----------+--------
CPU: Intel(R) Core(TM) i7 CPU M 620 @ 2.67GHz, 2 cores + Hyperthreading,
hot caches
(5) Compatibily considerations
...............................
Proper treatment of revlogv1 and later repositories as well as revlogv0
repositories is provided, based on this assumptions:
- In revlogv1 and later repositories there should always be '.hg/requires'.
- In revlogv0 repositories there should always be '.hg/00changelog.i'.
The patch applies properly to mercurial-2.2.2 sources.
diff --git a/mercurial-2.0.2/mercurial/cmdutil.py b/mercurial-2.0.2/mercurial/cmdutil.py
--- a/mercurial-2.0.2/mercurial/cmdutil.py
+++ b/mercurial-2.0.2/mercurial/cmdutil.py
@@ -69,7 +69,9 @@ def findcmd(cmd, table, strict=True):
raise error.UnknownCommand(cmd)
def findrepo(p):
- while not os.path.isdir(os.path.join(p, ".hg")):
+ while not (os.path.isdir(os.path.join(p, ".hg"))
+ and ((os.path.isfile(os.path.join(p, '.hg', 'requires' )) and os.access(os.path.join(p, '.hg', 'requires' ), os.R_OK))
+ or (os.path.isfile(os.path.join(p, '.hg', '00changelog.i')) and os.access(os.path.join(p, '.hg', '00changelog.i'), os.R_OK)))):
oldp, p = p, os.path.dirname(p)
if p == oldp:
return None
diff --git a/mercurial-2.0.2/mercurial/scmutil.py b/mercurial-2.0.2/mercurial/scmutil.py
--- a/mercurial-2.0.2/mercurial/scmutil.py
+++ b/mercurial-2.0.2/mercurial/scmutil.py
@@ -355,7 +355,9 @@ def walkrepos(path, followsym=False, see
adddir(seen_dirs, path)
for root, dirs, files in os.walk(path, topdown=True, onerror=errhandler):
dirs.sort()
- if '.hg' in dirs:
+ if ('.hg' in dirs
+ and ((os.path.isfile(os.path.join(root, '.hg', 'requires' )) and os.access(os.path.join(root, '.hg', 'requires' ), os.R_OK))
+ or (os.path.isfile(os.path.join(root, '.hg', '00changelog.i')) and os.access(os.path.join(root, '.hg', '00changelog.i'), os.R_OK)))):
yield root # found a repository
qroot = os.path.join(root, '.hg', 'patches')
if os.path.isdir(os.path.join(qroot, '.hg')):
--
Roland Eggner
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://selenic.com/pipermail/mercurial-devel/attachments/20120613/34546384/attachment.pgp>
More information about the Mercurial-devel
mailing list