Secure Code: By Design? Serendipity? Or...?

Posted by Jon, Comments

While researching Struts 2 again to expand our current framework support, I became a bit more familiar with its tag library. This lead to a better understanding of Struts 2 basic OGNL syntax and the creation of some test cases. During the test case development, I was amused at all the different ways one can obtain the value from a getter function. Here's a snippet for your amusement:

"%{tainted}": <s:property value="%{tainted}" escape="false" />
"getTainted()": <s:property value="getTainted()" escape="false" />
"%{getTainted()}": <s:property value="%{getTainted()}" escape="false" />
"#this.tainted": <s:property value="#this.tainted" escape="false" />
"%{#this.tainted}": <s:property value="%{#this.tainted}" escape="false" />
"top.tainted": <s:property value="top.tainted" escape="false" />
...

There are more ways, for sure, but getter synonyms isn't the purpose of this blog. Secure code by chance is.

Secure code isn't just having code that's devoid of vulnerabilities for whatever reasons. Just as insecure code isn't code that happens to have a couple of weaknesses. I've looked at code that had a couple of issues and it seemed relatively secure to me. Just as I've looked at code without any obvious vulnerabilities that I wouldn't consider secure. There are properties to code, hopefully not too subjective, that convey security. To me, one of these security properties is the ability to understand why something is secure or not. If you cannot understand if something is or isn't secure, it isn't secure; it's just serendipity.

Background

If you look at the last line in the JSP snippet above, notice the use of top as a value. This special value obtains the top value from the current ValueStack. In Struts 2 this often is a controller, which is sometimes a subclass of ActionSupport. Why's this important?

Struts 2 has been making the rounds again because of a couple of security advisories: S2-021 and S2-022. Good ol' ParametersInterceptor had a bad couple of days it seems, leading to remote code execution issues. Curiosity got the best of me and I started looking into the updated ParametersInterceptorTest test case and the updated / added ExcludedPatterns class. Looking at that list, you'll see that top isn't in it. Hmm...

Weakness

So top can be a parameter name. Does the framework actually do anything special? ParametersInterceptor eventually calls CompoundRootAccessor.getProperty when top is the parameter name. This returns the root or top of the ValueStack, which again is usually the Action controller that's being accessed. Nice! So we have access to the controller via a parameter name. Let's call some methods!

Since we're calling methods with a value, in OGNL, we're setting a property. This is handled by OgnlRuntime.setProperty:

public static void setProperty(OgnlContext context, Object target, Object name, Object value)
        throws OgnlException
{
    PropertyAccessor accessor;

    if (target == null) {
        throw new OgnlException("target is null for setProperty(null, \"" + name + "\", " + value + ")");
    }
    if ((accessor = getPropertyAccessor(getTargetClass(target))) == null) {
        throw new OgnlException("No property accessor for " + getTargetClass(target).getName());
    }

    accessor.setProperty(context, target, name, value);
}

When using top the target is the specific class, which is a custom class in this case. Since there's no specific PropertyAccessor for this class, this results in an accessor field set to an instance of ObjectAccessor. This calls its super, ObjectPropertyAccessor.setProperty, which calls ObjectPropertyAccessor.setPossibleProperty:

