[PATCH 3 of 3] hook: report untrusted hooks as failure (issue5110) (BC)
Simon Farnsworth
simonfar at fb.com
Fri Apr 15 07:29:14 EDT 2016
On 15/04/2016 08:50, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david at ens-lyon.org>
> # Date 1460626875 25200
> # Thu Apr 14 02:41:15 2016 -0700
> # Node ID 97b264764e417eefc025b8c7fe298114bde11f1c
> # Parent 41e1d17cdc4943805d5e7a6e7c131367eb94e8d3
> # EXP-Topic hooks
> hook: report untrusted hooks as failure (issue5110) (BC)
>
> Before this patch, there was no way for a repository owner to ensure that
> validation hooks would be run by people with write access. If someone had write
> access but did not trust the user owning the repository, the config and its hook
> would simply be ignored.
>
> After this patch, hooks from unstrusted config are taken in account but never
> actually run. Instead they are reported as failure right away. This will ensure
> no validation performed by a hook will be ignored (as a fallback to "False" is
> better than a fallback to "True" here)
>
> As a side effect writer can be forced to trust a repository hgrc by adding a
> 'pretxnopen.trust=true' hook to the file.
>
> This was discussed during the 3.8 sprint with Matt Mackall, Augie Fackler and
> Kevin Bullock.
>
> Here is some example output:
>
> non-mandatory hook:
> warning: post-log hook denied (from untrusted config)
>
> mandatory hook:
> abort: pretxnclose hook denied (from untrusted config)
>
> It turned out that the "trusted" concept did not had any ".t" test. The test
> process do not have enough priviledge to change config file ownership. It should
> be possible to implement a devel config option that makes a hgrc file
> untrusted to testing purpose. However, given that the freeze is quite close, I
> wanted this patch out for review quickly. I'll follow up with another series
> installing the test infrastructure for "trusted".
>
> diff -r 41e1d17cdc49 -r 97b264764e41 mercurial/hook.py
> --- a/mercurial/hook.py Thu Apr 14 17:03:49 2016 -0700
> +++ b/mercurial/hook.py Thu Apr 14 02:41:15 2016 -0700
> @@ -161,16 +161,29 @@
> ui.warn(_('warning: %s hook %s\n') % (name, desc))
> return r
>
> +# represent an untrusted hook command
> +_fromuntrusted = object()
> +
> def _allhooks(ui):
> """return a list of (hook-id, cmd) pair sorted by priority"""
> hooks = _hookitems(ui)
> + trusted = set(hooks)
> + # Be careful in this section, propagating the real command from an
> + # untrusted source would create a security vulnerability, make sure
> + # anything altered in that section use "_fromuntrusted" as its command.
> + untrustedhooks = _hookitems(ui, _untrusted=True)
> + for name, value in untrustedhooks.items():
> + if value != hooks.get(name, None):
> + trusted.discard(name)
> + hooks[name] = value[:2] + (_fromuntrusted,)
Given how sensitive this is, I'd prefer:
(p, o, k, v) = hooks[name]
hooks[name] = (p, o, k, _fromuntrusted)
Just to make it clear what you're replacing.
Patch 2 is fine in intent, but given the cleanup I'd like to see, not
applicable yet.
> + # (end of the security sensitive section)
> order = sorted(hooks, key=lambda x: hooks[x][:2] + (x,))
> return [(k, hooks[k][2]) for k in order]
>
> -def _hookitems(ui):
> +def _hookitems(ui, _untrusted=False):
> """return all hooks items ready to be sorted"""
> hooks = {}
> - for name, cmd in ui.configitems('hooks'):
> + for name, cmd in ui.configitems('hooks', untrusted=_untrusted):
> if not name.startswith('priority'):
> priority = ui.configint('hooks', 'priority.%s' % name, 0)
> hooks[name] = (-priority, len(hooks), cmd)
> @@ -215,7 +228,15 @@
> # files seem to be bogus, give up on redirecting (WSGI, etc)
> pass
>
> - if callable(cmd):
> + if cmd is _fromuntrusted:
> + abortmsg = _('%s hook denied (from untrusted config)')
> + warnmsg = _('warning: %s hook denied (from untrusted config)\n')
> + if throw:
> + raise error.HookAbort(abortmsg % name)
> + ui.warn(warnmsg % name)
> + r = 1
> + raised = False
> + elif callable(cmd):
> r, raised = _pythonhook(ui, repo, name, hname, cmd, args, throw)
> elif cmd.startswith('python:'):
> if cmd.count(':') >= 2:
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at mercurial-scm.org
> https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mercurial-2Dscm.org_mailman_listinfo_mercurial-2Ddevel&d=CwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=mEgSWILcY4c4W3zjApBQLA&m=L54A2dvUEdQx1q_9TNxyE7ArSm559cMdniGBzejNibM&s=QE80XELm_gFeAMKHA7rahAFoMkuswaB83i7Tq-1KAEA&e=
>
--
Simon Farnsworth
More information about the Mercurial-devel
mailing list