Max length for OGNL expression

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

Max length for OGNL expression

Yasser Zamani-2
Hi,

I thought it might be nice to add a config element which confines the length
of OGNL expression that Struts is going to evaluate. It is going to make
hackers life harder :)

How do you see it?

Best.


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Max length for OGNL expression

info@flyingfischer.ch
Seems to me not to be the right place to correct any possible problems,
and far off any related root of a possible issue.

The config would definitively need an option to be disabled totally. I
expect very unexpected and hard to trace side effects, depending on the
application in place.

Markus

Am 15.09.19 um 09:58 schrieb Yasser Zamani:

> Hi,
>
> I thought it might be nice to add a config element which confines the length
> of OGNL expression that Struts is going to evaluate. It is going to make
> hackers life harder :)
>
> How do you see it?
>
> Best.
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

AW: Max length for OGNL expression

Christoph.Nenning
I agree with this. Basically I like the idea to limit length of ognl and I think it would increase security. But IMHO it is likely to cause issues in applications and thus applications must be able to control it.

Regards,
Christoph


> Seems to me not to be the right place to correct any possible problems,
> and far off any related root of a possible issue.
>
> The config would definitively need an option to be disabled totally. I
> expect very unexpected and hard to trace side effects, depending on the
> application in place.
>
> Markus
>
> Am 15.09.19 um 09:58 schrieb Yasser Zamani:
> > Hi,
> >
> > I thought it might be nice to add a config element which confines the length
> > of OGNL expression that Struts is going to evaluate. It is going to make
> > hackers life harder :)
> >
> > How do you see it?
> >
> > Best.
> >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: [hidden email]
> > For additional commands, e-mail: [hidden email]
> >
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

RE: Max length for OGNL expression

Yasser Zamani-2
Thanks Markus and Christoph! Please see inline and see if it satisfies those challenges.

>-----Original Message-----
>From: [hidden email] <[hidden email]>
>Sent: Monday, September 16, 2019 11:39 AM
>To: [hidden email]
>Subject: AW: Max length for OGNL expression
>
>I agree with this. Basically I like the idea to limit length of ognl and I think it would
>increase security. But IMHO it is likely to cause issues in applications and thus
>applications must be able to control it.

Yes. By "config element" I meant they will be able to override it or set it to a very large number to disable it in their struts.xml. Please see also below.

>
>Regards,
>Christoph
>
>
>> Seems to me not to be the right place to correct any possible
>> problems, and far off any related root of a possible issue.
>>
>> The config would definitively need an option to be disabled totally. I
>> expect very unexpected and hard to trace side effects, depending on
>> the application in place.

Yes I have already thought that users might have long expressions e.g. <s:if test="<long expression>" but I think there exists an N such that no user has any expression longer than N. For example I guess N=200. Somebody might claim N=500 but at bottom we can discuss and find a default for N.

Furthermore we will log.warn() such unlikely expressions before blocking them, so user will be able to find and amend them which makes code more readable and maintainable -- obviously a raw expression longer than 200 should be hard to maintain and read :)

Best Regards.

>>
>> Markus
>>
>> Am 15.09.19 um 09:58 schrieb Yasser Zamani:
>> > Hi,
>> >
>> > I thought it might be nice to add a config element which confines
>> > the length of OGNL expression that Struts is going to evaluate. It
>> > is going to make hackers life harder :)
>> >
>> > How do you see it?
>> >
>> > Best.
>> >
>> >
>> > --------------------------------------------------------------------
>> > - To unsubscribe, e-mail: [hidden email] For
>> > additional commands, e-mail: [hidden email]
>> >
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: [hidden email] For
>> additional commands, e-mail: [hidden email]


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Max length for OGNL expression

info@flyingfischer.ch
Dear Yasser

we definitively need an option to totally disable this "feature". It
really depends on what kind of application you deploy.

Logging a warning seems appropriate. But we should avoid logging a
warning while the "feature" is disabled.

