Return-Path: Delivered-To: apmail-struts-dev-archive@www.apache.org Received: (qmail 64632 invoked from network); 12 Aug 2008 15:02:45 -0000 Received: from hermes.apache.org (HELO mail.apache.org) (140.211.11.2) by minotaur.apache.org with SMTP; 12 Aug 2008 15:02:45 -0000 Received: (qmail 84467 invoked by uid 500); 12 Aug 2008 15:02:43 -0000 Delivered-To: apmail-struts-dev-archive@struts.apache.org Received: (qmail 84128 invoked by uid 500); 12 Aug 2008 15:02:42 -0000 Mailing-List: contact dev-help@struts.apache.org; run by ezmlm Precedence: bulk List-Unsubscribe: List-Help: List-Post: List-Id: "Struts Developers List" Reply-To: "Struts Developers List" Delivered-To: mailing list dev@struts.apache.org Received: (qmail 84117 invoked by uid 99); 12 Aug 2008 15:02:42 -0000 Received: from athena.apache.org (HELO athena.apache.org) (140.211.11.136) by apache.org (qpsmtpd/0.29) with ESMTP; Tue, 12 Aug 2008 08:02:42 -0700 X-ASF-Spam-Status: No, hits=-0.0 required=10.0 tests=SPF_PASS X-Spam-Check-By: apache.org Received-SPF: pass (athena.apache.org: domain of musachy@gmail.com designates 64.233.166.178 as permitted sender) Received: from [64.233.166.178] (HELO py-out-1112.google.com) (64.233.166.178) by apache.org (qpsmtpd/0.29) with ESMTP; Tue, 12 Aug 2008 15:01:44 +0000 Received: by py-out-1112.google.com with SMTP id p76so1522103pyb.10 for ; Tue, 12 Aug 2008 08:01:54 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:received:received:message-id:date:from:to :subject:in-reply-to:mime-version:content-type :content-transfer-encoding:content-disposition:references; bh=mz1h94D0y3qzSCkf3p0lkEQ9W5X8Fm69scgEpX9KagQ=; b=l3o3xtLR9CVyHOrKhEjYds8NA9PQnXmhBCfWzYyoSeRx32R+71QuRXRcm0ExQD08AP BcbH1Z0BJUVEB3kkmZ0nU+ffIN1Mqn31cQN7q6m3ZM/sIRETZIWg+M1fDKK9Gtmd72jL rnGtRwP0cheBvhXpDBK6XlANIERhzocFlfkCI= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=message-id:date:from:to:subject:in-reply-to:mime-version :content-type:content-transfer-encoding:content-disposition :references; b=J7ITJL6N6DCNvPTN0xujqPmXQHgNl7zItgZnNvsueeDDhxKe0EvjrQdq3GkG46bEvi YW2oGfwgi7U23wAEWXPuTvRJoGm68pBYeVtYHNCqGUibQH2hk3K7XN1SGQ/v2+v7or+B PxKfg5l0hak0ppYhnvEFGm8i8AuASl+ylZbss= Received: by 10.141.180.5 with SMTP id h5mr4426837rvp.240.1218553314183; Tue, 12 Aug 2008 08:01:54 -0700 (PDT) Received: by 10.141.87.2 with HTTP; Tue, 12 Aug 2008 08:01:54 -0700 (PDT) Message-ID: Date: Tue, 12 Aug 2008 11:01:54 -0400 From: "Musachy Barroso" To: "Struts Developers List" Subject: Re: ParameterFilterInterceptor security issue In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Content-Disposition: inline References: <48A1800A.3000104@blueskyminds.com.au> <39687.80.87.162.65.1218546847.squirrel@webmail.nrgie.net> <019EB22ED464A146A26211222DD06E17018099EC@EMAIL02.cerner.net> X-Virus-Checked: Checked by ClamAV on apache.org 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 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 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 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 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 >>>>>> >>>>>> >>>>>> >>>>>> name >>>>>> >>>>>> >>>>>> >>>>>> 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 >>>>>> >>>>>> >>>>>> some pattern >>>>>> >>>>>> >>>>>> 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 --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org For additional commands, e-mail: dev-help@struts.apache.org