Mailing-List: contact dev-help@beehive.apache.org; run by ezmlm
Precedence: bulk
Reply-To: "Beehive Developers" <dev@beehive.apache.org>
Received-SPF: pass (asf.osuosl.org: domain of richfeit@gmail.com designates
 64.233.184.200 as permitted sender)
DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws;
        s=beta; d=gmail.com;
        h=received:message-id:date:from:user-agent:mime-version:to:subject:references:in-reply-to:content-type:content-transfer-encoding;
        b=Ym/LOqAjVk6wyulmNxfrZMCGL2nnRchxGQKYsUT/2r2YSgCfTokXuhJNRtsjt2UXUfnpWJ5qQOSaUm3rpRAvC6suk/PALcNNrHJazrrbCSY2zLOMUoU1DwsEQU/wl+pSEUxlx9rBjzJiOC+g6e1gGkiV9Nsfjwd5VOFm1b/X45E=
Message-ID: <43F60E6F.8010405@gmail.com>
Date: Fri, 17 Feb 2006 10:57:03 -0700
From: Rich Feit <richfeit@gmail.com>
User-Agent: Thunderbird 1.5 (Windows/20051201)
MIME-Version: 1.0
To: Beehive Developers <dev@beehive.apache.org>
Subject: Re: Page Flow Security Risk
References: <40f026540602170717i294b8abh344811c5841f5f42@mail.gmail.com>
	 <43F5F9C9.3010505@gmail.com>
 <40f026540602170859u44a36c48vb056e7fbbcc99c1d@mail.gmail.com>
In-Reply-To: <40f026540602170859u44a36c48vb056e7fbbcc99c1d@mail.gmail.com>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

Well, the Struts ModuleConfig and related objects are all immutable and 
always have been.  Are you seeing any other objects we expose that 
aren't in our control?  I agree that it's brittle -- feel free to add 
your option #2 if you're worried about continued support for the 
deprecated base class.

Daryl Olander wrote:
> I agree we need to move to a POJO model....
>
> I think the issue is that we expose objects like the struts config that are
> developed independently of Beehive which may have setters which could open
> up security holes.  It's also the case that we expose object that expose
> object and underlying modification to the runtime could open up a security
> hole and no one would know.  The just simply added a new feature and exposed
> a setter.  This is certainly a brittle design even if we verified all of the
> current paths.
>
> I don't think opion #3 is viable because we still need to support for at
> least some time the current model even if it's deprecated.
>
> On 2/17/06, Rich Feit <richfeit@gmail.com> wrote:
>   
>> It's not happenstance.  When we still extended Struts Action, I had
>> workarounds in there to prevent access to dangerous base class objects
>> (like getServlet()).  In general I allowed public getters for
>> unmodifiable objects.  If we're exposing something dangerous, then it's
>> my fault -- it isn't just bad luck.
>>
>>     
>>> Access to the shared flow Map is luckily illegal when the expressions
>>>       
>> are being updated.
>> I think I *did* expose a potential security hole by not returning
>> Collections.unmodifiableMap() from
>> FlowControllerFactory.getSharedFlowsForPath() -- this needs to be
>> fixed.  Why is access to the Map illegal currently?
>>
>>
>> I would vote for this option:
>>
>>     3) Verify that what's currently exposed is safe, and move to the
>> POJO-pageflow model (deprecate use of the base class).
>>
>> Rich
>>
>> Daryl Olander wrote:
>>     
>>> I've been looking at a possible security risk in page flows.  At the
>>>       
>> moment,
>>     
>>> I don't think we have an actual security hole, but I think we have a
>>> situation where we could create one very easy.
>>>
>>> The issue is that there are a number of public properties on the
>>> PageFlowController class.  There are public getters that give access to
>>>       
>> low
>>     
>>> level structures.  For example, you can get the ModuleConfig from
>>>       
>> Struts,
>>     
>>> the ActionForm, ActionServlet, the map of shared flows, etc.  This issue
>>> arises because you can submit a form that contains a hidden field that
>>>       
>> would
>>     
>>> update these data items.
>>>
>>>   <netui:form action="submit">
>>>     <netui:hidden dataSource="pageFlow.moduleConfig.prefix"
>>> dataInput="value"/>
>>>     <netui:button value="submit" />
>>>   </netui:form>
>>>
>>> In the above code, this could modify the Struts ModuleConfig structure
>>>       
>> and
>>     
>>> set the prefix value to "value".
>>>
>>> In fact, in looking around at this for a little while, I couldn't find
>>> anything you can do that is destructive.  The Struts config information
>>>       
>> is
>>     
>>> frozen, so the code above results in an IllegalStateException.  Access
>>>       
>> to
>>     
>>> the shared flow Map is luckily illegal when the expressions are being
>>> updated.
>>>
>>> I think that it's purely happenstance that we are not exposing a
>>>       
>> security
>>     
>>> hole here. In fact, with a bit more playing round, we might find that we
>>> really are exposing a hole.  We need to prevent page flow updates for
>>>       
>> these
>>     
>>> base class properties.  There seems to be a number of ways we could
>>>       
>> solve
>>     
>>> this,
>>>
>>> 1) We could prevent all update to PageFlow.  This is a pretty radical
>>> solution because it's a backward incompatible change.
>>> 2) We could create a list of properties that can't be updated.  The list
>>> could be created automatically through reflection.
>>>
>>> Right now, I would lean toward 2, but I think we should have more
>>>       
>> discussion
>>     
>>> of this issue.
>>>
>>> Thoughts?
>>>
>>>
>>>       
>
>