久しぶりに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
調査結果は以下の通り。見事にアウトです。
- setExprOverrides: OgnlValueStack#ovrridesに、先ほどセットしたヤバい値がそのまま保持される
- tryFindValue:
- lookupForOverridesが呼ばれ、さっきのヤバい値が取り出される
- 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)が行われています。
