[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