On Thu, Aug 9, 2012 at 7:38 PM, Stefan Küng <tortoisesvn_at_gmail.com> wrote:
> On 09.08.2012 19:34, Stefan Fuhrmann wrote:
>>
>> On Thu, Aug 9, 2012 at 4:01 PM, Stefan Küng <tortoisesvn_at_gmail.com> wrote:
>>>
>>> On Thu, Aug 9, 2012 at 12:20 AM, <tortoisesvn_at_googlecode.com> wrote:
>>>>
>>>> Revision: 23145
>>>> Author: stefan.fuhrmann.1974
>>>> Date: Wed Aug 8 15:19:46 2012
>>>> Log: Add optional "force" options to scripts. If set,
>>>> the respective script will be applied without
>>>> prior approval by the users. Also, forced scripts
>>>> cannot be ignored after failure.
>>>
>>>
>>> Uh-oh!
>>> That's exactly why I didn't implement this in earlier versions: this
>>> is a big, big security risk!
>>> Executing something without asking the user first is a NO-NO. It
>>> doesn't matter if its useful or not. And it doesn't matter if its
>>> optional.
>>
>>
>> I agree that executing commands from the repository content is
>> unsafe. I will change that as follows:
>>
>> * hook settings via tsvn:* properties will always require user approval
>> (at least once)
>> * if "force" has been set here, it will disable the "retry w/o hook"
>> button in the progress dialog
>> * "force"d hooks configured via settings menu have - by definition -
>> already been approved by user (or someone with access to the machine)
>>
>> This should eliminate the attack vector of some outside committer
>> executing code on your machine w/o prior approval.
>
>
> Agreed. This would work fine.
Implemented in r23163. Turned out to be a one-liner.
>>
>> Locally configured hooks are as trustworthy as the hook content itself.
>> The approval does not give extra security in that case.
>>
>>> Is there a specific reason you implemented this?
>>> If not, I'd rather revert this change.
>>
>>
>> I've got that from customers (even prior to WANdisco): The hook
>> scripts will tell them "your stuff does not work" but TSVN gives
>> them a "what the hell - commit it anyway!" button.
>
>
> Well, in that case the customer should set up a repository hook, not a
> client side hook. Then such commits could be rejected. That's the only way
> to be sure: TSVN now ships with the command line client (optional install),
> so users could just commit with that instead.
The client-side hook does not need to be water-proof,
in fact, it shouldn't. All I'm trying to prevent here is
stressed users semi-consciously ignoring all warnings
on the way to their goal, e.g. a commit.
And there are scenarios in which a server-side hook
doing the same checks is simply not feasible:
* build / test environment differ from server environment
(e.g. different OSes -> diffs need to sent to a 3rd
machine and results be sent back to the server)
* test to be executed take a very long time
(e.g. commit to stabilization branch requires
successful test suite completion)
>> A "force" option would work perfectly in 1.7.x as it simply hides
>> that button. But in 1.8 we need a more sophisticated set of rules.
>> It will not be watertight: since we won't trust repository dictated
>> hooks, a user must be allowed to not run them but still be able
>> to continue with the actual SVN operation. Everything else would
>> open a DOS attack vector.
>
>
> Please don't merge new features to 1.7.x - that's for bug fixes only.
> Merging the disabling of the retry button is a new feature.
I won't. All I'm saying is that based on the 1.7.x behavior,
the feature implementation would have been simpler.
-- Stefan^2.
--
Join us this October for Subversion Live 2012 – 2 full days of
training, networking, live demos and more! 25% off before Aug. 10th
with discount code “earlybird.”
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
------------------------------------------------------
http://tortoisesvn.tigris.org/ds/viewMessage.do?dsForumId=757&dsMessageId=2997872
To unsubscribe from this discussion, e-mail: [dev-unsubscribe_at_tortoisesvn.tigris.org].
Received on 2012-08-11 15:07:58 CEST