I also fear that this will lead to vulnerable applications, because the
"default" settings are not vulnerable to a given attack and therefore
the framework "needs no fixing". As I said before: the proposed fix is
at the wrong place. I may be mistaken, but we should try to find
solutions at the core of the problem.

However, as long as we have an option to disable this, it should work out.

Markus


Am 16.09.19 um 14:09 schrieb Yasser Zamani:

> Thanks Markus and Christoph! Please see inline and see if it satisfies those challenges.
>
>> -----Original Message-----
>> From: [hidden email] <[hidden email]>
>> Sent: Monday, September 16, 2019 11:39 AM
>> To: [hidden email]
>> Subject: AW: Max length for OGNL expression
>>
>> I agree with this. Basically I like the idea to limit length of ognl and I think it would
>> increase security. But IMHO it is likely to cause issues in applications and thus
>> applications must be able to control it.
> Yes. By "config element" I meant they will be able to override it or set it to a very large number to disable it in their struts.xml. Please see also below.
>
>> Regards,
>> Christoph
>>
>>
>>> Seems to me not to be the right place to correct any possible
>>> problems, and far off any related root of a possible issue.
>>>
>>> The config would definitively need an option to be disabled totally. I
>>> expect very unexpected and hard to trace side effects, depending on
>>> the application in place.
> Yes I have already thought that users might have long expressions e.g. <s:if test="<long expression>" but I think there exists an N such that no user has any expression longer than N. For example I guess N=200. Somebody might claim N=500 but at bottom we can discuss and find a default for N.
>
> Furthermore we will log.warn() such unlikely expressions before blocking them, so user will be able to find and amend them which makes code more readable and maintainable -- obviously a raw expression longer than 200 should be hard to maintain and read :)
>
> Best Regards.
>
>>> Markus
>>>
>>> Am 15.09.19 um 09:58 schrieb Yasser Zamani:
>>>> Hi,
>>>>
>>>> I thought it might be nice to add a config element which confines
>>>> the length of OGNL expression that Struts is going to evaluate. It
>>>> is going to make hackers life harder :)
>>>>
>>>> How do you see it?
>>>>
>>>> Best.
>>>>
>>>>
>>>> --------------------------------------------------------------------
>>>> - To unsubscribe, e-mail: [hidden email] For
>>>> additional commands, e-mail: [hidden email]
>>>>
>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: [hidden email] For
>>> additional commands, e-mail: [hidden email]
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

RE: Max length for OGNL expression

Yasser Zamani-2


>-----Original Message-----
>From: [hidden email] <[hidden email]>
>Sent: Monday, September 16, 2019 4:58 PM
>To: [hidden email]
>Subject: Re: Max length for OGNL expression
>
>Dear Yasser
>
>we definitively need an option to totally disable this "feature". It really depends
>on what kind of application you deploy.

Yes it's going to be user configurable and setting it to 0 would disable it :)

>
>Logging a warning seems appropriate. But we should avoid logging a warning
>while the "feature" is disabled.

Yes of course, +1.

>
>I also fear that this will lead to vulnerable applications, because the "default"
>settings are not vulnerable to a given attack and therefore the framework "needs
>no fixing". As I said before: the proposed fix is at the wrong place. I may be
>mistaken, but we should try to find solutions at the core of the problem.

(Sorry I forgot to mention earlier) Yes we have already fixed all known vulnerabilities. This one is just a proactive try for possible unknown expression injection vulnerabilities which are discovered by hacker sooner than us. So this is going to make it harder to actually exploit such unknown vulnerabilities.

Kind Regards.

