You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Markus Schaber <m....@codesys.com> on 2014/06/19 13:08:04 UTC
[Patch] Fix for Issue 3046: document security requirement for hook
script arguments
Hi,
The attached patch fixes issue 3046 and also adds an hint wr/t peg revisions, as inspired by danielsh on IRC.
[[[
Fix issue 3046 by adding a statement about quoting of parameters. Also
add a hint about peg revisions, while we are at it.
* subversion/libsvn_repos/repos.c
(create_hooks): Add a hint about quoting of parameters and url
Handling to the hook templates.
]]]
Best regards
Markus Schaber
CODESYS(r) a trademark of 3S-Smart Software Solutions GmbH
Inspiring Automation Solutions
3S-Smart Software Solutions GmbH
Dipl.-Inf. Markus Schaber | Product Development Core Technology
Memminger Str. 151 | 87439 Kempten | Germany
Tel. +49-831-54031-979 | Fax +49-831-54031-50
E-Mail: m.schaber@codesys.com | Web: http://www.codesys.com | CODESYS store: http://store.codesys.com
CODESYS forum: http://forum.codesys.com
Managing Directors: Dipl.Inf. Dieter Hess, Dipl.Inf. Manfred Werner | Trade register: Kempten HRB 6186 | Tax ID No.: DE 167014915
Re: [Patch] Fix for Issue 3046: document security requirement for
hook script arguments
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Markus Schaber wrote on Thu, Jun 19, 2014 at 13:12:44 +0000:
> Index: subversion/libsvn_repos/repos.c
> ===================================================================
> --- subversion/libsvn_repos/repos.c (revision 1603773)
> +++ subversion/libsvn_repos/repos.c (working copy)
> @@ -280,6 +280,13 @@
> "# http://svn.apache.org/repos/asf/subversion/trunk/tools/hook-scripts/ and" NL \
> "# http://svn.apache.org/repos/asf/subversion/trunk/contrib/hook-scripts/" NL
>
> +#define HOOKS_QUOTE_ARGUMENTS_TEXT \
> + "# CAUTION:" NL \
> + "# For security reasions, you MUST always properly quote arguments when" NL \
> + "# you use them. For example, a malicious client could try to set a" NL \
> + "# revision property named \"foo; rm -rf /;\"." NL \
As I said on IRC: the problem values would usually be those that start with a
minus (should be protected against by adding a "--" argument; e.g., see
freeze_freeze() in svnadmin_tests.py) and those that contain whitespace
(should be protected against by "quoting"). Embedded semicolons aren't
usually going to be a problem.
Apart from that, I'd be happy to +1 that based on glasser's thinking
it's a good idea in the issue. But I think it the text should enumerate
the correct set of problems.
Cheers,
Daniel
P.S. Markus - thanks for forwarding my suggestion on IRC earlier over to
the svnbook issue tracker :)
> + "# For similar reasons, you should also add a trailing @ to URLs which" NL \
> + "# are passed to SVN commands which accept URLs with peg revisions." NL
Re: [Patch] Fix for Issue 3046: document security requirement for
hook script arguments
Posted by James McCoy <ja...@debian.org>.
On Thu, Jun 19, 2014 at 01:12:44PM +0000, Markus Schaber wrote:
> Index: subversion/libsvn_repos/repos.c
> ===================================================================
> --- subversion/libsvn_repos/repos.c (revision 1603773)
> +++ subversion/libsvn_repos/repos.c (working copy)
> @@ -280,6 +280,13 @@
> "# http://svn.apache.org/repos/asf/subversion/trunk/tools/hook-scripts/ and" NL \
> "# http://svn.apache.org/repos/asf/subversion/trunk/contrib/hook-scripts/" NL
>
> +#define HOOKS_QUOTE_ARGUMENTS_TEXT \
> + "# CAUTION:" NL \
> + "# For security reasions, you MUST always properly quote arguments when" NL \
s/reasions/reasons/
Cheers,
--
James
GPG Key: 4096R/331BA3DB 2011-12-05 James McCoy <ja...@debian.org>
Re: [Patch] Fix for Issue 3046: document security requirement for
hook script arguments
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Markus Schaber wrote on Fri, Jun 20, 2014 at 07:53:09 +0000:
> Hi,
>
> See attached the third iteration of the patch.
>
> I did add coverage for the problems of arguments containing whitespace and dashes, and did drop the example I got from the issue tracker, as it is questionable whether that specific example really is a problem.
>
>
> [[[
> Fix issue 3046 by adding a statement about quoting of parameters and delimiting argument lists. Also add a hint about peg revisions, while we are at it.
>
> * subversion/libsvn_repos/repos.c
> (create_hooks): Add a hint about quoting of parameters and url
> handling to the hook templates.
> ]]]
>
> +#define HOOKS_QUOTE_ARGUMENTS_TEXT \
> + "# CAUTION:" NL \
> + "# For security reasons, you MUST always properly quote arguments when" NL \
> + "# you use them, as those arguments could contain whitespace or other" NL \
> + "# problematic characters. Additionally, you should delimit the list" NL \
> + "# of options with \"--\" before passing the arguments, so malicious" NL \
> + "# clients cannot bootleg unexpected options to the commands your" NL \
> + "# script aims to execute." NL \
> + "# For similar reasons, you should also add a trailing @ to URLs which" NL \
> + "# are passed to SVN commands accepting URLs with peg revisions." NL
+1, thanks!
AW: [Patch] Fix for Issue 3046: document security requirement for
hook script arguments
Posted by Markus Schaber <m....@codesys.com>.
Hi,
See attached the third iteration of the patch.
I did add coverage for the problems of arguments containing whitespace and dashes, and did drop the example I got from the issue tracker, as it is questionable whether that specific example really is a problem.
[[[
Fix issue 3046 by adding a statement about quoting of parameters and delimiting argument lists. Also add a hint about peg revisions, while we are at it.
* subversion/libsvn_repos/repos.c
(create_hooks): Add a hint about quoting of parameters and url
handling to the hook templates.
]]]
Best regards
Markus Schaber
CODESYS® a trademark of 3S-Smart Software Solutions GmbH
Inspiring Automation Solutions
3S-Smart Software Solutions GmbH
Dipl.-Inf. Markus Schaber | Product Development Core Technology
Memminger Str. 151 | 87439 Kempten | Germany
Tel. +49-831-54031-979 | Fax +49-831-54031-50
E-Mail: m.schaber@codesys.com | Web: http://www.codesys.com | CODESYS store: http://store.codesys.com
CODESYS forum: http://forum.codesys.com
Managing Directors: Dipl.Inf. Dieter Hess, Dipl.Inf. Manfred Werner | Trade register: Kempten HRB 6186 | Tax ID No.: DE 167014915
> -----Ursprüngliche Nachricht-----
> Von: Markus Schaber
> Gesendet: Donnerstag, 19. Juni 2014 15:13
> An: Markus Schaber; Konstantin Kolinko
> Cc: Subversion Dev (dev@subversion.apache.org)
> Betreff: AW: [Patch] Fix for Issue 3046: document security
> requirement for hook script arguments
>
> Hi,
>
> The patch actually attached.
>
>
>
> Best regards
>
> Markus Schaber
>
> CODESYS® a trademark of 3S-Smart Software Solutions GmbH
>
> Inspiring Automation Solutions
>
> 3S-Smart Software Solutions GmbH
> Dipl.-Inf. Markus Schaber | Product Development Core Technology
> Memminger Str. 151 | 87439 Kempten | Germany Tel. +49-831-54031-979 |
> Fax +49-831-54031-50
>
> E-Mail: m.schaber@codesys.com | Web: http://www.codesys.com | CODESYS
> store: http://store.codesys.com CODESYS forum:
> http://forum.codesys.com
>
> Managing Directors: Dipl.Inf. Dieter Hess, Dipl.Inf. Manfred Werner |
> Trade register: Kempten HRB 6186 | Tax ID No.: DE 167014915
>
> > -----Ursprüngliche Nachricht-----
> > Von: Markus Schaber [mailto:m.schaber@codesys.com]
> > Gesendet: Donnerstag, 19. Juni 2014 14:01
> > An: Konstantin Kolinko
> > Cc: Subversion Dev (dev@subversion.apache.org)
> > Betreff: AW: [Patch] Fix for Issue 3046: document security
> requirement
> > for hook script arguments
> >
> > Hi,
> >
> > The second iteration of the patch to fix issue 3046 and also add a
> > hint wr/t peg revisions, as inspired by danielsh on IRC while
> > discussing issue 2349. This iteration fixes 2 typos.
> >
> > [[[
> > Fix issue 3046 by adding a statement about quoting of parameters.
> > Also add a hint about peg revisions, while we are at it.
> >
> > * subversion/libsvn_repos/repos.c
> > (create_hooks): Add a hint about quoting of parameters and url
> > handling to the hook templates.
> > ]]]
> >
> > Best regards
> >
> > Markus Schaber
> >
> > CODESYS® a trademark of 3S-Smart Software Solutions GmbH
> >
> > Inspiring Automation Solutions
> >
> > 3S-Smart Software Solutions GmbH
> > Dipl.-Inf. Markus Schaber | Product Development Core Technology
> > Memminger Str. 151 | 87439 Kempten | Germany Tel. +49-831-54031-979
> |
> > Fax +49-831-54031-50
> >
> > E-Mail: m.schaber@codesys.com | Web: http://www.codesys.com |
> CODESYS
> > store: http://store.codesys.com CODESYS forum:
> > http://forum.codesys.com
> >
> > Managing Directors: Dipl.Inf. Dieter Hess, Dipl.Inf. Manfred Werner
> |
> > Trade register: Kempten HRB 6186 | Tax ID No.: DE 167014915
> >
> > > -----Ursprüngliche Nachricht-----
> > > Von: Konstantin Kolinko [mailto:knst.kolinko@gmail.com]
> > > Gesendet: Donnerstag, 19. Juni 2014 13:45
> > > An: Markus Schaber
> > > Cc: Subversion Dev (dev@subversion.apache.org)
> > > Betreff: Re: [Patch] Fix for Issue 3046: document security
> > requirement
> > > for hook script arguments
> > >
> > > 2014-06-19 15:08 GMT+04:00 Markus Schaber <m....@codesys.com>:
> > > > Hi,
> > > >
> > > > The attached patch fixes issue 3046 and also adds an hint wr/t
> > peg
> > > revisions, as inspired by danielsh on IRC.
> > > >
> > > > [[[
> > > > Fix issue 3046 by adding a statement about quoting of
> parameters.
> > > Also
> > > > add a hint about peg revisions, while we are at it.
> > > >
> > > > * subversion/libsvn_repos/repos.c
> > > > (create_hooks): Add a hint about quoting of parameters and
> url
> > > > Handling to the hook templates.
> > > > ]]]
> > >
> > > Interesting.
> > >
> > > A typo in the text:
> > > s/qoute/quote/
> > >
> > > In commit message:
> > > s/Handling/handling/
> > >
> > > Best regards,
> > > Konstantin Kolinko
AW: [Patch] Fix for Issue 3046: document security requirement for
hook script arguments
Posted by Markus Schaber <m....@codesys.com>.
Hi,
The patch actually attached.
Best regards
Markus Schaber
CODESYS® a trademark of 3S-Smart Software Solutions GmbH
Inspiring Automation Solutions
3S-Smart Software Solutions GmbH
Dipl.-Inf. Markus Schaber | Product Development Core Technology
Memminger Str. 151 | 87439 Kempten | Germany
Tel. +49-831-54031-979 | Fax +49-831-54031-50
E-Mail: m.schaber@codesys.com | Web: http://www.codesys.com | CODESYS store: http://store.codesys.com
CODESYS forum: http://forum.codesys.com
Managing Directors: Dipl.Inf. Dieter Hess, Dipl.Inf. Manfred Werner | Trade register: Kempten HRB 6186 | Tax ID No.: DE 167014915
> -----Ursprüngliche Nachricht-----
> Von: Markus Schaber [mailto:m.schaber@codesys.com]
> Gesendet: Donnerstag, 19. Juni 2014 14:01
> An: Konstantin Kolinko
> Cc: Subversion Dev (dev@subversion.apache.org)
> Betreff: AW: [Patch] Fix for Issue 3046: document security
> requirement for hook script arguments
>
> Hi,
>
> The second iteration of the patch to fix issue 3046 and also add a
> hint wr/t peg revisions, as inspired by danielsh on IRC while
> discussing issue 2349. This iteration fixes 2 typos.
>
> [[[
> Fix issue 3046 by adding a statement about quoting of parameters.
> Also add a hint about peg revisions, while we are at it.
>
> * subversion/libsvn_repos/repos.c
> (create_hooks): Add a hint about quoting of parameters and url
> handling to the hook templates.
> ]]]
>
> Best regards
>
> Markus Schaber
>
> CODESYS® a trademark of 3S-Smart Software Solutions GmbH
>
> Inspiring Automation Solutions
>
> 3S-Smart Software Solutions GmbH
> Dipl.-Inf. Markus Schaber | Product Development Core Technology
> Memminger Str. 151 | 87439 Kempten | Germany Tel. +49-831-54031-979 |
> Fax +49-831-54031-50
>
> E-Mail: m.schaber@codesys.com | Web: http://www.codesys.com | CODESYS
> store: http://store.codesys.com CODESYS forum:
> http://forum.codesys.com
>
> Managing Directors: Dipl.Inf. Dieter Hess, Dipl.Inf. Manfred Werner |
> Trade register: Kempten HRB 6186 | Tax ID No.: DE 167014915
>
> > -----Ursprüngliche Nachricht-----
> > Von: Konstantin Kolinko [mailto:knst.kolinko@gmail.com]
> > Gesendet: Donnerstag, 19. Juni 2014 13:45
> > An: Markus Schaber
> > Cc: Subversion Dev (dev@subversion.apache.org)
> > Betreff: Re: [Patch] Fix for Issue 3046: document security
> requirement
> > for hook script arguments
> >
> > 2014-06-19 15:08 GMT+04:00 Markus Schaber <m....@codesys.com>:
> > > Hi,
> > >
> > > The attached patch fixes issue 3046 and also adds an hint wr/t
> peg
> > revisions, as inspired by danielsh on IRC.
> > >
> > > [[[
> > > Fix issue 3046 by adding a statement about quoting of parameters.
> > Also
> > > add a hint about peg revisions, while we are at it.
> > >
> > > * subversion/libsvn_repos/repos.c
> > > (create_hooks): Add a hint about quoting of parameters and url
> > > Handling to the hook templates.
> > > ]]]
> >
> > Interesting.
> >
> > A typo in the text:
> > s/qoute/quote/
> >
> > In commit message:
> > s/Handling/handling/
> >
> > Best regards,
> > Konstantin Kolinko
AW: [Patch] Fix for Issue 3046: document security requirement for
hook script arguments
Posted by Markus Schaber <m....@codesys.com>.
Hi,
The second iteration of the patch to fix issue 3046 and also add a hint wr/t peg revisions, as inspired by danielsh on IRC while discussing issue 2349. This iteration fixes 2 typos.
[[[
Fix issue 3046 by adding a statement about quoting of parameters. Also
add a hint about peg revisions, while we are at it.
* subversion/libsvn_repos/repos.c
(create_hooks): Add a hint about quoting of parameters and url
handling to the hook templates.
]]]
Best regards
Markus Schaber
CODESYS® a trademark of 3S-Smart Software Solutions GmbH
Inspiring Automation Solutions
3S-Smart Software Solutions GmbH
Dipl.-Inf. Markus Schaber | Product Development Core Technology
Memminger Str. 151 | 87439 Kempten | Germany
Tel. +49-831-54031-979 | Fax +49-831-54031-50
E-Mail: m.schaber@codesys.com | Web: http://www.codesys.com | CODESYS store: http://store.codesys.com
CODESYS forum: http://forum.codesys.com
Managing Directors: Dipl.Inf. Dieter Hess, Dipl.Inf. Manfred Werner | Trade register: Kempten HRB 6186 | Tax ID No.: DE 167014915
> -----Ursprüngliche Nachricht-----
> Von: Konstantin Kolinko [mailto:knst.kolinko@gmail.com]
> Gesendet: Donnerstag, 19. Juni 2014 13:45
> An: Markus Schaber
> Cc: Subversion Dev (dev@subversion.apache.org)
> Betreff: Re: [Patch] Fix for Issue 3046: document security
> requirement for hook script arguments
>
> 2014-06-19 15:08 GMT+04:00 Markus Schaber <m....@codesys.com>:
> > Hi,
> >
> > The attached patch fixes issue 3046 and also adds an hint wr/t peg
> revisions, as inspired by danielsh on IRC.
> >
> > [[[
> > Fix issue 3046 by adding a statement about quoting of parameters.
> Also
> > add a hint about peg revisions, while we are at it.
> >
> > * subversion/libsvn_repos/repos.c
> > (create_hooks): Add a hint about quoting of parameters and url
> > Handling to the hook templates.
> > ]]]
>
> Interesting.
>
> A typo in the text:
> s/qoute/quote/
>
> In commit message:
> s/Handling/handling/
>
> Best regards,
> Konstantin Kolinko
Re: [Patch] Fix for Issue 3046: document security requirement for
hook script arguments
Posted by Konstantin Kolinko <kn...@gmail.com>.
2014-06-19 15:08 GMT+04:00 Markus Schaber <m....@codesys.com>:
> Hi,
>
> The attached patch fixes issue 3046 and also adds an hint wr/t peg revisions, as inspired by danielsh on IRC.
>
> [[[
> Fix issue 3046 by adding a statement about quoting of parameters. Also
> add a hint about peg revisions, while we are at it.
>
> * subversion/libsvn_repos/repos.c
> (create_hooks): Add a hint about quoting of parameters and url
> Handling to the hook templates.
> ]]]
Interesting.
A typo in the text:
s/qoute/quote/
In commit message:
s/Handling/handling/
Best regards,
Konstantin Kolinko