You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@wicket.apache.org by Carl-Eric Menzel <ca...@duesenklipper.de> on 2011/11/20 14:25:40 UTC
Possible security problem with isRenderAllowed(), beforeRender(),
determineVisibility() and component hierarchy.
Hi,
I ran into an odd problem this week. A model fed to a ListView was
calling service methods the current user wasn't allowed to use, and I
was wondering how that could happen. A panel far above this ListView in
the hierarchy had been secured (using Shiro annotations, but that turns
out to not matter at all) and was not supposed to be rendered for this
user. From this I had expected the ListView not to be rendered either,
but here it was trying to assemble itself in onBeforeRender and thus
calling the "forbidden" service methods.
I investigated Component and friends for a bit, and have found a
potential problem.
internalBeforeRender() checks determineVisibility() before doing
anything. So far so good. determineVisibility() then checks
isRenderAllowed() so the application's IAuthorizationStrategy can block
certain components. This is where it goes wrong though:
isRenderAllowed() only looks at FLAG_IS_RENDER_ALLOWED for performance
reasons, and that flag hasn't been set yet! internalPrepareForRender()
only calls setRenderAllowed() *after* beforeRender().
Due to this, the supposedly secure panel was going through its own
beforeRender and thus calling that ListView's beforeRender.
I think this can be a serious problem, such as in my case described
above. I'd expect that if isActionAuthorized(RENDER) is false, the
secured component should basically never get to the beforeRender phase.
My questions are now:
- Is this intentional? If yes, please explain the reasoning behind it,
because it isn't obvious to me.
- If not, can we fix it? My intuitive suggestion would be to simply
move the call to setRenderAllowed() from the end of
internalBeforeRender() (prepareForRender in 1.4) to the beginning of
that method, so beforeRender() can reliably look at that flag.
- If we can fix it, when and where do we fix it? This hit me in 1.4,
and looking at the code it's still there in 1.5. I'd *really* like it
fixed in the last 1.4 release, and certainly in 1.5, given that this
has the potential to impact security.
It's not an API break, but I'm not sure whether the implications for
application behavior are tolerable for all existing applications. On
the other hand, it seems to be a real security problem, so maybe the
change is justified. I'd like some core dev opinions please :-)
If this is in fact a bug, I'm of course willing to provide a ticket and
a patch :-)
Thanks!
Carl-Eric
www.wicketbuch.de
Re: Possible security problem with isRenderAllowed(), beforeRender(),
determineVisibility() and component hierarchy.
Posted by Igor Vaynberg <ig...@gmail.com>.
backported..
-igor
On Wed, Nov 23, 2011 at 1:40 AM, Carl-Eric Menzel <cm...@wicketbuch.de> wrote:
> On Tue, 22 Nov 2011 22:39:17 -0800
> Igor Vaynberg <ig...@gmail.com> wrote:
>
>> fixed in WICKET-4256
>
> Awesome!
> Is this a possible candidate to be fixed in the last 1.4 release too?
>
> Thanks
> Carl-Eric
> www.wicketbuch.de
>
Re: Possible security problem with isRenderAllowed(),
beforeRender(), determineVisibility() and component hierarchy.
Posted by Carl-Eric Menzel <cm...@wicketbuch.de>.
On Tue, 22 Nov 2011 22:39:17 -0800
Igor Vaynberg <ig...@gmail.com> wrote:
> fixed in WICKET-4256
Awesome!
Is this a possible candidate to be fixed in the last 1.4 release too?
Thanks
Carl-Eric
www.wicketbuch.de
Re: Possible security problem with isRenderAllowed(), beforeRender(),
determineVisibility() and component hierarchy.
Posted by Igor Vaynberg <ig...@gmail.com>.
fixed in WICKET-4256
-igor
On Mon, Nov 21, 2011 at 10:36 AM, Igor Vaynberg <ig...@gmail.com> wrote:
> Jira this up so we don't lose it...
>
> -igor
>
> On Nov 20, 2011 5:48 AM, "Carl-Eric Menzel" <ca...@duesenklipper.de>
> wrote:
>>
>> Hi,
>>
>> I ran into an odd problem this week. A model fed to a ListView was
>> calling service methods the current user wasn't allowed to use, and I
>> was wondering how that could happen. A panel far above this ListView in
>> the hierarchy had been secured (using Shiro annotations, but that turns
>> out to not matter at all) and was not supposed to be rendered for this
>> user. From this I had expected the ListView not to be rendered either,
>> but here it was trying to assemble itself in onBeforeRender and thus
>> calling the "forbidden" service methods.
>>
>> I investigated Component and friends for a bit, and have found a
>> potential problem.
>>
>> internalBeforeRender() checks determineVisibility() before doing
>> anything. So far so good. determineVisibility() then checks
>> isRenderAllowed() so the application's IAuthorizationStrategy can block
>> certain components. This is where it goes wrong though:
>> isRenderAllowed() only looks at FLAG_IS_RENDER_ALLOWED for performance
>> reasons, and that flag hasn't been set yet! internalPrepareForRender()
>> only calls setRenderAllowed() *after* beforeRender().
>>
>> Due to this, the supposedly secure panel was going through its own
>> beforeRender and thus calling that ListView's beforeRender.
>>
>> I think this can be a serious problem, such as in my case described
>> above. I'd expect that if isActionAuthorized(RENDER) is false, the
>> secured component should basically never get to the beforeRender phase.
>> My questions are now:
>>
>> - Is this intentional? If yes, please explain the reasoning behind it,
>> because it isn't obvious to me.
>>
>> - If not, can we fix it? My intuitive suggestion would be to simply
>> move the call to setRenderAllowed() from the end of
>> internalBeforeRender() (prepareForRender in 1.4) to the beginning of
>> that method, so beforeRender() can reliably look at that flag.
>>
>> - If we can fix it, when and where do we fix it? This hit me in 1.4,
>> and looking at the code it's still there in 1.5. I'd *really* like it
>> fixed in the last 1.4 release, and certainly in 1.5, given that this
>> has the potential to impact security.
>>
>> It's not an API break, but I'm not sure whether the implications for
>> application behavior are tolerable for all existing applications. On
>> the other hand, it seems to be a real security problem, so maybe the
>> change is justified. I'd like some core dev opinions please :-)
>>
>> If this is in fact a bug, I'm of course willing to provide a ticket and
>> a patch :-)
>>
>> Thanks!
>>
>> Carl-Eric
>> www.wicketbuch.de
>
Re: Possible security problem with isRenderAllowed(), beforeRender(),
determineVisibility() and component hierarchy.
Posted by Igor Vaynberg <ig...@gmail.com>.
Jira this up so we don't lose it...
-igor
On Nov 20, 2011 5:48 AM, "Carl-Eric Menzel" <ca...@duesenklipper.de>
wrote:
> Hi,
>
> I ran into an odd problem this week. A model fed to a ListView was
> calling service methods the current user wasn't allowed to use, and I
> was wondering how that could happen. A panel far above this ListView in
> the hierarchy had been secured (using Shiro annotations, but that turns
> out to not matter at all) and was not supposed to be rendered for this
> user. From this I had expected the ListView not to be rendered either,
> but here it was trying to assemble itself in onBeforeRender and thus
> calling the "forbidden" service methods.
>
> I investigated Component and friends for a bit, and have found a
> potential problem.
>
> internalBeforeRender() checks determineVisibility() before doing
> anything. So far so good. determineVisibility() then checks
> isRenderAllowed() so the application's IAuthorizationStrategy can block
> certain components. This is where it goes wrong though:
> isRenderAllowed() only looks at FLAG_IS_RENDER_ALLOWED for performance
> reasons, and that flag hasn't been set yet! internalPrepareForRender()
> only calls setRenderAllowed() *after* beforeRender().
>
> Due to this, the supposedly secure panel was going through its own
> beforeRender and thus calling that ListView's beforeRender.
>
> I think this can be a serious problem, such as in my case described
> above. I'd expect that if isActionAuthorized(RENDER) is false, the
> secured component should basically never get to the beforeRender phase.
> My questions are now:
>
> - Is this intentional? If yes, please explain the reasoning behind it,
> because it isn't obvious to me.
>
> - If not, can we fix it? My intuitive suggestion would be to simply
> move the call to setRenderAllowed() from the end of
> internalBeforeRender() (prepareForRender in 1.4) to the beginning of
> that method, so beforeRender() can reliably look at that flag.
>
> - If we can fix it, when and where do we fix it? This hit me in 1.4,
> and looking at the code it's still there in 1.5. I'd *really* like it
> fixed in the last 1.4 release, and certainly in 1.5, given that this
> has the potential to impact security.
>
> It's not an API break, but I'm not sure whether the implications for
> application behavior are tolerable for all existing applications. On
> the other hand, it seems to be a real security problem, so maybe the
> change is justified. I'd like some core dev opinions please :-)
>
> If this is in fact a bug, I'm of course willing to provide a ticket and
> a patch :-)
>
> Thanks!
>
> Carl-Eric
> www.wicketbuch.de
>
Re: Possible security problem with isRenderAllowed(),
beforeRender(), determineVisibility() and component hierarchy.
Posted by Carl-Eric Menzel <cm...@wicketbuch.de>.
On Mon, 21 Nov 2011 09:55:44 +0100
Daniel Stoch <da...@gmail.com> wrote:
> Maybe it is a related problem to this one?
> http://apache-wicket.1842946.n4.nabble.com/setRenderAllowed-called-for-invisible-components-td3790937.html
It is somewhat related, but I think setRenderAllowed should be called
for all components, since up to and including onBeforeRender visibility
can still be changed.
Carl-Eric
www.wicketbuch.de
Re: Possible security problem with isRenderAllowed(), beforeRender(),
determineVisibility() and component hierarchy.
Posted by Daniel Stoch <da...@gmail.com>.
Maybe it is a related problem to this one?
http://apache-wicket.1842946.n4.nabble.com/setRenderAllowed-called-for-invisible-components-td3790937.html
Here it is a quickstart app:
http://dl.dropbox.com/u/138504/wicket/render-allowed-bug.zip
--
Daniel