DecodeableRpcInvocation增加入参类型校验

[aquariuspj]

目的

加固 关于CVE-2020-1948 的问题修复。

问题

通过简单的修改CVE-2020-1948的POC示例，我在dubbo-2.7.7中再次复现了该问题。

1. 使用 https://github.com/apache/dubbo-spring-boot-project 最新的2.7.7的TAG分支。
2. 其他poc内容参照：https://blog.csdn.net/caiqiiqi/article/details/106934770，但修改其中POC的部分代码：

    resp = client.send_request_and_return_response(
    service_name='org.apache.dubbo.spring.boot.demo.consumer.DemoService',
    method_name='rce',
    args=[toStringBean])

修改为

    resp = client.send_request_and_return_response(
    service_name='org.apache.dubbo.spring.boot.sample.consumer.DemoService',
    method_name='$invoke',
    service_version='1.0.0',
    args=[toStringBean])

注入的恶意代码ExploitMac.java依然会被执行。

提交内容

1. DecodeableRpcInvocation解码时，对$invoke、$invokeAsync、$echo等特殊方法进行参数描述的校验，防止外部输入不合法的参数类型。

[haiyang1985]

这个修复好像一点都拦不住恶意代码，看下面这个例子，藏在types和objects里的恶意代码，你根本识别不了，也无法判断到底是不是恶意代码。

String[] types = new String[]{"com.xxx.rome"};
Object[] objects = new Object[]{new RomeObject()};
service.$invoke("$invoke", types, objects);

[aquariuspj]

这个修复好像一点都拦不住恶意代码，看下面这个例子，藏在types和objects里的恶意代码，你根本识别不了，也无法判断到底是不是恶意代码。

String[] types = new String[]{"com.xxx.rome"};
Object[] objects = new Object[]{new RomeObject()};
service.$invoke("$invoke", types, objects);

抱歉...github这块操作我还不太熟悉..不小心点了很多奇怪的操作。

1. 假设service.$invoke(String, String[], Object[])方法的声明确实存在，那么由于入参类型正确，因此恶意对象RemoObject的反序列化过程也一定会是正确的，此时也不会触发toString这个注入点。
2. 如果入参类型全都正确，那么就应该交给应用层对入参进行额外校验来保证调用的安全性。。

[chickenlj]

LGTM.

[chickenlj]

@aquariuspj Thanks for the enhancement. Next time when discussing a vulnerability issue, please send an email to security@dubbo.apache.org.

[haiyang1985]

这个修复好像一点都拦不住恶意代码，看下面这个例子，藏在types和objects里的恶意代码，你根本识别不了，也无法判断到底是不是恶意代码。

String[] types = new String[]{"com.xxx.rome"};
Object[] objects = new Object[]{new RomeObject()};
service.$invoke("$invoke", types, objects);

抱歉...github这块操作我还不太熟悉..不小心点了很多奇怪的操作。

1. 假设service.$invoke(String, String[], Object[])方法的声明确实存在，那么由于入参类型正确，因此恶意对象RemoObject的反序列化过程也一定会是正确的，此时也不会触发toString这个注入点。
2. 如果入参类型全都正确，那么就应该交给应用层对入参进行额外校验来保证调用的安全性。。

你的规则应该只能挡住调用$invoke的入参也是错误的情况吧，只要参数是符合String,String[],Object[]，即使包含了恶意代码，也防不住的吧？

