[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [Patch] Fix for Issue 3046: document security requirement for hook script arguments

From: Daniel Shahaf <d.s_at_daniel.shahaf.name>
Date: Thu, 19 Jun 2014 23:07:49 +0000

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
Received on 2014-06-20 01:08:24 CEST

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.