public Object setPossibleProperty(Map context, Object target, String name, Object value)
        throws OgnlException
{
// snip
    if (!OgnlRuntime.setMethodValue(ognlContext, target, name, value, true))
    {
        result = OgnlRuntime.setFieldValue(ognlContext, target, name, value) ? null : OgnlRuntime.NotFound;
    }

    if (result == OgnlRuntime.NotFound)
    {
        Method m = OgnlRuntime.getWriteMethod(target.getClass(), name);
        if (m != null)
        {
            result = m.invoke(target, new Object[] { value});
        }
    }
// snip

In this method, three different ways are used to set or write a value to a property:

  • OgnlRuntime.setMethodValue
  • OgnlRuntime.setFieldValue
  • Invoking the method with the user-provided value.

If all of these fail, the caller method ObjectPropertyAccessor.setProperty throws an exception. And when devMode is on, something like the below is logged:

Unexpected Exception caught setting 'top.foo' on 'class com.coverity.testsuite.property.PropertyAction: Error setting expression 'top.foo' with value '[Ljava.lang.String;@1ef1a094'

So if an exception like the above occurs, we know we hit a Whammy. Otherwise, if we don't hit a Whammy we might be in luck. :) So let's request ?top.text=%25{1*2} and get some RCE! No Whammy, no Whammy, no Whammy, and.... stop!

RCE Attempt 1

Whammy :(

Unexpected Exception caught setting 'top.text' on 'class com.coverity.testsuite.property.PropertyAction: Error setting expression 'top.text' with value '[Ljava.lang.String;@16321271'

Well, what happened? In this case, the call to OgnlRuntime.getWriteMethod returns null in ObjectPropertyAccessor.setPossibleProperty. Hmm...

public static Method getWriteMethod(Class target, String name, int numParms)
{
// snip 
    if ((methods[i].getName().equalsIgnoreCase(name)
         || methods[i].getName().toLowerCase().equals(name.toLowerCase())
         || methods[i].getName().toLowerCase().equals("set" + name.toLowerCase()))
        && !methods[i].getName().startsWith("get")) {
// snip

D'oh! Notice the last part of that conditional. There's a check disallowing one to set a property on a method that starts with get. Boo!

OK, so any methods outside of get* can be called. Guess what, there's yet another OGNL sink on ActionSupport that doesn't start with 'get': hasKey(String)! Let's request ?top.hasKey=%25{1*2}... No Whammy, no Whammy, no Whammy, and... stop!

RCE Attempt 2

Whammy again, drat.

Unexpected Exception caught setting 'top.hasKey' on 'class com.coverity.testsuite.property.PropertyAction: Error setting expression 'top.hasKey' with value '[Ljava.lang.String;@36c63134'

Debugging shows that OgnlRuntime.getWriteMethod when called from ObjectPropertyAccessor.setPossibleProperty did return something this time, awesome! So the method was invoked by reflection via m.invoke(target, new Object[] { value}); with tainted data, nice! Except, it wasn't....

Breaking on ObjectPropertyAccessor.setPossibleProperty and stepping through shows an IllegalArgumentException exception thrown with the message argument type mismatch. Hmm. Looking at the log output, you can see [Ljava.lang.String;, which is the mangled name for String[]. So it seems Struts 2 stores request parameters in a String array. That make sense since one could specify the same parameter name but have different values. And the parameter signature for hasKey is expecting a String, not a String[]. Mismatched argument type. :( Well, shucks. What's one to do w/ a vector that doesn't do anything?!

Recap

What did we learn?

  • The top value bypasses the regular expression exclusion and passes the acceptable names in ParameterInterceptor.
  • top should return a reference to the instance of the Action associated to the URL.
  • It's common, although not required, that Actions exposed are usually subclasses of ActionSupport
  • Methods that start with get* cannot be used as a parameter key. (ActionSupport.getText failed here.)
  • Methods that don't conform to the JavaBean getter / setter convertion need to accept a String[] parameter. (ActionSupport.hasKey failed here.)

It's just dumb luck that RCE didn't happen. For example, if the value was massaged from a String[] to a String, as what happens when the normal getter / setters are called (see XWorkBasicConverter.convertValue), then this could have been RCE. Is it obvious to anyone supporting this code that a custom public method on a class called addValues(String[] values) is accessible via ?top.addValues=value1&top.addValues=value2&... ?

Final Thoughts

I tried to think what the Struts 2 developers could do but I'm lost. I'd rather they remove the exclusion list and remove the functionality that's causing the code to be evaluated in such a way. Exclusion or black lists are like door braces trying to keep out the invading hordes; eventually the hordes break through and raid the castle. Maybe they could ensure ObjectAccessor isn't called on a parameter name, which is tainted. However, I'm guessing there are a lot of things I dunno about Struts 2 that makes this a horrible (and possibly insecure) design choice. Maybe in this case a black list is as good as it gets? If so, is the code still secure? Or is it just lucky?

Appendix

'top.hasKey' vs. 'hasKey'

If you're wondering why specifying top.hasKey is different than hasKey, debug the call to OgnlRuntime.setProperty. Here's the snippet from above again:

public static void setProperty(OgnlContext context, Object target, Object name, Object value)
        throws OgnlException
{
    PropertyAccessor accessor;

    if (target == null) {
        throw new OgnlException("target is null for setProperty(null, \"" + name + "\", " + value + ")");
    }
    if ((accessor = getPropertyAccessor(getTargetClass(target))) == null) {
        throw new OgnlException("No property accessor for " + getTargetClass(target).getName());
    }

    accessor.setProperty(context, target, name, value);
}

When called, if the target is of type CompoundRoot, which it will be for hasKey, accessor is set to an instance of CompoundRootAccessor. When calling top.hasKey the target is the specific class, which is a custom class in this case. This results in an accessor field set to an instance of ObjectAccessor. These two types perform different checks when calling their setProperty methods, with the top case having some potential holes.

Shenanigans with Numbers

Try out the following :)

  • ?0xdeadbeef['equals']=something
  • ?066['equals']=something
  • ?1L['equals']=something

You shouldn't notice any exceptions firing. The OGNL parser is parsing those as numbers and successfully calling the equals method on the respective boxed number class (e.g. Integer.equals).

Now try these:

  • ?12_['ignored']=whatever // Throws ognl.ParseException in ognl.OgnlParser.topLevelExpression()
  • ?123[066]=whatever // 066 converted to decimal 54, and Integer.54() is called, which results in a "54" property not found exception.

Comments!