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:45 GMT
I think the best thing to do would be to make the methods protected 
(they're legitimately used in derived classes), and offer public 
non-getter accessors as necessary.

Daryl Olander wrote:
> BTW, What do you think is the best solution.  I believe we should just
> rename these methods.  Even if they are public, they are probably not called
> often by page flow authors.
>
> On 2/17/06, Daryl Olander <dolander@gmail.com> 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