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 20:28:57 GMT
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
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message