bytedance / appshark

Appshark is a static taint analysis platform to scan vulnerabilities in an Android app.
Apache License 2.0
1.49k stars 165 forks source link

如何区分source的原始对象和传播中被污染的对象 #63

Closed firmianay closed 8 months ago

firmianay commented 9 months ago

https://codeql.github.com/codeql-query-help/java/java-android-intent-uri-permission-manipulation/

下面分别是有漏洞和没漏洞的例子:

    // BAD: the user-provided Intent is returned as-is
    public void dangerous() {
        Intent intent = getIntent();
        intent.putExtra("result", "resultData");
        setResult(intent);
    }

    // GOOD: a new Intent is created and returned
    public void safe() {
        Intent intent = new Intent();
        intent.putExtra("result", "resultData");
        setResult(intent);
    }

那如果是这种情况,intent1虽然是新建的,但是被intent2污染了,如果不能区分这两者,那就会产生误报:

    public void safe() {
        Intent intent1 = new Intent();
        Intent intent2 = getIntent();
        String string2 = intent2.getDataString();
        intent1.putExtra("result", string2);
        setResult(intent1);
    }
nkbai commented 9 months ago

通过 throughapi 辅助 判断,是否可行?

firmianay commented 9 months ago

感觉不行,不管through了什么api,最后都会回到如何区分这两个对象的问题

nkbai commented 9 months ago

感觉不行,不管through了什么api,最后都会回到如何区分这两个对象的问题

比如Intent.getParcelable?,主要是从本质上区分,从Intent中直接提取一个Intent,如果只是从Intent中提取string,integer之类的,这种污点传播,认为是没有危害的。

firmianay commented 9 months ago

比如Intent.getParcelable?,主要是从本质上区分,从Intent中直接提取一个Intent,如果只是从Intent中提取string,integer之类的,这种污点传播,认为是没有危害的。

对,所以就产生了这个误报,因为只有setResult(intent2)才是漏洞,还是说intent1.putExtra("result", string2);并不会导致intent1被污染,因此不会有这条链,引擎里对这种污点传播有特殊处理吗?

nkbai commented 9 months ago

刚刚的throughpapi就是设计用来做这个事情的。 具体是什么api,需要你自己根据你的业务代码进行总结。

nkbai commented 9 months ago

比如Intent.getParcelable?,主要是从本质上区分,从Intent中直接提取一个Intent,如果只是从Intent中提取string,integer之类的,这种污点传播,认为是没有危害的。

对,所以就产生了这个误报,因为只有setResult(intent2)才是漏洞,还是说intent1.putExtra("result", string2);并不会导致intent1被污染,因此不会有这条链,引擎里对这种污点传播有特殊处理吗?

这个本质是如何操控pathfinder,你看看有什么其他的sanitizer方式,可以自定义加进去的。

firmianay commented 9 months ago

限定在getParcelable并不能解决这种类型的误报:

    public void safe() {
        Intent intent1 = new Intent();
        Intent intent2 = getIntent().getExtras().getParcelable(key)
        String string2 = intent2.getDataString();
        intent1.putExtra("result", string2);

        setResult(intent2);  // 漏洞
        setResult(intent1);  // 误报
    }

这种通用漏洞似乎应该假设跟业务代码没关系,当然如果是case by case,熟悉业务逻辑的话,确实可以针对性做一些限制。你们有没有那种仅针对某个App的规则,增加个packageName字段?

nkbai commented 9 months ago

确实不能,针对性的限制,可以举例说说么?只要能在sanitizer中表达的,都是可以的。 工具本来就是在误报和漏报之间寻求平衡。

firmianay commented 8 months ago

我分析了很多样本,发现最常见的代码模式是这样的:

val str = getIntent().getStringExtra("str")
val newIntent = Intent()
newIntent.putExtra("str", str)
setResult(1, newIntent)

所以我尝试去检查put*这种函数,用NotTaint去排除新建的Intent,即污点没有污染@this,但是污染了p*

    sanitizer: {
      newIntentPut: {
        "<android.content.Intent: android.content.Intent put*(*)>": {
          TaintCheck: ["p*"],
          NotTaint: ["@this"],
        },
      },

但是好像不太行,appshark认为@this也是污染的,就感觉NotTaint是指执行完put*这条程序之后的情况?因为执行前@this是干净的,只有p*被污染。

你们有规则用到NotTaint: ["@this"]吗,感觉执行后的情况是没有意义的,因为传播规则里有param->@this,肯定会被污染,这块能改成执行前检查吗?

nkbai commented 8 months ago

appshark进行的是流不敏感 分析,所以可以直接忽略指令间的顺序

nkbai commented 8 months ago

考虑一下https://github.com/bytedance/appshark/blob/main/doc/zh/EngineConfig.md中的VariableFlowRule是否更适合你的需求

firmianay commented 8 months ago

appshark进行的是流不敏感 分析,所以可以直接忽略指令间的顺序

那可能没戏了...改VariableFlowRule的话,删了put*param->@this其他分析估计也进行不下去了