Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Can someone explain what isAdditionalTaintStep means? #6729

Closed
ox1234 opened this issue Sep 22, 2021 · 11 comments
Closed

Can someone explain what isAdditionalTaintStep means? #6729

ox1234 opened this issue Sep 22, 2021 · 11 comments
Assignees
Labels
acknowledged GitHub staff acknowledges this issue awaiting-response The CodeQL team is awaiting further input or clarification from the original reporter of this issue. question Further information is requested Stale

Comments

@ox1234
Copy link

ox1234 commented Sep 22, 2021

I don't understand what isAdditionalTaintStep means
Can someone explain what that means and when it can be used, hope you can give me an example for that

@ox1234 ox1234 added the question Further information is requested label Sep 22, 2021
@tonghuaroot
Copy link

@aeisenberg aeisenberg added the acknowledged GitHub staff acknowledges this issue label Sep 22, 2021
@aeisenberg
Copy link
Contributor

aeisenberg commented Sep 22, 2021

Hi there, thanks for your question.

isAdditionalTaintStep is the taint analysis analog for isAdditionalFlowStep. They have the same purpose, just for different kinds of analyses.

When doing flow and taint analysis, CodeQL only analyzes paths that go through user code. Calls into third party methods are considered black boxes, unless you add some additional modeling steps. That's where isAdditional{Flow|Taint}Step comes in. You can model paths through library code by implementing these predicates and describing how data goes in and out.

Complete docs for Java are here.

There are many examples of its usage in this repository. For example:

https://github.com/github/codeql/blob/main/java/ql/src/experimental/Security/CWE/CWE-652/XQueryInjection.ql#L35-L37

Here, the additional taint step is saying that the output of a call to XQConnection.prepareExpression is tainted by its first argument.

Here is a detailed article that walks through taint analysis using CodeQL: https://msrc-blog.microsoft.com/2019/03/19/vulnerability-hunting-with-semmle-ql-part-2/

@aeisenberg aeisenberg self-assigned this Sep 22, 2021
@aeisenberg aeisenberg added the awaiting-response The CodeQL team is awaiting further input or clarification from the original reporter of this issue. label Sep 22, 2021
@xsser
Copy link

xsser commented Sep 30, 2021

将数据流断掉的地方连接起来的东西

@github-actions
Copy link
Contributor

This issue is stale because it has been open 14 days with no activity. Comment or remove the Stale label in order to avoid having this issue closed in 7 days.

@github-actions github-actions bot added the Stale label Oct 15, 2021
@github-actions
Copy link
Contributor

This issue was closed because it has been inactive for 7 days.

@p1n93r
Copy link

p1n93r commented Feb 10, 2022

将数据流断掉的地方连接起来的东西

连起来好实现,主要是不知道哪些点会断。。。就会有漏报

@Aur0ra-m
Copy link

连起来好实现,主要是不知道哪些点会断。。。就会有漏报
我也疑惑在这里了,师傅现在有思路了嘛

@p1n93r
Copy link

p1n93r commented Apr 11, 2022

连起来好实现,主要是不知道哪些点会断。。。就会有漏报
我也疑惑在这里了,师傅现在有思路了嘛

针对这个点,我有一个思路,但是我自己还没完全实现(你也可以思考下我这个思路是否可行);

我的思路:写一个比较通用的 isAdditionalTaintStep 方法,比如存在如下污点断开的场景:

Option<String> cmd = new Option<>();
cmd.setValue(test1);
CmdUtil.runCmd(cmd.getValue());

假设Option类是第三方jar内的类,这里CodeQL就会出现污点数据流断开,这种场景可以总结成一个模型:污点进入实例,又从实例流出;那么就可以构造如下通用的写法:

// 将断开的数据流接上
override predicate isAdditionalTaintStep(DataFlow::Node n1, DataFlow::Node n2) {
    exists( Call callOne,Call callTwo |
        callOne.getAnArgument() = n1.asExpr()
        and callTwo = n2.asExpr()
        // callOne和callTwo之间的关系: 来自同一个对象
        and callOne.getQualifier().(VarAccess).getVariable() = callTwo.getQualifier().(VarAccess).getVariable()
    )
}

