Jira ticket with patch: https://issues.apache.org/struts/browse/WW-2761
Crucible review: http://fisheye6.atlassian.com/cru/CR-10/preview
I added this tests to the testcases:
put("blah", "This is blah"); //assert it is good
put("name", "try_1"); //assert it is not set
put("(name)", "try_2"); //assert it is not set
put("['name']", "try_3"); //assert it is not set
if you can think of other tests, suggest them to add them. Making this
patch I found a bug in parameter interceptor which I had to fix:
protected boolean isAccepted(String paramName) {
if (!this.acceptedParams.isEmpty()) {
for (Pattern pattern : acceptedParams) {
Matcher matcher = pattern.matcher(paramName);
if (matcher.matches()) {
return true; // <-- was returning false here
}
}
}
return acceptedPattern.matcher(paramName).matches();
}
please review it and let me know. I will commit it later, and will
also look into Brian Relph's patch (WW-2760 )
musachy
On Tue, Aug 12, 2008 at 10:27 AM, Musachy Barroso <[EMAIL PROTECTED]> wrote:
> I think that would be great, create a jira ticket, attach a patch to
> it and we will check it out. I am writing a patch for the approach
> that I mentioned before, which will provide the same behavior using
> xml configuration, but having annotations as an option would be good.
>
> musachy
>
> On Tue, Aug 12, 2008 at 10:22 AM, Relph,Brian <[EMAIL PROTECTED]> wrote:
>>
>> I wrote an annotation based parameters interceptor that extends the current
>> parameters interceptor while allowing you to configure the default "accept"
>> policy for an actions properties, as well as a per-property annotation that
>> can override the action's policy. This lets you use the same interceptor /
>> interceptor stack for all actions, and configure each individually to accept
>> or reject parameters. I would still like to add some regex support to the
>> action annotation. Would this interest you?
>>
>> Brian Relph
>>
>> -----Original Message-----
>> From: Musachy Barroso [mailto:[EMAIL PROTECTED]
>> Sent: Tuesday, August 12, 2008 8:53 AM
>> To: Struts Developers List
>> Subject: Re: ParameterFilterInterceptor security issue
>>
>> I forgot to say, that this would prevent all the OGNL expression tricks,
>> because the property name that is passed to MemberAccess to be checked, is
>> the actual property name, and not an expression.
>>
>> musachy
>>
>> On Tue, Aug 12, 2008 at 9:48 AM, Musachy Barroso <[EMAIL PROTECTED]> wrote:
>>> It seems to me like there is an elegant solution to this. We can
>>> rename StaticMemeberAccess to SecurityMemeberAccess, and in there not
>>> only block static member access, but also fields that can be
>>> configured using regular expressions. The params interceptor would
>>> just set these fields before binding the params.
>>>
>>> //OGNL parameter binding just went to #2 in my "must kill" list :)
>>>
>>> musachy
>>>
>>> On Tue, Aug 12, 2008 at 9:14 AM, Rene Gielen <[EMAIL PROTECTED]> wrote:
>>>>
>>>> Am Di, 12.08.2008, 14:20, schrieb Jeromy Evans:
>>>>>
>>>>> This relates to Musachy's recent proposal to remove OGNL entirely
>>>>> from the parameter-setting process. Which I think is a very good idea.
>>>>>
>>>>
>>>> Indeed removing OGNL for parameters would fix this issue, but even if
>>>> we would decide to do so this won't be trivial and might have many
>>>> side effects.
>>>>
>>>>> If I've understood correctly, currently there is no way to filter
>>>>> the parameter names, using regex or otherwise, other than to verify
>>>>> them use a whitelist of valid names.
>>>>>
>>>>
>>>> You can blacklist parameter names in the ParameterInterceptor ref,
>>>> including the possibility to define RegExp patterns. The latter one
>>>> is not possible for the ParameterFilterInterceptor right now, which I
>>>> think is a feature we should add.
>>>>
>>>> Jelmer, would you mind creating an Jira issue for that?
>>>> https://issues.apache.org/struts/
>>>>
>>>> - Rene
>>>>
>>>>> jelmer wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> I was looking into an easy way to prevent people binding on fields
>>>>>> they shouldn't be binding on.
>>>>>>
>>>>>> Say you have a User object, you do not want people to be able to
>>>>>> bind on the isAdmin property.
>>>>>>
>>>>>> Various people remommended using the ParameterFilterInterceptor for
>>>>>> this but it seems to be flatout broken
>>>>>>
>>>>>> When you configure an action like this
>>>>>>
>>>>>> <action name="test" class="com.webapp.action.TestAction">
>>>>>> <interceptor-ref name="param-namevalue-filter">
>>>>>> <param name="blocked">name</param>
>>>>>> </interceptor-ref>
>>>>>> <interceptor-ref name="params"/> </action>
>>>>>>
>>>>>> then this wont work :
>>>>>>
>>>>>> /test.action?name=myname
>>>>>>
>>>>>> but this does :
>>>>>>
>>>>>> /test.action?(name)=jelmer
>>>>>>
>>>>>> and so does this
>>>>>>
>>>>>> /test.action?((name))=jelmer
>>>>>>
>>>>>> And so on, infact it is impossible to block any parameter
>>>>>> effectively with the ParameterFilterInterceptor.
>>>>>>
>>>>>>
>>>>>> Btw. I am aware that there is also the excludeParams method on the
>>>>>> ParametersInterceptor that accepts a regexp, so theoretically you
>>>>>> could use this to block parameters effectively but it would be
>>>>>> extremely hard to write a correct regexp for it. Also I havent
>>>>>> found a way to configure both params interceptors in a
>>>>>> paramsPrepareParamsStack. This will only configure the first params
>>>>>> interceptor in the stack
>>>>>>
>>>>>> <interceptor-ref name="clientCrudStack">
>>>>>> <param name="params.excludeParams">some pattern</param>
>>>>>> </interceptor-ref>
>>>>>>
>>>>>> Struts really seems to be lacking in this area.
>>>>>>
>>>>>>
>>>>
>>>>
>>>>
>>>> ---------------------------------------------------------------------
>>>> To unsubscribe, e-mail: [EMAIL PROTECTED] For
>>>> additional commands, e-mail: [EMAIL PROTECTED]
>>>>
>>>>
>>>
>>>
>>>
>>> --
>>> "Hey you! Would you help me to carry the stone?" Pink Floyd
>>>
>>
>>
>>
>> --
>> "Hey you! Would you help me to carry the stone?" Pink Floyd
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail:
>> [EMAIL PROTECTED]
>>
>> ----------------------------------------------------------------------
>> CONFIDENTIALITY NOTICE This message and any included attachments are from
>> Cerner Corporation and are intended only for the addressee. The information
>> contained in this message is confidential and may constitute inside or
>> non-public information under international, federal, or state securities
>> laws. Unauthorized forwarding, printing, copying, distribution, or use of
>> such information is strictly prohibited and may be unlawful. If you are not
>> the addressee, please promptly delete this message and notify the sender of
>> the delivery error by e-mail or you may call Cerner's corporate offices in
>> Kansas City, Missouri, U.S.A at (+1) (816)221-1024.
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: [EMAIL PROTECTED]
>> For additional commands, e-mail: [EMAIL PROTECTED]
>>
>>
>
>
>
> --
> "Hey you! Would you help me to carry the stone?" Pink Floyd
>
--
"Hey you! Would you help me to carry the stone?" Pink Floyd
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
