beehive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Rich Feit <richf...@gmail.com>
Subject Re: Page Flow Security Risk
Date Fri, 17 Feb 2006 20:41:08 GMT
I agree it's a risk -- just wanted to know if you saw anything 
specific.  I support the idea of locking it down...

Daryl Olander wrote:
> It seems to be a risk if someone expects only page A to be accessable from
> an action and all of the sudden page Bis being navigated to.  Futher, I just
> tweaked one thing in those structures.  You're really the only one that
> would understand what other things could be done :-)
>
> It just seems like anytime a user can override navigation out of an action
> that there could be some security issues.  I agree that they can't access
> things they don't have access to because we always forward to the JSP.  But,
> we pass page inputs, etc along those links.  These may actually have
> consequences that differ from just directly hitting the .JSPs.
>
> At this point, it might depend on what the App / PageFlow / JSP all do and
> how the data flows in the application.  I just worry some where out there
> there could be a problem.  It's not a huge risk, but it is a risk.
>
> On 2/17/06, Rich Feit <richfeit@gmail.com> wrote:
>   
>> First, I agree that this needs to be fixed.
>> Second, just so I understand... this is a hole because the user might
>> have set up the webapp to prevent direct browser URL access to
>> whatever's being forwarded to?
>>
>> Daryl Olander wrote:
>>     
>>> OK...We've got a hole...
>>>
>>> I have the following form that change the forward path to /bar.jsp
>>>
>>>   <netui:form action="submit">
>>>     <netui:hidden dataSource="pageFlow.currentPageInfo.forward.path"
>>> dataInput="/bar.jsp"/>
>>>     <netui:button value="submit" />
>>>   </netui:form>
>>>
>>> I also have the following action in my page flow.
>>>
>>>     @Jpf.Action(
>>>         forwards={
>>>            @Jpf.Forward(name="index", navigateTo =
>>> Jpf.NavigateTo.currentPage)
>>>         }
>>>     )
>>>     protected Forward submit(Form form)
>>>     {
>>>         return new Forward("index");
>>>     }
>>>
>>> If the current page is index.jsp, this should navigate back to that,
>>>       
>> when
>>     
>>> the form is submitted it will navigate to bar.jsp.  In my mind this is
>>> actually a security hole.  I can dynamically change the navigation
>>> externally in this situation.  I haven't played around with the other
>>> exposed properties (currentPageInfo, previousPageInfo,
>>>       
>> previousActionInfo)
>>     
>>> all expose the same JavaBean that is not immutable.
>>>
>>> I'm going to open a Jiri bug on this.  I think this is critical and
>>>       
>> needs to
>>     
>>> be fixed now.  My suggestion is that we rename these methods on the
>>> PageFlowController so they aren't picked up as JavaBean properties.
>>>
>>> I suggest we do this to:
>>>
>>> currentPageInfo
>>> previousPageInfo
>>> previousActionInfo
>>> modeulConfig
>>> actions
>>>
>>> We need to spin a new release on this.
>>>
>>>
>>> On 2/17/06, Rich Feit <richfeit@gmail.com> wrote:
>>>
>>>       
>>>> 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?
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>               
>>>       
>
>   

Mime
View raw message