当然,可以继续完善这个写法,比如一般污点流入是通过setter方法,污点流出是通过getter方法,那么可以再这个基础上再加上方法名限定等等;

上面提到的只是一种数据流断开的模型,可能还有很多其他的模型,持续完善即可,最终写成一个通用的isAdditionalTaintStep方法;

@Aur0ra-m
Copy link

连起来好实现,主要是不知道哪些点会断。。。就会有漏报
我也疑惑在这里了,师傅现在有思路了嘛

针对这个点,我有一个思路,但是我自己还没完全实现(你也可以思考下我这个思路是否可行);

我的思路:写一个比较通用的 isAdditionalTaintStep 方法,比如存在如下污点断开的场景:

Option<String> cmd = new Option<>();
cmd.setValue(test1);
CmdUtil.runCmd(cmd.getValue());

假设Option类是第三方jar内的类,这里CodeQL就会出现污点数据流断开,这种场景可以总结成一个模型:污点进入实例,又从实例流出;那么就可以构造如下通用的写法:

// 将断开的数据流接上
override predicate isAdditionalTaintStep(DataFlow::Node n1, DataFlow::Node n2) {
    exists( Call callOne,Call callTwo |
        callOne.getAnArgument() = n1.asExpr()
        and callTwo = n2.asExpr()
        // callOne和callTwo之间的关系: 来自同一个对象
        and callOne.getQualifier().(VarAccess).getVariable() = callTwo.getQualifier().(VarAccess).getVariable()
    )
}

当然,可以继续完善这个写法,比如一般污点流入是通过setter方法,污点流出是通过getter方法,那么可以再这个基础上再加上方法名限定等等;

上面提到的只是一种数据流断开的模型,可能还有很多其他的模型,持续完善即可,最终写成一个通用的isAdditionalTaintStep方法;

那除了这类进入jar包导致数据流断开,还有其他场景会使得数据流断开嘛

@p1n93r
Copy link

p1n93r commented Apr 11, 2022

连起来好实现,主要是不知道哪些点会断。。。就会有漏报
我也疑惑在这里了,师傅现在有思路了嘛

针对这个点,我有一个思路,但是我自己还没完全实现(你也可以思考下我这个思路是否可行);
我的思路:写一个比较通用的 isAdditionalTaintStep 方法,比如存在如下污点断开的场景:

Option<String> cmd = new Option<>();
cmd.setValue(test1);
CmdUtil.runCmd(cmd.getValue());

假设Option类是第三方jar内的类,这里CodeQL就会出现污点数据流断开,这种场景可以总结成一个模型:污点进入实例,又从实例流出;那么就可以构造如下通用的写法:

// 将断开的数据流接上
override predicate isAdditionalTaintStep(DataFlow::Node n1, DataFlow::Node n2) {
    exists( Call callOne,Call callTwo |
        callOne.getAnArgument() = n1.asExpr()
        and callTwo = n2.asExpr()
        // callOne和callTwo之间的关系: 来自同一个对象
        and callOne.getQualifier().(VarAccess).getVariable() = callTwo.getQualifier().(VarAccess).getVariable()
    )
}

当然,可以继续完善这个写法,比如一般污点流入是通过setter方法,污点流出是通过getter方法,那么可以再这个基础上再加上方法名限定等等;
上面提到的只是一种数据流断开的模型,可能还有很多其他的模型,持续完善即可,最终写成一个通用的isAdditionalTaintStep方法;

那除了这类进入jar包导致数据流断开,还有其他场景会使得数据流断开嘛

应该没有了(可能是我还没遇到);这个问题我也思考过,我想到了另外一种场景:不同maven模块之间的调用;但是测试发现并不会导致数据流断开(按道理不同maven模块会被编译成各个独立的jar,所以针对这种场景测试了一下);所以目前已知就发现调用第三方jar会有这个问题;

@Aur0ra-m
Copy link

好的

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
acknowledged GitHub staff acknowledges this issue awaiting-response The CodeQL team is awaiting further input or clarification from the original reporter of this issue. question Further information is requested Stale
Projects
None yet
Development

No branches or pull requests

6 participants