>
>However, as long as we have an option to disable this, it should work out.
>
>Markus
>
>
>Am 16.09.19 um 14:09 schrieb Yasser Zamani:
>> Thanks Markus and Christoph! Please see inline and see if it satisfies those
>challenges.
>>
>>> -----Original Message-----
>>> From: [hidden email] <[hidden email]>
>>> Sent: Monday, September 16, 2019 11:39 AM
>>> To: [hidden email]
>>> Subject: AW: Max length for OGNL expression
>>>
>>> I agree with this. Basically I like the idea to limit length of ognl
>>> and I think it would increase security. But IMHO it is likely to
>>> cause issues in applications and thus applications must be able to control it.
>> Yes. By "config element" I meant they will be able to override it or set it to a
>very large number to disable it in their struts.xml. Please see also below.
>>
>>> Regards,
>>> Christoph
>>>
>>>
>>>> Seems to me not to be the right place to correct any possible
>>>> problems, and far off any related root of a possible issue.
>>>>
>>>> The config would definitively need an option to be disabled totally.
>>>> I expect very unexpected and hard to trace side effects, depending
>>>> on the application in place.
>> Yes I have already thought that users might have long expressions e.g. <s:if
>test="<long expression>" but I think there exists an N such that no user has any
>expression longer than N. For example I guess N=200. Somebody might claim
>N=500 but at bottom we can discuss and find a default for N.
>>
>> Furthermore we will log.warn() such unlikely expressions before
>> blocking them, so user will be able to find and amend them which makes
>> code more readable and maintainable -- obviously a raw expression
>> longer than 200 should be hard to maintain and read :)
>>
>> Best Regards.
>>
>>>> Markus
>>>>
>>>> Am 15.09.19 um 09:58 schrieb Yasser Zamani:
>>>>> Hi,
>>>>>
>>>>> I thought it might be nice to add a config element which confines
>>>>> the length of OGNL expression that Struts is going to evaluate. It
>>>>> is going to make hackers life harder :)
>>>>>
>>>>> How do you see it?
>>>>>
>>>>> Best.
>>>>>
>>>>>
>>>>> -------------------------------------------------------------------
>>>>> -
>>>>> - To unsubscribe, e-mail: [hidden email] For
>>>>> additional commands, e-mail: [hidden email]
>>>>>
>>>>
>>>> --------------------------------------------------------------------
>>>> - To unsubscribe, e-mail: [hidden email] For
>>>> additional commands, e-mail: [hidden email]
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: [hidden email] For
>> additional commands, e-mail: [hidden email]
>>
>
>
>---------------------------------------------------------------------
>To unsubscribe, e-mail: [hidden email] For additional
>commands, e-mail: [hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Max length for OGNL expression

info@flyingfischer.ch
Dear Yasser

I perfectly understood that the proposed change is proactive and that
there are no open known vulnerabilities. ;-)

Best regards
Markus

Am 16.09.19 um 15:42 schrieb Yasser Zamani:

