| 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
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= >
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
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.
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,
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
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,
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:
# 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".