You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@beehive.apache.org by Daryl Olander <do...@gmail.com> on 2006/02/17 16:17:31 UTC
Page Flow Security Risk
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?
Re: Page Flow Security Risk
Posted by Rich Feit <ri...@gmail.com>.
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 <ri...@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 <ri...@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 <ri...@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?
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>
>
>
Re: Page Flow Security Risk
Posted by Rich Feit <ri...@gmail.com>.
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 <do...@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 <ri...@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 <ri...@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 <ri...@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?
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>
>>
>
>
Re: Page Flow Security Risk
Posted by Daryl Olander <do...@gmail.com>.
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 <do...@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 <ri...@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 <ri...@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 <ri...@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?
> > >>>>>
> > >>>>>
> > >>>>>
> > >>>>>
> > >>>
> > >
> > >
> >
>
>
Re: Page Flow Security Risk
Posted by Daryl Olander <do...@gmail.com>.
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 <ri...@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 <ri...@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 <ri...@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?
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>
> >
> >
>
Re: Page Flow Security Risk
Posted by Rich Feit <ri...@gmail.com>.
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 <ri...@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 <ri...@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?
>>>>>
>>>>>
>>>>>
>>>>>
>>>
>
>
Re: Page Flow Security Risk
Posted by Daryl Olander <do...@gmail.com>.
Jiri bug 1069 has been created. I assigned it to Carlin. We need to get
this fixed ASAP so we can role a 1.0.2 release.
On 2/17/06, Daryl Olander <do...@gmail.com> 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 <ri...@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 <ri...@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?
> > >>>
> > >>>
> > >>>
> > >
> > >
> >
>
>
Re: Page Flow Security Risk
Posted by Daryl Olander <do...@gmail.com>.
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 <ri...@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 <ri...@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?
> >>>
> >>>
> >>>
> >
> >
>
Re: Page Flow Security Risk
Posted by Rich Feit <ri...@gmail.com>.
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 <ri...@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?
>>>
>>>
>>>
>
>
Re: Page Flow Security Risk
Posted by Daryl Olander <do...@gmail.com>.
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 <ri...@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?
> >
> >
>
Re: Page Flow Security Risk
Posted by Rich Feit <ri...@gmail.com>.
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?
>
>