Patchwork [3,of,3] hook: report untrusted hooks as failure (issue5110) (BC)

login
register
mail settings
Submitter Pierre-Yves David
Date April 15, 2016, 7:50 a.m.
Message ID <97b264764e417eefc025.1460706622@nodosa.octopoid.net>
Download mbox | patch
Permalink /patch/14638/
State Superseded
Headers show

Comments

Pierre-Yves David - April 15, 2016, 7:50 a.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@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".
Simon Farnsworth - April 15, 2016, 11:29 a.m.
On 15/04/2016 08:50, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@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@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=
>
timeless - April 15, 2016, 1:26 p.m.
On Apr 15, 2016 3:50 AM, "Pierre-Yves David" <pierre-yves.david@ens-lyon.org>
wrote:

> After this patch, hooks from unstrusted config are taken in account but
never

Err untrusted configs; taken into

> actually run. Instead they are reported as failure right away.

As failures

> This will ensure

This ensures

> no validation performed by a hook will be ignored
"Will be" is too complicated.

Try "validation performed by a hook is not ignored"

> (as a fallback to "False" is
> better than a fallback to "True" here)

This doesn't make sense

> As a side effect writer can be forced to trust a repository hgrc by
adding a
> 'pretxnopen.trust=true' hook to the file.

> It turned out that the "trusted" concept did not had any ".t" test.

Did not have ... tests

> The test
> process do not have enough priviledge to change config file ownership.

Err privileges

> +    # Be careful in this section, propagating the real command from an

Real commands

> +    # untrusted source would create a security vulnerability, make sure

From untrusted sources

> +    # anything altered in that section use "_fromuntrusted" as its
command.

Uses

> +                abortmsg = _('%s hook denied (from untrusted config)')

I'm not a fan of this wording

Probably "denied %s hook (....)"

> +                warnmsg = _('warning: %s hook denied (from untrusted
config)\n')

Ditto
Yuya Nishihara - April 15, 2016, 3:46 p.m.
On Fri, 15 Apr 2016 00:50:22 -0700, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@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)

The series looks good to me, but I think the nits spotted by Simon are legit.
So I let someone decide to queue these.

> 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".

You can fake the trusted flag of repo/.hg/hgrc by extension.

  $ cat << EOF > untrustedui.py
  > def uisetup(ui):
  >     class untrustedui(ui.__class__):
  >         def _trusted(self, fp, f):
  >             return False  # XXX per "f" ?
  >             return super(untrustedui, self)._trusted(fp, f)
  >     ui.__class__ = untrustedui
  > EOF

This only applies to files loaded without trust=True.
Pierre-Yves David - April 15, 2016, 6:48 p.m.
On 04/15/2016 06:26 AM, timeless wrote:
>
>
> On Apr 15, 2016 3:50 AM, "Pierre-Yves David" 
> <pierre-yves.david@ens-lyon.org 
> <mailto:pierre-yves.david@ens-lyon.org>> wrote:
>
> > +                abortmsg = _('%s hook denied (from untrusted config)')
>
> I'm not a fan of this wording
>
> Probably "denied %s hook (....)"
>

All all test related message starts with "<hookname> hook <stuff that 
happened>" I would rather stick with that.

> > +                warnmsg = _('warning: %s hook denied (from 
> untrusted config)\n')
>

Cheers,
timeless - April 15, 2016, 7:12 p.m.
can i convince you to replace "denied" with "skipped"?

On Fri, Apr 15, 2016 at 2:48 PM, Pierre-Yves David
<pierre-yves.david@ens-lyon.org> wrote:
>
>
> On 04/15/2016 06:26 AM, timeless wrote:
>>
>>
>>
>> On Apr 15, 2016 3:50 AM, "Pierre-Yves David"
>> <pierre-yves.david@ens-lyon.org <mailto:pierre-yves.david@ens-lyon.org>>
>> wrote:
>>
>> > +                abortmsg = _('%s hook denied (from untrusted config)')
>>
>> I'm not a fan of this wording
>>
>> Probably "denied %s hook (....)"
>>
>
> All all test related message starts with "<hookname> hook <stuff that
> happened>" I would rather stick with that.
>
>> > +                warnmsg = _('warning: %s hook denied (from untrusted
>> > config)\n')
>>
>
> Cheers,
>
> --
> Pierre-Yves David
Pierre-Yves David - April 15, 2016, 7:20 p.m.
On 04/15/2016 12:12 PM, timeless wrote:
> can i convince you to replace "denied" with "skipped"?

Skipped seemed a bit too light for the abort case:

Abort: pretxnopen.foo hook skipped (from unstrusted source)

(it is not really "skipped" as it aborted the whole operation).

What do you think?

Cheers,
timeless - April 15, 2016, 7:26 p.m.
Pierre-Yves David wrote:
> timeless wrote:
>> can i convince you to replace "denied" with "skipped"?

> Skipped seemed a bit too light for the abort case:
>
> Abort: pretxnopen.foo hook skipped (from unstrusted source)
>
> (it is not really "skipped" as it aborted the whole operation).
>
> What do you think?

I'm thinking "Can I get it in a sentence?"
But in practical terms, can you add tests w/ this commit showing
pretxnopen/pretxnclose (passing, failing)?

"forbidden" is probably the right word in the pretxnopen case (instead
of denied).

Patch

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,)
+    # (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: