hohn/codeql
1
0
Fork 0
mirror of https://github.com/github/codeql.git synced 2026-04-28 10:15:14 +02:00
Code Issues Packages Projects Releases Wiki Activity

Go: fix incorrect-integer-conversion sanitizer

Browse Source 
This was amended as part of https://github.com/github/codeql/pull/12186, but the conversion was inadequate because the new implementation didn't work when a sink (type conversion) led directly to a non-`localTaintStep` step, such as a store step or an interprocedural step. Here I move the sink back one step to the argument of the type
conversion and sanitize the result of the conversion instead, to ensure there is always a unique local successor to a sink.

This should eliminate unexpected extra results that resulted from https://github.com/github/codeql/pull/12186. Independently there are also *lost* results that stem from needing a higher `fieldFlowBranchLimit` that are not addressed in this PR, but raising that limit is a performance risk and so I will address this separately.
This commit is contained in:
Chris Smowton
2023-03-08 09:48:35 +00:00
parent bdad847584
commit a63a4c29e2
3 changed files with 34 additions and 14 deletions
Download Patch File Download Diff File Expand all files Collapse all files

13
go/ql/lib/semmle/go/security/IncorrectIntegerConversionLib.qll
View File

@@ -123,7 +123,13 @@ class ConversionWithoutBoundsCheckConfig extends TaintTracking::Configuration {
)
}
override predicate isSink(DataFlow::Node sink) { this.isSinkWithBitSize(sink, sinkBitSize) }
override predicate isSink(DataFlow::Node sink) {
// We use the argument of the type conversion as the configuration sink so that we
// can sanitize the result of the conversion to prevent flow on to further sinks
// without needing to use `isSanitizerOut`, which doesn't work with flow states
// (and therefore the legacy `TaintTracking::Configuration` class).
this.isSinkWithBitSize(sink.getASuccessor(), sinkBitSize)
}
override predicate isSanitizer(DataFlow::Node node) {
// To catch flows that only happen on 32-bit architectures we
@@ -135,10 +141,9 @@ class ConversionWithoutBoundsCheckConfig extends TaintTracking::Configuration {
g.isBoundFor(bitSize, sinkIsSigned)
)
or
exists(DataFlow::Node sink, int bitSize |
exists(int bitSize |
isIncorrectIntegerConversion(sourceBitSize, bitSize) and
this.isSinkWithBitSize(sink, bitSize) and
TaintTracking::localTaintStep(sink, node)
this.isSinkWithBitSize(node, bitSize)
)
}
}

11
go/ql/src/Security/CWE-681/IncorrectIntegerConversionQuery.ql
View File

@@ -19,10 +19,13 @@ import semmle.go.security.IncorrectIntegerConversionLib
from
DataFlow::PathNode source, DataFlow::PathNode sink, ConversionWithoutBoundsCheckConfig cfg,
DataFlow::CallNode call
where cfg.hasFlowPath(source, sink) and call.getResult(0) = source.getNode()
select sink.getNode(), source, sink,
DataFlow::CallNode call, DataFlow::Node sinkConverted
where
cfg.hasFlowPath(source, sink) and
call.getResult(0) = source.getNode() and
sinkConverted = sink.getNode().getASuccessor()
select sinkConverted, source, sink,
"Incorrect conversion of " +
describeBitSize(cfg.getSourceBitSize(), getIntTypeBitSize(source.getNode().getFile())) +
" from $@ to a lower bit size type " + sink.getNode().getType().getUnderlyingType().getName() +
" from $@ to a lower bit size type " + sinkConverted.getType().getUnderlyingType().getName() +
" without an upper bound check.", source, call.getTarget().getQualifiedName()

24
go/ql/test/query-tests/Security/CWE-681/IncorrectIntegerConversion.ql
View File

@@ -1,11 +1,23 @@
import go
import TestUtilities.InlineFlowTest
import TestUtilities.InlineExpectationsTest
import semmle.go.security.IncorrectIntegerConversionLib
class IncorrectIntegerConversionTest extends InlineFlowTest {
override DataFlow::Configuration getValueFlowConfig() {
result = any(ConversionWithoutBoundsCheckConfig config)
}
class TestIncorrectIntegerConversion extends InlineExpectationsTest {
TestIncorrectIntegerConversion() { this = "TestIncorrectIntegerConversion" }
override DataFlow::Configuration getTaintFlowConfig() { none() }
override string getARelevantTag() { result = "hasValueFlow" }
override predicate hasActualResult(Location location, string element, string tag, string value) {
tag = "hasValueFlow" and
exists(DataFlow::Node sink, DataFlow::Node sinkConverted |
any(ConversionWithoutBoundsCheckConfig config).hasFlowTo(sink) and
sinkConverted = sink.getASuccessor()
|
sinkConverted
.hasLocationInfo(location.getFile().getAbsolutePath(), location.getStartLine(),
location.getStartColumn(), location.getEndLine(), location.getEndColumn()) and
element = sinkConverted.toString() and
value = "\"" + sinkConverted.toString() + "\""
)
}
}