struts-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Musachy Barroso" <musa...@gmail.com>
Subject Re: ParameterFilterInterceptor security issue
Date Thu, 14 Aug 2008 12:59:49 GMT
Is it possible to get email notifications on the reviews?

musachy

On 8/14/08, Dave Newton <newton.dave@yahoo.com> wrote:
> Did you fix the "Memer" spelling error I noted on the review?
>
>
> --- On Wed, 8/13/08, Musachy Barroso <musachy@gmail.com> wrote:
>
>> From: Musachy Barroso <musachy@gmail.com>
>> Subject: Re: ParameterFilterInterceptor security issue
>> To: "Struts Developers List" <dev@struts.apache.org>
>> Date: Wednesday, August 13, 2008, 10:41 AM
>> the patch for WW-2761 was committed to xwork trunk
>>
>> On 8/12/08, Musachy Barroso <musachy@gmail.com>
>> wrote:
>> > 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
>> <musachy@gmail.com> 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
>> <Brian.Relph@cerner.com>
>> >> 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:musachy@gmail.com]
>> >>> 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 <musachy@gmail.com>
>> >>> 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 <gielen@it-neering.net>
>> >>>> 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:
>> dev-unsubscribe@struts.apache.org For
>> >>>>> additional commands, e-mail:
>> dev-help@struts.apache.org
>> >>>>>
>> >>>>>
>> >>>>
>> >>>>
>> >>>>
>> >>>> --
>> >>>> "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:
>> dev-unsubscribe@struts.apache.org For additional
>> >>> commands, e-mail: dev-help@struts.apache.org
>> >>>
>> >>>
>> ----------------------------------------------------------------------
>> >>> 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:
>> dev-unsubscribe@struts.apache.org
>> >>> For additional commands, e-mail:
>> dev-help@struts.apache.org
>> >>>
>> >>>
>> >>
>> >>
>> >>
>> >> --
>> >> "Hey you! Would you help me to carry the
>> stone?" Pink Floyd
>> >>
>> >
>> >
>> >
>> > --
>> > "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: dev-unsubscribe@struts.apache.org
>> For additional commands, e-mail: dev-help@struts.apache.org
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
> For additional commands, e-mail: dev-help@struts.apache.org
>
>


-- 
"Hey you! Would you help me to carry the stone?" Pink Floyd

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org


Mime
View raw message