struts-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dave Newton <newton.d...@yahoo.com>
Subject Re: ParameterFilterInterceptor security issue
Date Thu, 14 Aug 2008 10:27:26 GMT
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


Mime
View raw message