1. 如果请求的$invoke的入参是String,String[],Object[]，而且objects包含了RomeObject恶意代码，这个请求的desc还是会符合Ljava/lang/String;[Ljava/lang/String;[Ljava/lang/Object;规则的。
2. 参考下现在的反序列化过程，因为pts的length是3，也就会执行3次in.readObject()，其中就包括in.readObject(RemoObject)，导致恶意代码被反序列化，只要恶意代码被反序列化了，不是就有可能会触发toString的注入点嘛。

args = new Object[pts.length];
for (int i = 0; i < args.length; i++) {
    try {
        args[i] = in.readObject(pts[i]);
    } catch (Exception e) {
        if (log.isWarnEnabled()) {
            log.warn("Decode argument failed: " + e.getMessage(), e);
        }
    }
}

[aquariuspj]

这个修复好像一点都拦不住恶意代码，看下面这个例子，藏在types和objects里的恶意代码，你根本识别不了，也无法判断到底是不是恶意代码。

String[] types = new String[]{"com.xxx.rome"};
Object[] objects = new Object[]{new RomeObject()};
service.$invoke("$invoke", types, objects);

抱歉...github这块操作我还不太熟悉..不小心点了很多奇怪的操作。

1. 假设service.$invoke(String, String[], Object[])方法的声明确实存在，那么由于入参类型正确，因此恶意对象RemoObject的反序列化过程也一定会是正确的，此时也不会触发toString这个注入点。
2. 如果入参类型全都正确，那么就应该交给应用层对入参进行额外校验来保证调用的安全性。。

你的规则应该只能挡住调用$invoke的入参也是错误的情况吧，只要参数是符合String,String[],Object[]，即使包含了恶意代码，也防不住的吧？

1. 如果请求的$invoke的入参是String,String[],Object[]，而且objects包含了RomeObject恶意代码，这个请求的desc还是会符合Ljava/lang/String;[Ljava/lang/String;[Ljava/lang/Object;规则的。
2. 参考下现在的反序列化过程，因为pts的length是3，也就会执行3次in.readObject()，其中就包括in.readObject(RemoObject)，导致恶意代码被反序列化，只要恶意代码被反序列化了，不是就有可能会触发toString的注入点嘛。

args = new Object[pts.length];
for (int i = 0; i < args.length; i++) {
    try {
        args[i] = in.readObject(pts[i]);
    } catch (Exception e) {
        if (log.isWarnEnabled()) {
            log.warn("Decode argument failed: " + e.getMessage(), e);
        }
    }
}

只要参数是符合String,String[],Object[]，即使包含了恶意代码，也防不住的吧？

是的，入参类型正确，反序列化一定会被执行。
不过在方法调用入参完全符合接口声明的情况下，现有的逻辑不会执行到DubboProtocol中抛出异常时的toString注入点。

1. 现有的反序列化过程我觉得其实是没问题的，反序列化只是还原对象本身的信息，它也无法校验对象所携带的信息是否是恶意的。
2. DubboProtocol中抛出异常那部分逻辑如何修改我觉得可能还需要再做一些讨论，不过在那之前先对入参类型进行校验应该是一个比较好的选择。

[haiyang1985]

这个修复好像一点都拦不住恶意代码，看下面这个例子，藏在types和objects里的恶意代码，你根本识别不了，也无法判断到底是不是恶意代码。

String[] types = new String[]{"com.xxx.rome"};
Object[] objects = new Object[]{new RomeObject()};
service.$invoke("$invoke", types, objects);

抱歉...github这块操作我还不太熟悉..不小心点了很多奇怪的操作。

1. 假设service.$invoke(String, String[], Object[])方法的声明确实存在，那么由于入参类型正确，因此恶意对象RemoObject的反序列化过程也一定会是正确的，此时也不会触发toString这个注入点。
2. 如果入参类型全都正确，那么就应该交给应用层对入参进行额外校验来保证调用的安全性。。

你的规则应该只能挡住调用$invoke的入参也是错误的情况吧，只要参数是符合String,String[],Object[]，即使包含了恶意代码，也防不住的吧？

1. 如果请求的$invoke的入参是String,String[],Object[]，而且objects包含了RomeObject恶意代码，这个请求的desc还是会符合Ljava/lang/String;[Ljava/lang/String;[Ljava/lang/Object;规则的。
2. 参考下现在的反序列化过程，因为pts的length是3，也就会执行3次in.readObject()，其中就包括in.readObject(RemoObject)，导致恶意代码被反序列化，只要恶意代码被反序列化了，不是就有可能会触发toString的注入点嘛。

args = new Object[pts.length];
for (int i = 0; i < args.length; i++) {
    try {
        args[i] = in.readObject(pts[i]);
    } catch (Exception e) {
        if (log.isWarnEnabled()) {
            log.warn("Decode argument failed: " + e.getMessage(), e);
        }
    }
}

只要参数是符合String,String[],Object[]，即使包含了恶意代码，也防不住的吧？

是的，入参类型正确，反序列化一定会被执行。
不过在方法调用入参完全符合接口声明的情况下，现有的逻辑不会执行到DubboProtocol中抛出异常时的toString注入点。

1. 现有的反序列化过程我觉得其实是没问题的，反序列化只是还原对象本身的信息，它也无法校验对象所携带的信息是否是恶意的。
2. DubboProtocol中抛出异常那部分逻辑如何修改我觉得可能还需要再做一些讨论，不过在那之前先对入参类型进行校验应该是一个比较好的选择。

没太理解，要么加个钉钉详聊一下？钉钉：haiyang198542

[jiangyunpeng]

不好意思，请问下 2.6.x 版本不修复这个漏洞吗？