>> -----Original Message-----
>> From: [hidden email] <[hidden email]>
>> Sent: Monday, September 16, 2019 4:58 PM
>> To: [hidden email]
>> Subject: Re: Max length for OGNL expression
>>
>> Dear Yasser
>>
>> we definitively need an option to totally disable this "feature". It really depends
>> on what kind of application you deploy.
> Yes it's going to be user configurable and setting it to 0 would disable it :)
>
>> Logging a warning seems appropriate. But we should avoid logging a warning
>> while the "feature" is disabled.
> Yes of course, +1.
>
>> I also fear that this will lead to vulnerable applications, because the "default"
>> settings are not vulnerable to a given attack and therefore the framework "needs
>> no fixing". As I said before: the proposed fix is at the wrong place. I may be
>> mistaken, but we should try to find solutions at the core of the problem.
> (Sorry I forgot to mention earlier) Yes we have already fixed all known vulnerabilities. This one is just a proactive try for possible unknown expression injection vulnerabilities which are discovered by hacker sooner than us. So this is going to make it harder to actually exploit such unknown vulnerabilities.
>
> Kind Regards.
>
>> However, as long as we have an option to disable this, it should work out.
>>
>> Markus
>>
>>
>> Am 16.09.19 um 14:09 schrieb Yasser Zamani:
>>> Thanks Markus and Christoph! Please see inline and see if it satisfies those
>> challenges.
>>>> -----Original Message-----
>>>> From: [hidden email] <[hidden email]>
>>>> Sent: Monday, September 16, 2019 11:39 AM
>>>> To: [hidden email]
>>>> Subject: AW: Max length for OGNL expression
>>>>
>>>> I agree with this. Basically I like the idea to limit length of ognl
>>>> and I think it would increase security. But IMHO it is likely to
>>>> cause issues in applications and thus applications must be able to control it.
>>> Yes. By "config element" I meant they will be able to override it or set it to a
>> very large number to disable it in their struts.xml. Please see also below.
>>>> Regards,
>>>> Christoph
>>>>
>>>>
>>>>> Seems to me not to be the right place to correct any possible
>>>>> problems, and far off any related root of a possible issue.
>>>>>
>>>>> The config would definitively need an option to be disabled totally.
>>>>> I expect very unexpected and hard to trace side effects, depending
>>>>> on the application in place.
>>> Yes I have already thought that users might have long expressions e.g. <s:if
>> test="<long expression>" but I think there exists an N such that no user has any
>> expression longer than N. For example I guess N=200. Somebody might claim
>> N=500 but at bottom we can discuss and find a default for N.
>>> Furthermore we will log.warn() such unlikely expressions before
>>> blocking them, so user will be able to find and amend them which makes
>>> code more readable and maintainable -- obviously a raw expression
>>> longer than 200 should be hard to maintain and read :)
>>>
>>> Best Regards.
>>>
>>>>> Markus
>>>>>
>>>>> Am 15.09.19 um 09:58 schrieb Yasser Zamani:
>>>>>> Hi,
>>>>>>
>>>>>> I thought it might be nice to add a config element which confines
>>>>>> the length of OGNL expression that Struts is going to evaluate. It
>>>>>> is going to make hackers life harder :)
>>>>>>
>>>>>> How do you see it?
>>>>>>
>>>>>> Best.
>>>>>>
>>>>>>
>>>>>> -------------------------------------------------------------------
>>>>>> -
>>>>>> - To unsubscribe, e-mail: [hidden email] For
>>>>>> additional commands, e-mail: [hidden email]
>>>>>>
>>>>> --------------------------------------------------------------------
>>>>> - To unsubscribe, e-mail: [hidden email] For
>>>>> additional commands, e-mail: [hidden email]
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: [hidden email] For
>>> additional commands, e-mail: [hidden email]
>>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: [hidden email] For additional
>> commands, e-mail: [hidden email]
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Max length for OGNL expression

Lukasz Lenart
Hi,

I think by default OGNL can have this limit set to Integer.MAX_VALUE
or something like that, and we just must expose that setting to the
Struts users by a constant. BTW. there are few other things that
should be exposed as well e.g.: debugging OGNL expression


Regards
--
Łukasz
+ 48 606 323 122 http://www.lenart.org.pl/

pon., 16 wrz 2019 o 15:58 [hidden email]
<[hidden email]> napisał(a):

