beehive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Daryl Olander <dolan...@gmail.com>
Subject Re: Page Flow Security Risk
Date Fri, 17 Feb 2006 18:16:38 GMT
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 <dolander@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 <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
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message