読者です 読者をやめる 読者になる 読者になる

この日記は私的なものであり所属会社の見解とは無関係です。 GitHub: takahashikzn

Struts-2.2.3.1リリース(緊急度高!!)

久しぶりにStruts-2.2系がアップデートされていました。
緊急性の高いセキュリティFIXを行ったようです。

何がFIXされた?

POSTした値をアクションフォームにセットするとき、『文字列→値』の変換エラーになる場合は、
入力フォームにエラーの値を再表示してあげるわけですが、その方式に問題がありました。


入力エラー値を保持するときに、副作用として『一切のサニタイズなしで値をOGNLの式として再評価してしまう』という脆弱性が存在します。
問題の箇所は
com.opensymphony.xwork2.interceptor.ConversionErrorInterceptor

com.opensymphony.xwork2.validator.validators.RepopulateConversionErrorFieldValidatorSupport
です。以下、ConversionErrorInterceptorに限定して説明しますが、RepopulateConversionErrorFieldValidatorSupportでも問題の本質は同じ。


さて、例えば


数値型のフィールドの値に

<' + #session + '>

なんて値を入力すると、数値型への変換が失敗するのでアクションフォームへの値セットは行われないわけですが、
その後、副作用的にOGNLの式として再評価されてしまうというのが、今回の脆弱性です。


上記の値を入力すればセッションの値を見放題だし、

{
    userinfo: {
        role: USER
    }
}

なんて値が表示されてしまったりすると、

<' +  (#session.userinfo.role = 'ADMIN') + '>

なんて入力できてしまうわけです。その結果はトンデモナイことに。ひええ!!


テラヤバス


本家のスレッドはコレ。

https://issues.apache.org/jira/browse/WW-3668

Hi guys,
after some tests I have to say that this scenario is worst than I thought.
It also works on Jetty , furthermore it allows to inject any arbitrary value inside the session.
We should strongly consider to sanitize the user input and release a new S2 release ASAP.
I am continuing to investigate on it.

おいオマイら、
ちょっと調べてみたんだけどさ、これって思ったよりヤバイぞ!!!
Jettyでも再現するし、セッションに任意の値をセットできてしまう!
フォームの入力値をサニタイズしないと絶対ダメだ。一刻も早くセキュリティパッチをリリースしないと。
いま、俺は他にもヤバい箇所がないか調査を継続中だ。


かなり緊迫した事態だ、という空気が伝わってきます。


元のソース(注釈付き)


ConversionErrorInterceptorの、問題の箇所にコメントを入れました。

public class ConversionErrorInterceptor extends AbstractInterceptor {

    public static final String ORIGINAL_PROPERTY_OVERRIDE = "original.property.override";

    protected Object getOverrideExpr(ActionInvocation invocation, Object value) {
        // ここが元凶!!
        return "'" + value + "'";
    }

    @Override
    public String intercept(ActionInvocation invocation) throws Exception {

        ActionContext invocationContext = invocation.getInvocationContext();
        Map<String, Object> conversionErrors = invocationContext.getConversionErrors();
        ValueStack stack = invocationContext.getValueStack();

        // 入力エラー値(復元用)
        HashMap<Object, Object> fakie = null;

        for (Map.Entry<String, Object> entry : conversionErrors.entrySet()) {
            String propertyName = entry.getKey();
            Object value = entry.getValue();

            if (shouldAddError(propertyName, value)) {
                String message = XWorkConverter.getConversionErrorMessage(propertyName, stack);

                Object action = invocation.getAction();
                if (action instanceof ValidationAware) {
                    ValidationAware va = (ValidationAware) action;
                    va.addFieldError(propertyName, message);
                }

                if (fakie == null) {
                    fakie = new HashMap<Object, Object>();
                }

                // 入力エラーになった値を取っておく
                fakie.put(propertyName, getOverrideExpr(invocation, value));
            }
        }

        if (fakie != null) {
            // if there were some errors, put the original (fake) values in place right before the result
            stack.getContext().put(ORIGINAL_PROPERTY_OVERRIDE, fakie);
            invocation.addPreResultListener(new PreResultListener() {
                public void beforeResult(ActionInvocation invocation, String resultCode) {
                    Map<Object, Object> fakie = (Map<Object, Object>) invocation.getInvocationContext().get(ORIGINAL_PROPERTY_OVERRIDE);

                    if (fakie != null) {
                        invocation.getStack().setExprOverrides(fakie); // 復元用の値をセーブ→後で式の再評価が走る!!
                    }
                }
            });
        }
        return invocation.invoke();
    }

