# HG changeset patch # User Bryan O'Sullivan # Date 1186767963 25200 # Node ID d316124ebbea2900037b7e11b5cafe1b98cabbc1 # Parent f6c520fd70cf6ceb3e079e7cda6c6129bb61f5c9 Make audit_path more stringent. The following properties of a path are now checked for: - under top-level .hg - starts at the root of a windows drive - contains ".." - traverses a symlink (e.g. a/symlink_here/b) - inside a nested repository If any of these is true, the path is rejected. The check for traversing a symlink is arguably stricter than necessary; perhaps we should be checking for symlinks that point outside the repository. diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py --- a/mercurial/localrepo.py +++ b/mercurial/localrepo.py @@ -69,7 +69,8 @@ class localrepository(repo.repository): self.encodefn = lambda x: x self.decodefn = lambda x: x self.spath = self.path - self.sopener = util.encodedopener(util.opener(self.spath), self.encodefn) + self.sopener = util.encodedopener(util.opener(self.spath), + self.encodefn) self.ui = ui.ui(parentui=parentui) try: diff --git a/mercurial/merge.py b/mercurial/merge.py --- a/mercurial/merge.py +++ b/mercurial/merge.py @@ -391,13 +391,15 @@ def applyupdates(repo, action, wctx, mct repo.ui.debug(_("copying %s to %s\n") % (f, fd)) repo.wwrite(fd, repo.wread(f), flags) + audit_path = util.path_auditor(repo.root) + for a in action: f, m = a[:2] if f and f[0] == "/": continue if m == "r": # remove repo.ui.note(_("removing %s\n") % f) - util.audit_path(f) + audit_path(f) try: util.unlink(repo.wjoin(f)) except OSError, inst: diff --git a/mercurial/util.py b/mercurial/util.py --- a/mercurial/util.py +++ b/mercurial/util.py @@ -13,8 +13,8 @@ platform-specific details from the core. """ from i18n import _ -import cStringIO, errno, getpass, popen2, re, shutil, sys, tempfile -import os, threading, time, calendar, ConfigParser, locale, glob +import cStringIO, errno, getpass, popen2, re, shutil, sys, tempfile, strutil +import os, stat, threading, time, calendar, ConfigParser, locale, glob try: set = set @@ -366,6 +366,7 @@ def canonpath(root, cwd, myname): if not os.path.isabs(name): name = os.path.join(root, cwd, name) name = os.path.normpath(name) + audit_path = path_auditor(root) if name != rootsep and name.startswith(rootsep): name = name[len(rootsep):] audit_path(name) @@ -680,12 +681,45 @@ def copyfiles(src, dst, hardlink=None): else: shutil.copy(src, dst) -def audit_path(path): - """Abort if path contains dangerous components""" - parts = os.path.normcase(path).split(os.sep) - if (os.path.splitdrive(path)[0] or parts[0] in ('.hg', '') - or os.pardir in parts): - raise Abort(_("path contains illegal component: %s") % path) +class path_auditor(object): + '''ensure that a filesystem path contains no banned components. + the following properties of a path are checked: + + - under top-level .hg + - starts at the root of a windows drive + - contains ".." + - traverses a symlink (e.g. a/symlink_here/b) + - inside a nested repository''' + + def __init__(self, root): + self.audited = {} + self.root = root + + def __call__(self, path): + if path in self.audited: + return + parts = os.path.normcase(path).split(os.sep) + if (os.path.splitdrive(path)[0] or parts[0] in ('.hg', '') + or os.pardir in parts): + raise Abort(_("path contains illegal component: %s") % path) + def check(prefix): + curpath = os.path.join(self.root, prefix) + try: + st = os.lstat(curpath) + except OSError, err: + if err.errno != errno.ENOENT: + raise + else: + if stat.S_ISLNK(st.st_mode): + raise Abort(_('path %r traverses symbolic link %r') % + (path, prefix)) + if os.path.exists(os.path.join(curpath, '.hg')): + raise Abort(_('path %r is inside repo %r') % + (path, prefix)) + self.audited[prefix] = True + for c in strutil.rfindall(path, os.sep): + check(path[:c]) + self.audited[path] = True def _makelock_file(info, pathname): ld = os.open(pathname, os.O_CREAT | os.O_WRONLY | os.O_EXCL) @@ -1262,7 +1296,10 @@ class opener(object): """ def __init__(self, base, audit=True): self.base = base - self.audit = audit + if audit: + self.audit_path = path_auditor(base) + else: + self.audit_path = always def __getattr__(self, name): if name == '_can_symlink': @@ -1271,8 +1308,7 @@ class opener(object): raise AttributeError(name) def __call__(self, path, mode="r", text=False, atomictemp=False): - if self.audit: - audit_path(path) + self.audit_path(path) f = os.path.join(self.base, path) if not text and "b" not in mode: @@ -1293,8 +1329,7 @@ class opener(object): return posixfile(f, mode) def symlink(self, src, dst): - if self.audit: - audit_path(dst) + self.audit_path(dst) linkname = os.path.join(self.base, dst) try: os.unlink(linkname) diff --git a/tests/test-audit-path b/tests/test-audit-path new file mode 100755 --- /dev/null +++ b/tests/test-audit-path @@ -0,0 +1,23 @@ +#!/bin/sh + +hg init + +echo % should fail +hg add .hg/00changelog.i + +mkdir a +echo a > a/a +hg ci -Ama +ln -s a b +echo b > a/b + +echo % should fail +hg add b/b + +echo % should succeed +hg add b + +echo % should still fail - maybe +hg add b/b + +exit 0 diff --git a/tests/test-audit-path.out b/tests/test-audit-path.out new file mode 100644 --- /dev/null +++ b/tests/test-audit-path.out @@ -0,0 +1,8 @@ +% should fail +abort: path contains illegal component: .hg/00changelog.i +adding a/a +% should fail +abort: path 'b/b' traverses symbolic link 'b' +% should succeed +% should still fail - maybe +abort: path 'b/b' traverses symbolic link 'b' diff --git a/tests/test-nested-repo b/tests/test-nested-repo --- a/tests/test-nested-repo +++ b/tests/test-nested-repo @@ -4,16 +4,21 @@ hg init a cd a hg init b echo x > b/x + echo '# should print nothing' +hg add b hg st -echo '# should print ? b/x' + +echo '# should fail' hg st b/x - hg add b/x -echo '# should print A b/x' +echo '# should arguably print nothing' +hg st b + +echo a > a +hg ci -Ama a + +echo '# should fail' +hg mv a b hg st -echo '# should forget b/x' -hg revert --all -echo '# should print nothing' -hg st b diff --git a/tests/test-nested-repo.out b/tests/test-nested-repo.out --- a/tests/test-nested-repo.out +++ b/tests/test-nested-repo.out @@ -1,8 +1,7 @@ # should print nothing -# should print ? b/x -? b/x -# should print A b/x -A b/x -# should forget b/x -forgetting b/x -# should print nothing +# should fail +abort: path 'b/x' is inside repo 'b' +abort: path 'b/x' is inside repo 'b' +# should arguably print nothing +# should fail +abort: path 'b/a' is inside repo 'b'