>
> Dear Yasser
>
> I perfectly understood that the proposed change is proactive and that
> there are no open known vulnerabilities. ;-)
>
> Best regards
> Markus
>
> Am 16.09.19 um 15:42 schrieb Yasser Zamani:
> >> -----Original Message-----
> >> From: [hidden email] <[hidden email]>
> >> Sent: Monday, September 16, 2019 4:58 PM
> >> To: [hidden email]
> >> Subject: Re: Max length for OGNL expression
> >>
> >> Dear Yasser
> >>
> >> we definitively need an option to totally disable this "feature". It really depends
> >> on what kind of application you deploy.
> > Yes it's going to be user configurable and setting it to 0 would disable it :)
> >
> >> Logging a warning seems appropriate. But we should avoid logging a warning
> >> while the "feature" is disabled.
> > Yes of course, +1.
> >
> >> I also fear that this will lead to vulnerable applications, because the "default"
> >> settings are not vulnerable to a given attack and therefore the framework "needs
> >> no fixing". As I said before: the proposed fix is at the wrong place. I may be
> >> mistaken, but we should try to find solutions at the core of the problem.
> > (Sorry I forgot to mention earlier) Yes we have already fixed all known vulnerabilities. This one is just a proactive try for possible unknown expression injection vulnerabilities which are discovered by hacker sooner than us. So this is going to make it harder to actually exploit such unknown vulnerabilities.
> >
> > Kind Regards.
> >
> >> However, as long as we have an option to disable this, it should work out.
> >>
> >> Markus
> >>
> >>
> >> Am 16.09.19 um 14:09 schrieb Yasser Zamani:
> >>> Thanks Markus and Christoph! Please see inline and see if it satisfies those
> >> challenges.
> >>>> -----Original Message-----
> >>>> From: [hidden email] <[hidden email]>
> >>>> Sent: Monday, September 16, 2019 11:39 AM
> >>>> To: [hidden email]
> >>>> Subject: AW: Max length for OGNL expression
> >>>>
> >>>> I agree with this. Basically I like the idea to limit length of ognl
> >>>> and I think it would increase security. But IMHO it is likely to
> >>>> cause issues in applications and thus applications must be able to control it.
> >>> Yes. By "config element" I meant they will be able to override it or set it to a
> >> very large number to disable it in their struts.xml. Please see also below.
> >>>> Regards,
> >>>> Christoph
> >>>>
> >>>>
> >>>>> Seems to me not to be the right place to correct any possible
> >>>>> problems, and far off any related root of a possible issue.
> >>>>>
> >>>>> The config would definitively need an option to be disabled totally.
> >>>>> I expect very unexpected and hard to trace side effects, depending
> >>>>> on the application in place.
> >>> Yes I have already thought that users might have long expressions e.g. <s:if
> >> test="<long expression>" but I think there exists an N such that no user has any
> >> expression longer than N. For example I guess N=200. Somebody might claim
> >> N=500 but at bottom we can discuss and find a default for N.
> >>> Furthermore we will log.warn() such unlikely expressions before
> >>> blocking them, so user will be able to find and amend them which makes
> >>> code more readable and maintainable -- obviously a raw expression
> >>> longer than 200 should be hard to maintain and read :)
> >>>
> >>> Best Regards.
> >>>
> >>>>> Markus
> >>>>>
> >>>>> Am 15.09.19 um 09:58 schrieb Yasser Zamani:
> >>>>>> Hi,
> >>>>>>
> >>>>>> I thought it might be nice to add a config element which confines
> >>>>>> the length of OGNL expression that Struts is going to evaluate. It
> >>>>>> is going to make hackers life harder :)
> >>>>>>
> >>>>>> How do you see it?
> >>>>>>
> >>>>>> Best.
> >>>>>>
> >>>>>>
> >>>>>> -------------------------------------------------------------------
> >>>>>> -
> >>>>>> - To unsubscribe, e-mail: [hidden email] For
> >>>>>> additional commands, e-mail: [hidden email]
> >>>>>>
> >>>>> --------------------------------------------------------------------
> >>>>> - To unsubscribe, e-mail: [hidden email] For
> >>>>> additional commands, e-mail: [hidden email]
> >>> ---------------------------------------------------------------------
> >>> To unsubscribe, e-mail: [hidden email] For
> >>> additional commands, e-mail: [hidden email]
> >>>
> >>
> >> ---------------------------------------------------------------------
> >> To unsubscribe, e-mail: [hidden email] For additional
> >> commands, e-mail: [hidden email]
> >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: [hidden email]
> > For additional commands, e-mail: [hidden email]
> >
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]