    protected boolean shouldAddError(String propertyName, Object value) {
        return true;
    }
}

この後、どうなるのか自分なりに追っかけてみました。

invocation.getStack().setExprOverrides(fakie);

OgnlValueStack


調査結果は以下の通り。見事にアウトです。

  1. setExprOverrides: OgnlValueStack#ovrridesに、先ほどセットしたヤバい値がそのまま保持される
  2. tryFindValue:
    1. lookupForOverridesが呼ばれ、さっきのヤバい値が取り出される
    2. findValueでヤバい値がOGNL式として評価される
package com.opensymphony.xwork2.ognl;

...

/**
 * ...
 *
 * @author Patrick Lightbody
 * @author tm_jee
 * @version $Date: 2010-07-09 12:11:54 +0200 (Fri, 09 Jul 2010) $ $Id: OgnlValueStack.java 962472 2010-07-09 10:11:54Z lukaszlenart $
 */
public class OgnlValueStack implements Serializable, ValueStack, ClearableValueStack, MemberAccessValueStack {

    ...

    Map<Object, Object> overrides;

    ...

    /**
     * @see com.opensymphony.xwork2.util.ValueStack#setExprOverrides(java.util.Map)
     */
    public void setExprOverrides(Map<Object, Object> overrides) {
        if (this.overrides == null) {
            this.overrides = overrides;
        } else {
            this.overrides.putAll(overrides);
        }
    }

    /**
     * @see com.opensymphony.xwork2.util.ValueStack#getExprOverrides()
     */
    public Map<Object, Object> getExprOverrides() {
        return this.overrides;
    }

    ...

    private Object tryFindValue(String expr) throws OgnlException {
        Object value;
        expr = lookupForOverrides(expr);
        if (defaultType != null) {
            value = findValue(expr, defaultType);
        } else {
            value = getValueUsingOgnl(expr);
            if (value == null) {
                value = findInContext(expr);
            }
        }
        return value;
    }

    private String lookupForOverrides(String expr) {
        if ((overrides != null) && overrides.containsKey(expr)) {
            expr = (String) overrides.get(expr);
        }
        return expr;
    }

    ...

}

修正内容

パッチはコレ

--- struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/interceptor/ConversionErrorInterceptor.java	2011/08/12 08:29:13	1157008
+++ struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/interceptor/ConversionErrorInterceptor.java	2011/08/12 08:38:39	1157009
@@ -20,6 +20,7 @@
 import com.opensymphony.xwork2.ValidationAware;
 import com.opensymphony.xwork2.conversion.impl.XWorkConverter;
 import com.opensymphony.xwork2.util.ValueStack;
+import org.apache.commons.lang.StringEscapeUtils;
 
 import java.util.HashMap;
 import java.util.Map;
@@ -84,7 +85,11 @@
     public static final String ORIGINAL_PROPERTY_OVERRIDE = "original.property.override";
 
     protected Object getOverrideExpr(ActionInvocation invocation, Object value) {
-        return "'" + value + "'";
+        return escape(value);
+    }
+
+    protected String escape(Object value) {
+        return "\"" + StringEscapeUtils.escapeJava(String.valueOf(value)) + "\"";
     }
 
     @Override

上記ソースのコメント『復元用の値をセーブ→後で式の再評価が走る!!』で
OGNLの危険な式のままセーブしなければ良いので、
このパッチでは『入力値を"Javaの文字列"化する』ように修正(escapeJava)が行われています。