Hi Yasser thanks for reconsidering and your detailed answers. I appreciate your detailed feedback very much. And thanks for specifying that there _is_ an option to disable the restrictions by using:
<constant name="struts.ognl.expressionMaxLength" value="" /> I suspect it will never be possible with such an approach to find a general correct balance between the definition of a magically allowed number of characters and a real security benefit. Therefore this setting should be disabled by default and may optionally be activated. If handled and declared in this manner, the option may prove helpful indeed. And it should be documented, that this option only has proactive character. As you mention below, it is always possible to work around such restrictions and to adapt any given logic, move OGNL expressions into the program logic, shorten variable names and so on. However there are valid use cases where the code remains easier to maintain by using a longer OGNL expression (containing e.g. or statements), to avoid to clutter every action with things that may belong into the view, especially if you work in the view with central imports (e.g. for headers, menus, footer etc.). See also inline below... Markus Am 08.11.19 um 11:18 schrieb Yasser Zamani: > Hi Markus, > > Sorry for inconvenience - yes that was my genius idea ;) ensued from my > vision on our security reports and in the first place, it didn't look > bad to me because I'd seen similar practices in variety of places for > example in http, tomcat, nginx and etc. > > However, I also shared and discussed it in dev list and feedback was OK > and we only had doubt about the default constant value and if enable by > default; "However, as long as we have an option to disable this, it > should work out." you said. Setting it to an empty string will disable > it - sorry I couldn't find time but I'll document these and other new > functionalities ASAP. > > You're right that "Limiting OGNL expressions is hiding a real problem" > (as it's just trying to be a proactive defense) and we shouldn't block > heavy cars only because of drunken people. > > Maybe I'm wrong but the thing is that I didn't expect such long > expression in an application. Could you please count how many expression > fail at your side? (maybe they're few) Could you please share one of > them with us? (Does that one look rational? Is it easily readable and > maintainable? If not, isn't it better to move that logic into your > action and just use <s:if test="myLogic()" which makes it readable and > maintainable?). > > Please see inline... > > On 11/8/2019 9:43 AM, i...@flyingfischer.ch wrote: >> Hello JC >> >> thanks for replying. There are several flaws with the idea to limit the >> length of a OGNL expression string due to secutity reasons: >> >> First: the parsing of the expression will be BLOCKED, as intended, and >> an exception is being thrown: >> >> ognl.OgnlException: Parsing blocked due to security reasons! >> [java.lang.SecurityException: This expression exceeded maximum allowed >> length: >> >> However, it is being logged as WARNING: >> >> WARN com.opensymphony.xwork2.ognl.OgnlUtil - Could not evaluate this >> expression due to security constraints >> >> So first off, if using such a measure, log it as what it is: an ERROR. > By WARN I had tried to warn user to fix such long expression via moving > it to action or etc. I did like Struts doesn't find a setter to set ognl > value. But now I concede you're right, ERROR is better. I'll pull > request to see :) thanks! > >> Second, could you please elaborate what the LENGTH of a OGNL expression >> has to do with security? Do you mean, just because the bad guys tend to >> use lengthy expressions to hack a struts application it is a good idea >> to limit the length of a OGNL expression as such? >> >> If we can see that car accidents tend to happen with heavy cars driven >> by drunken people cause the most damage, should we then restrict the >> damage by limiting the weight of a car? I do not think this is a >> sensible approach. >> >> Limiting OGNL expressions is hiding a real problem by obfuscating the >> real problem by something that is not affiliated with the root cause. >> This may hinder the early detection of any true root cause to take >> appropriate counter measures and is therefore anything else than a >> security measure. > Already discussed above. I need your answers to above questions please, > to rethink and analysis of this decision again. Thanks! > >> in contrary: you may cause true security issues when application start >> to have lacking functionality due to "parsing errors" by limiting OGNL >> expression. > Sorry I couldn't get this. Could you please extend? I may be wrong on this, but loosing information and decisions what should be displayed or not based on a restricted OGNL expression could pose real problems. >> Any limit will anyway always be totally arbitrarily: Assuming a >> limitation of 30 characters and expression like this will pass <s:if >> test="a.b.size() > 0 || c.d.size() > 0"> while <s:if >> test="firstObject.myCollection() > 0 || >> secondObject.myCollection().size() > 0"> will fail. Raising the >> limitation to 200 characters or 1024 or whatever does not change this at >> all. > I think there is always an N where it's not sensible to have an > expression longer than it, don't you? See above: I doubt that there is a sensible general approach, that satisfies both application specific needs and a targeted security gain. This may be well possible on individual level. > > >> So if you are still convinced that implementing such a restriction is a >> good idea, the default setting in Struts/OGNL should be that such a >> limitation is disabled by default and may be enabled at best on >> recommendation. > I agree. As you and Lukasz expressed, lets disable it in 2.5 and enable > it in 2.6 by default, thanks! (to confine future possible similar > inconveniences for users) Perfect. If handled like this, the option may prove useful. > >> Looking at your "fix": >> >> <constant name="struts.ognl.expressionMaxLength" value="1024" /> >> >> This does "fix" the warnings and errors in my case of course. However >> there seems not to be any possibility to DISABLE this limitation >> totally: <constant name="struts.ognl.expressionMaxLength" value="0" /> > Setting it to an empty string will disable it - sorry I couldn't find > time but I'll document these and other new functionalities ASAP. Thanks for this! > >> yields any expression as non functional. >> >> Thanks for reconsidering this weak design decision in Struts! >> > Thank you so much for your time writing and analyzing to us! We really > appreciate it! > > Kind Regards, > Yasser. > >> Markus >> >> PS: we had a discussion on this matter before on this list, and there >> was no consensus to implement it. >> >> >> Am 08.11.19 um 02:07 schrieb J C: >>> Sorry - theree is a typo I missed in copy/paste. That should have been: >>> >>> (if using struts.xml) - >>> <constant name="struts.ognl.expressionMaxLength" value="1024" /> >>> >>> James. On Thursday, November 7, 2019, 8:02:13 p.m. EST, J C >>> <jcyh24...@yahoo.ca.invalid> wrote: >>> >>> (Sorry about the separate thread for reply) >>> >>> Hello Markus. >>> >>> If you have expressions in your application longer than the default limit >>> in 2.5.21 (200), that may be causing the exception (and hopefully also the >>> WARN output). >>> >>> Please try applying a configuration change for your application (replace >>> 1024 with whatever is the largest expression length you need for your >>> application): >>> >>> (if using struts.properties) - >>> struts.ognl.expressionMaxLength=1024 >>> >>> (if using struts.xml) - >>> <constant name="sstruts.ognl.expressionMaxLength" value="1024" /> >>> >>> and see if that resolves the failure ? >>> >>> Please reply to the dev list to let us know if that helps or not. >>> >>> Thanks, >>> >>> James. >>> >>>> It is reported in WARN level: >>>> >>>> WARN com.opensymphony.xwork2.ognl.OgnlValueStack - Could not evaluate >>>> this expression due to security constraints: >>>> >>>> Markus >>>> >>>>> Am 07.11.19 um 23:12 schrieb i...@flyingfischer.ch: >>>>> See new errors like this: >>>>> >>>>> Caused by: java.lang.SecurityException: This expression exceeded maximum >>>>> allowed length:.. >>>>> >>>>> followed by a longer OGNL expression in JSP. >>>>> >>>>> Markus >>>>> >>>>>> Am 07.11.19 um 20:57 schrieb Lukasz Lenart: >>>>>> Hi, >>>>>> >>>>>> Please take a time and test the bits - any help is appreciated. Please >>>>>> report any problems. I'll call for a vote in a few days if no problems >>>>>> will be spotted. >>>>>> >>>>>> Staging Maven repo >>>>>> https://repository.apache.org/content/groups/staging/ >>>>>> Standalone artifacts >>>>>> https://dist.apache.org/repos/dist/dev/struts/2.5.21/ >>>>>> >>>>>> Release notes >>>>>> https://cwiki.apache.org/confluence/display/WW/Version+Notes+2.5.21 >>>>>> >>>>>> >>>>>> Kind regards >>>>>> >>>>>> --------------------------------------------------------------------- >>>>>> To unsubscribe, e-mail: dev-unsubscr...@struts.apache.org >>>>>> For additional commands, e-mail: dev-h...@struts.apache.org >>>>>> >>> --------------------------------------------------------------------- >>> To unsubscribe, e-mail: dev-unsubscr...@struts.apache.org >>> For additional commands, e-mail: dev-h...@struts.apache.org >>> >>> >> >> --------------------------------------------------------------------- >> To unsubscribe, e-mail: dev-unsubscr...@struts.apache.org >> For additional commands, e-mail: dev-h...@struts.apache.org >> > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@struts.apache.org > For additional commands, e-mail: dev-h...@struts.apache.org > --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@struts.apache.org For additional commands, e-mail: dev-h...@struts.apache.org
