Date: Mon, 28 Apr 2014 11:52:23 +0200
From: Rene Gielen <rgielen@...che.org>
To: fulldisclosure@...lists.org
Subject: Re: [FD] [ANN] Struts 2 up to 2.3.16.1: Zero-Day Exploit Mitigation
 (security | critical)

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 dev@...uts.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: security@...uts.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/

Powered by blists - more mailing lists