A follow up to my last message: of course getClass() *is public*, which makes things even worse. As such, class is an 100% property according to Java Beans Specification. I would have failed on the job interview :)
- René Am 26.04.14 12:59, schrieb Rene Gielen: > Hi Tim, > > Am 25.04.14 22:06, schrieb Tim: >> >> Hi Rene, >> >> Thanks for your responses. Keep in mind my criticisms are not >> directed soley at you. They are directed at the entire Struts team, >> it's practices and culture. > > I don't know what insights you have to our practices in general and our > culture. I invite you to name concrete points and make suggestions for > improvements on the d...@struts.apache.org mailing list. > >> >> I've been on the front lines with applications who were pwned by >> Struts bugs and thousands of users' personal information exposed. >> Millions of $$ burnt on the cleanup efforts. So there may be a few >> emotions attached to my emails. Just the way it is. >> >> >>>> A) I questioned the last bug fix in the thread here [1], where we >>>> were all reassured that it was just "ClassLoader manipulation", not >>>> RCE. Clearly that's not true. >>>> >>> >>> At this point in time, it was true. The RCE is not exactly a Struts >>> issue alone, the Struts issue just opens the door to an unprotected >>> field in a certain servlet container environment. >> >> You expect every servlet container environment to protect against >> Struts? I find this response to be very pass-the-buckish. >> > > You seem to have misread my paragraph. I'm not sure how this > interpretation regarding such expectation can be derived from what I wrote. > > Anyway, let's try to rephrase carefully. > > The core part of each EL I'm aware of in JVM space is property based > object graph navigation and property mutation based on the Java Beans > Specification. The said specification defines what a property is and how > to grant public read and/or write access to it. Developers are > recommended to decide carefully when public read or write access should > be granted. > > Here is a nice question for a Java job interview: Is it possible to > write a Java class having no properties according to the Java Beans > Specification? The answer is yes, of course. Now, drop the public > modifier requirement for a moment. How about properties now? Well, since > each class extends java.lang.Object, it exposes at least the readable > property "class", since the inherited method getClass() follows the > getter specification (minus public modifier). Here is where Struts > developers "failed" - we missed out on that fact. Lessons learned: each > framework offering EL based graph navigation should think of special > treatment for the class property, especially given the fact that most > EL's mode of work does not care too much about visibility limitations. > > So here is my point: the specific container environment we have a > confirmed RCE attack for has a writable property leading to heavy > effects when being re-written. With today's knowledge, I personally > doubt that it was the best possible decision to make this an unguarded > writable property. And no, I don't think this should be revisited for > the purpose of protecting Struts users - it should be revisited in > general, for this particular product. > >> My point here is that your team failed to adequately characterize the >> problem's risks. This leads to more developers ignoring the patch and >> subsequently getting pwned. Even if you only thought an RCE *might* >> be possible you should state that. It is the responsible thing to do. >> > > An SQL injection vulnerability might introduce some probability for > leading to RCE. If I understand it right, I should name it RCE rather > than SQL injection then - as RCE *might* be possible? This sounds like > publishing gut feeling based security information. Our initial impact > statement was "classloader manipulation" which precisely names the > problem. By the time we released security bulletin S2-020, we had no > knowledge about how this could lead to RCE. > > Strange enough, there are some smart and creative people out there. Even > stranger, some of them find how to exploit the said vulnerability to > allow for RCE in a certain environment, a few weeks after the > announcement. Luckily, the white- to grey-hatted of these folks tend to > reach a security response team then. This is what happened last week. > From then on we knew we had to deal with an RCE. > > Unfortunately, the RCE impact coincidented with reports about an > insufficient fix. Even worse, the insuffient fix report was disclosed by > accident. > > So what would have happened if we had gotten a report about an RCE > exploit which would have been covered already by a *sufficient* fix > introduced with 2.3.16.1? We would have made an announcement to increase > maximum security rating of CVE-2014-0094 due to RCE impact along with a > strong recommendation to upgrade immediately. > > We name it an RCE when there *is* an RCE, and believe me, we are neither > shy nor ashamed to do so. To call something an RCE when there is just > some probability for it is IMO *not* a responsible thing to do. > >> >> >>>> B) The fix for the last CVE was that crappy "^class\." filter, which >>>> I pointed out was insufficient. The Struts team quickly fixed >>>> that, but never bothered to update the "workaround" section in the >>>> last advisory to the less-terrible ".*\.class\..*" regex (or whatever >>>> it was). So if developers just implemented the work around from >>>> the advisory, they were obviously not protected. (In hindsight, >>>> they never were protected even with the better regex, but was just >>>> irresponsible not to make the second regex more public.) >>>> >>> >>> Better suggestions are always welcome. We have a mail address to reach >>> us for any concerns regarding security: secur...@struts.apache.org >> >> Well, what I'm saying is that I did give suggestions. I believe that >> email address was CCed. Your team took them to heart, which is great. >> But then your team failed to publish them adequately. >> > > I understand your point better now. Indeed, it would have been advisable > to update the mitigation pattern in the security bulletin along with an > announcement. We seem to have missed that. It would have been great if > someone had nagged us to update the page once this finding was made. > Chances are we'll do better on this in future. > > Anyway, I'd love to avoid mitigation advices entirely. They more than > often lead to a feeling of false security and might keep users from > upgrading. For practical reasons we have to give them sometimes, though. > >> >> >>>> C) The Struts team is playing whack-a-mole. Instead of fixing the >>>> root issue, they are just adding one blacklist regex after another, >>>> hoping no one figures out yet another way around it. >>> >>> We try to protect users who are not using a properly configured security >>> manager, as it is always recommended when working with application >>> servers. >> >> Wow. Now you sound like an Oracle representative. I'm not >> exaggerating either, because they've used that line a few times with >> me. We all know how well Oracle handles security. >> >> Using a security manager for web applications is certainly a good >> idea. No arguing against that. Do developers do it? NO. It simply >> isn't common and usually when I suggest it to a client, they give me a >> blank stare, clueless as to what I'm talking about. >> > > Please re-read my paragraph. We are trying to protect our users where we > can! Even if they don't use a security manager, since we *know* that > most users aren't aware of it. > > This leads to a funny situation: since we *do* provide protection code > not relying on JVM security manager, we get slapped in our face once > this mechanisms fail. If we'd throw out these mechanisms for the sake of > telling our users "use security manager, stupid!", we would be in the > comfortable situation of never getting slapped again, nor investing > unpaid time and even own money to fix other folks' security problems any > more. > > I still believe that we are doing it right when investing time and code > in protecting our users even without using JVM security. Nevertheless, I > do think we do have a craftsmanship problem both in dev and ops by > ignoring JVM security. "Senior" Java devs, ITIL certified ops and > Web(Sphere|Logic)-buying clients giving me a blank stare? Yes, this is > part of the problem. > > If you like, take some time to investigate possible impacts of creating > UEL 3.0 (JSR-341) incorporating web applications (using JSF, or simply > JSP). You might want to re-think running them without JVM security > manager enabled... > >> >> >>> Sometimes we seem to fail, indeed. As others, we don't seem to >>> be perfect. BTW, we are not only blacklisting - the blacklist is applied >>> for special cases that make it through the whitelist. >> >> Sure, and if this were only one iteration of failing on the same >> problem, I wouldn't have said a word. It's just that the failure is >> happening over and over again. I felt public criticism, if not >> outright shaming, is warranted. >> > > Do you have better technical solutions and project governance patterns > on the open source projects you are contributing to that you can share > with us? Can you concretely contribute better technical solutions to > Struts related issues you have identified - I mean other than just > "throw out OGNL"? Are you or your clients willing to sponsor some full > time devs or dev days for the product they are using for free? If the > answer is yes to one of these questions, we are happily accepting your > help! If the answer is no, I suggest being more careful with terms like > "shame" and such > >> >>>> I urge you to take OGNL and *throw it out*. Replace it with something >>>> that allows only a white list of properties to be set, based on what >>>> the application defines as relevant. Until then, I'm recommending to >>>> my clients that they avoid Struts like the plague. >>>> >>> >>> To what alternative? UEL? The attack vector is just using a simple >>> getter semantic which basically works with any EL. Throwing out EL >>> capabilities is no option for most users. Anyway, if somebody likes to >>> help with more than just fingerpointing, he/she is heartly welcome! >> >> Something that doesn't allow you to directly call methods, and only >> allows you to access properties on objects explicitly defined by the >> app developer. Keep the syntax similar if you like, but chuck the >> reflection. Data is data. Code is code. Keep them separate. >> > > This is a nice wish list. While we at Struts are in the process of > constantly limiting what OGNL and our interface to it allows (static > method access and what not), other EL providers and consumers seem to go > the opposite way. Users nowadays do like EL expressiveness as much as > they are hating security managers, it seems. However, I do have some > more takeaways on how to improve that I'd rather not like share here. > The Struts development list would be a proper place to discuss this. > > But again, keep in mind that the discussed attack vector *solely* relies > on property access. Not much more, not much less. No weird OGNL features > involved, no static method access, no reflection tricks, no double > evaluation. Just getters and setters. > > Regards, > René > >> tim >> > -- René Gielen http://twitter.com/rgielen _______________________________________________ Sent through the Full Disclosure mailing list http://nmap.org/mailman/listinfo/fulldisclosure Web Archives & RSS: http://seclists.org/fulldisclosure/
