# HG changeset patch # User Yuya Nishihara # Date 2017-11-23 13:17:03 # Node ID c9740b69b9b7110aa23db3ce87820e4e4a8bbb2b # Parent 898c6f812a51a86bcdc3b7d97d993e5607550cba dispatch: add HGPLAIN=+strictflags to restrict early parsing of global options If this feature is enabled, early options are parsed using the global options table. As the parser stops processing options when non/unknown option is encountered, it won't mistakenly take an option value as a new early option. Still "--" can be injected to terminate the parsing (e.g. "hg -R -- log"), I think it's unlikely to lead to an RCE. To minimize a risk of this change, new fancyopts.earlygetopt() path is enabled only when +strictflags is set. Also the strict parser doesn't support '--repo', a short for '--repository' yet. This limitation will be removed later. As this feature is backward incompatible, I decided to add a new opt-in mechanism to HGPLAIN. I'm not pretty sure if this is the right choice, but I'm thinking of adding +feature/-feature syntax to HGPLAIN. Alternatively, we could add a new environment variable. Any bikeshedding is welcome. Note that HGPLAIN=+strictflags doesn't work correctly in chg session since command arguments are pre-processed in C. This wouldn't be easily fixed. diff --git a/mercurial/chgserver.py b/mercurial/chgserver.py --- a/mercurial/chgserver.py +++ b/mercurial/chgserver.py @@ -220,8 +220,17 @@ def _loadnewui(srcui, args): newui._csystem = srcui._csystem # command line args - args = args[:] - dispatch._parseconfig(newui, dispatch._earlygetopt(['--config'], args)) + options = {} + if srcui.plain('strictflags'): + options.update(dispatch._earlyparseopts(args)) + else: + args = args[:] + options['config'] = dispatch._earlygetopt(['--config'], args) + cwds = dispatch._earlygetopt(['--cwd'], args) + options['cwd'] = cwds and cwds[-1] or '' + rpath = dispatch._earlygetopt(["-R", "--repository", "--repo"], args) + options['repository'] = rpath and rpath[-1] or '' + dispatch._parseconfig(newui, options['config']) # stolen from tortoisehg.util.copydynamicconfig() for section, name, value in srcui.walkconfig(): @@ -232,10 +241,9 @@ def _loadnewui(srcui, args): newui.setconfig(section, name, value, source) # load wd and repo config, copied from dispatch.py - cwds = dispatch._earlygetopt(['--cwd'], args) - cwd = cwds and os.path.realpath(cwds[-1]) or None - rpath = dispatch._earlygetopt(["-R", "--repository", "--repo"], args) - rpath = rpath and rpath[-1] or '' + cwd = options['cwd'] + cwd = cwd and os.path.realpath(cwd) or None + rpath = options['repository'] path, newlui = dispatch._getlocal(newui, rpath, wd=cwd) return (newui, newlui) diff --git a/mercurial/dispatch.py b/mercurial/dispatch.py --- a/mercurial/dispatch.py +++ b/mercurial/dispatch.py @@ -150,6 +150,8 @@ def dispatch(req): try: if not req.ui: req.ui = uimod.ui.load() + if req.ui.plain('strictflags'): + req.earlyoptions.update(_earlyparseopts(req.args)) if _earlyreqoptbool(req, 'traceback', ['--traceback']): req.ui.setconfig('ui', 'traceback', 'on', '--traceback') @@ -644,6 +646,12 @@ def _parseconfig(ui, config): return configs +def _earlyparseopts(args): + options = {} + fancyopts.fancyopts(args, commands.globalopts, options, + gnu=False, early=True) + return options + def _earlygetopt(aliases, args, strip=True): """Return list of values for an option (or aliases). @@ -732,12 +740,16 @@ def _earlygetopt(aliases, args, strip=Tr def _earlyreqopt(req, name, aliases): """Peek a list option without using a full options table""" + if req.ui.plain('strictflags'): + return req.earlyoptions[name] values = _earlygetopt(aliases, req.args, strip=False) req.earlyoptions[name] = values return values def _earlyreqoptstr(req, name, aliases): """Peek a string option without using a full options table""" + if req.ui.plain('strictflags'): + return req.earlyoptions[name] value = (_earlygetopt(aliases, req.args, strip=False) or [''])[-1] req.earlyoptions[name] = value return value @@ -745,13 +757,15 @@ def _earlyreqoptstr(req, name, aliases): def _earlyreqoptbool(req, name, aliases): """Peek a boolean option without using a full options table - >>> req = request([b'x', b'--debugger']) + >>> req = request([b'x', b'--debugger'], uimod.ui()) >>> _earlyreqoptbool(req, b'debugger', [b'--debugger']) True - >>> req = request([b'x', b'--', b'--debugger']) + >>> req = request([b'x', b'--', b'--debugger'], uimod.ui()) >>> _earlyreqoptbool(req, b'debugger', [b'--debugger']) """ + if req.ui.plain('strictflags'): + return req.earlyoptions[name] try: argcount = req.args.index("--") except ValueError: diff --git a/mercurial/help/environment.txt b/mercurial/help/environment.txt --- a/mercurial/help/environment.txt +++ b/mercurial/help/environment.txt @@ -56,9 +56,17 @@ HGPLAIN localization. This can be useful when scripting against Mercurial in the face of existing user configuration. + In addition to the features disabled by ``HGPLAIN=``, the following + values can be specified to adjust behavior: + + ``+strictflags`` + Restrict parsing of command line flags. + Equivalent options set via command line flags or environment variables are not overridden. + See :hg:`help scripting` for details. + HGPLAINEXCEPT This is a comma-separated list of features to preserve when HGPLAIN is enabled. Currently the following values are supported: diff --git a/mercurial/help/scripting.txt b/mercurial/help/scripting.txt --- a/mercurial/help/scripting.txt +++ b/mercurial/help/scripting.txt @@ -74,6 +74,32 @@ HGRCPATH like the username and extensions that may be required to interface with a repository. +Command-line Flags +================== + +Mercurial's default command-line parser is designed for humans, and is not +robust against malicious input. For instance, you can start a debugger by +passing ``--debugger`` as an option value:: + + $ REV=--debugger sh -c 'hg log -r "$REV"' + +This happens because several command-line flags need to be scanned without +using a concrete command table, which may be modified while loading repository +settings and extensions. + +Since Mercurial 4.4.2, the parsing of such flags may be restricted by setting +``HGPLAIN=+strictflags``. When this feature is enabled, all early options +(e.g. ``-R/--repository``, ``--cwd``, ``--config``) must be specified first +amongst the other global options, and cannot be injected to an arbitrary +location:: + + $ HGPLAIN=+strictflags hg -R "$REPO" log -r "$REV" + +In earlier Mercurial versions where ``+strictflags`` isn't available, you +can mitigate the issue by concatenating an option value with its flag:: + + $ hg log -r"$REV" --keyword="$KEYWORD" + Consuming Command Output ======================== diff --git a/mercurial/ui.py b/mercurial/ui.py --- a/mercurial/ui.py +++ b/mercurial/ui.py @@ -761,6 +761,7 @@ class ui(object): The return value can either be - False if HGPLAIN is not set, or feature is in HGPLAINEXCEPT + - False if feature is disabled by default and not included in HGPLAIN - True otherwise ''' if ('HGPLAIN' not in encoding.environ and @@ -768,6 +769,9 @@ class ui(object): return False exceptions = encoding.environ.get('HGPLAINEXCEPT', '').strip().split(',') + # TODO: add support for HGPLAIN=+feature,-feature syntax + if '+strictflags' not in encoding.environ.get('HGPLAIN', '').split(','): + exceptions.append('strictflags') if feature and exceptions: return feature not in exceptions return True diff --git a/tests/test-commandserver.t b/tests/test-commandserver.t --- a/tests/test-commandserver.t +++ b/tests/test-commandserver.t @@ -137,6 +137,20 @@ typical client does not want echo-back m summary: 1 +check strict parsing of early options: + + >>> import os + >>> from hgclient import check, readchannel, runcommand + >>> os.environ['HGPLAIN'] = '+strictflags' + >>> @check + ... def cwd(server): + ... readchannel(server) + ... runcommand(server, ['log', '-b', '--config=alias.log=!echo pwned', + ... 'default']) + *** runcommand log -b --config=alias.log=!echo pwned default + abort: unknown revision '--config=alias.log=!echo pwned'! + [255] + check that "histedit --commands=-" can read rules from the input channel: >>> import cStringIO diff --git a/tests/test-dispatch.t b/tests/test-dispatch.t --- a/tests/test-dispatch.t +++ b/tests/test-dispatch.t @@ -113,6 +113,51 @@ Shell aliases bypass any command parsing $ hg log -b '--config=alias.log=!echo howdy' howdy +Early options must come first if HGPLAIN=+strictflags is specified: +(BUG: chg cherry-picks early options to pass them as a server command) + +#if no-chg + $ HGPLAIN=+strictflags hg log -b --config='hooks.pre-log=false' default + abort: unknown revision '--config=hooks.pre-log=false'! + [255] + $ HGPLAIN=+strictflags hg log -b -R. default + abort: unknown revision '-R.'! + [255] + $ HGPLAIN=+strictflags hg log -b --cwd=. default + abort: unknown revision '--cwd=.'! + [255] +#endif + $ HGPLAIN=+strictflags hg log -b --debugger default + abort: unknown revision '--debugger'! + [255] + $ HGPLAIN=+strictflags hg log -b --config='alias.log=!echo pwned' default + abort: unknown revision '--config=alias.log=!echo pwned'! + [255] + + $ HGPLAIN=+strictflags hg log --config='hooks.pre-log=false' -b default + abort: option --config may not be abbreviated! + [255] + $ HGPLAIN=+strictflags hg log -q --cwd=.. -b default + abort: option --cwd may not be abbreviated! + [255] + $ HGPLAIN=+strictflags hg log -q -R . -b default + abort: option -R has to be separated from other options (e.g. not -qR) and --repository may only be abbreviated as --repo! + [255] + + $ HGPLAIN=+strictflags hg --config='hooks.pre-log=false' log -b default + abort: pre-log hook exited with status 1 + [255] + $ HGPLAIN=+strictflags hg --cwd .. -q -Ra log -b default + 0:cb9a9f314b8b + +For compatibility reasons, HGPLAIN=+strictflags is not enabled by plain HGPLAIN: + + $ HGPLAIN= hg log --config='hooks.pre-log=false' -b default + abort: pre-log hook exited with status 1 + [255] + $ HGPLAINEXCEPT= hg log --cwd .. -q -Ra -b default + 0:cb9a9f314b8b + [defaults] $ hg cat a