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? > > 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? > > 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) > > 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. > 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 >
