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:12:27 GMT
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