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

C++: Refine the strcat and strcpy models, have BufferWrite depend on them so that information isn't duplicated.

Browse Source
This commit is contained in:
Geoffrey White
2020-06-23 14:00:33 +01:00
parent 38067b5b34
commit c681e6999d
3 changed files with 113 additions and 112 deletions
Download Patch File Download Diff File Expand all files Collapse all files

15
cpp/ql/src/semmle/code/cpp/models/implementations/Strcat.qll
View File

@@ -24,6 +24,21 @@ class StrcatFunction extends TaintFunction, DataFlowFunction, ArrayFunction, Sid
)
}
/**
* Gets the index of the parameter that is the size of the copy (in characters).
*/
int getParamSize() { if exists(getParameter(2)) then result = 2 else none() }
/**
* Gets the index of the parameter that is the source of the copy.
*/
int getParamSrc() { result = 1 }
/**
* Gets the index of the parameter that is the destination to be appended to.
*/
int getParamDest() { result = 0 }
override predicate hasDataFlow(FunctionInput input, FunctionOutput output) {
input.isParameter(0) and
output.isReturnValue()

132
cpp/ql/src/semmle/code/cpp/models/implementations/Strcpy.qll
View File

@@ -8,70 +8,107 @@ import semmle.code.cpp.models.interfaces.SideEffect
*/
class StrcpyFunction extends ArrayFunction, DataFlowFunction, TaintFunction, SideEffectFunction {
StrcpyFunction() {
this.hasName("strcpy") or
this.hasName("_mbscpy") or
this.hasName("wcscpy") or
this.hasName("strncpy") or
this.hasName("_strncpy_l") or
this.hasName("_mbsncpy") or
this.hasName("_mbsncpy_l") or
this.hasName("wcsncpy") or
this.hasName("_wcsncpy_l")
exists(string name | name = getName() |
// strcpy(dst, src)
name = "strcpy"
or
// wcscpy(dst, src)
name = "wcscpy"
or
// _mbscpy(dst, src)
name = "_mbscpy"
or
(
name = "strcpy_s" or // strcpy_s(dst, max_amount, src)
name = "wcscpy_s" or // wcscpy_s(dst, max_amount, src)
name = "_mbscpy_s" // _mbscpy_s(dst, max_amount, src)
) and
// exclude the 2-parameter template versions
// that find the size of a fixed size destination buffer.
getNumberOfParameters() = 3
or
// strncpy(dst, src, max_amount)
name = "strncpy"
or
// _strncpy_l(dst, src, max_amount, locale)
name = "_strncpy_l"
or
// wcsncpy(dst, src, max_amount)
name = "wcsncpy"
or
// _wcsncpy_l(dst, src, max_amount, locale)
name = "_wcsncpy_l"
or
// _mbsncpy(dst, src, max_amount)
name = "_mbsncpy"
or
// _mbsncpy_l(dst, src, max_amount, locale)
name = "_mbsncpy_l"
)
}
override predicate hasArrayInput(int bufParam) { bufParam = 1 }
/**
* Holds if this is one of the `strcpy_s` variants.
*/
private predicate isSVariant() {
exists(string name | name = getName() | name.suffix(name.length() - 2) = "_s")
}
override predicate hasArrayOutput(int bufParam) { bufParam = 0 }
/**
* Gets the index of the parameter that is the maximum size of the copy (in characters).
*/
int getParamSize() {
if isSVariant()
then result = 1
else
if exists(getName().indexOf("ncpy"))
then result = 2
else none()
}
override predicate hasArrayWithNullTerminator(int bufParam) { bufParam = 1 }
/**
* Gets the index of the parameter that is the source of the copy.
*/
int getParamSrc() { if isSVariant() then result = 2 else result = 1 }
/**
* Gets the index of the parameter that is the destination of the copy.
*/
int getParamDest() { result = 0 }
override predicate hasArrayInput(int bufParam) { bufParam = getParamSrc() }
override predicate hasArrayOutput(int bufParam) { bufParam = getParamDest() }
override predicate hasArrayWithNullTerminator(int bufParam) { bufParam = getParamSrc() }
override predicate hasArrayWithVariableSize(int bufParam, int countParam) {
(
this.hasName("strncpy") or
this.hasName("_strncpy_l") or
this.hasName("_mbsncpy") or
this.hasName("_mbsncpy_l") or
this.hasName("wcsncpy") or
this.hasName("_wcsncpy_l")
) and
bufParam = 0 and
countParam = 2
bufParam = getParamDest() and
countParam = getParamSize()
}
override predicate hasArrayWithUnknownSize(int bufParam) {
(
this.hasName("strcpy") or
this.hasName("_mbscpy") or
this.hasName("wcscpy")
) and
bufParam = 0
not exists(getParamSize()) and
bufParam = getParamDest()
}
override predicate hasDataFlow(FunctionInput input, FunctionOutput output) {
input.isParameterDeref(1) and
output.isParameterDeref(0)
input.isParameterDeref(getParamSrc()) and
output.isParameterDeref(getParamDest())
or
input.isParameterDeref(1) and
input.isParameterDeref(getParamSrc()) and
output.isReturnValueDeref()
or
input.isParameter(0) and
input.isParameter(getParamDest()) and
output.isReturnValue()
}
override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
// these may do only a partial copy of the input buffer to the output
// buffer
input.isParameter(getParamSize()) and
(
// these may do only a partial copy of the input buffer to the output
// buffer
this.hasName("strncpy") or
this.hasName("_strncpy_l") or
this.hasName("_mbsncpy") or
this.hasName("_mbsncpy_l") or
this.hasName("wcsncpy") or
this.hasName("_wcsncpy_l")
) and
input.isParameter(2) and
(
output.isParameterDeref(0) or
output.isParameterDeref(getParamDest()) or
output.isReturnValueDeref()
)
}
@@ -81,17 +118,18 @@ class StrcpyFunction extends ArrayFunction, DataFlowFunction, TaintFunction, Sid
override predicate hasOnlySpecificWriteSideEffects() { any() }
override predicate hasSpecificWriteSideEffect(ParameterIndex i, boolean buffer, boolean mustWrite) {
i = 0 and
i = getParamDest() and
buffer = true and
mustWrite = false
}
override predicate hasSpecificReadSideEffect(ParameterIndex i, boolean buffer) {
i = 1 and
i = getParamSrc() and
buffer = true
}
override ParameterIndex getParameterSizeIndex(ParameterIndex i) {
hasArrayWithVariableSize(i, result)
i = getParamDest() and
result = getParamSize()
}
}

78
cpp/ql/src/semmle/code/cpp/security/BufferWrite.qll
View File

@@ -10,6 +10,7 @@ import semmle.code.cpp.commons.Alloc
import semmle.code.cpp.commons.Buffer
import semmle.code.cpp.commons.Scanf
import semmle.code.cpp.models.implementations.Strcat
import semmle.code.cpp.models.implementations.Strcpy
/*
* --- BufferWrite framework ---
@@ -106,74 +107,19 @@ abstract class BufferWriteCall extends BufferWrite, FunctionCall { }
* A call to a variant of `strcpy`.
*/
class StrCopyBW extends BufferWriteCall {
StrCopyBW() {
exists(TopLevelFunction fn, string name | fn = getTarget() and name = fn.getName() |
// strcpy(dst, src)
name = "strcpy"
or
// wcscpy(dst, src)
name = "wcscpy"
or
// _mbscpy(dst, src)
name = "_mbscpy"
or
(
name = "strcpy_s" or // strcpy_s(dst, max_amount, src)
name = "wcscpy_s" or // wcscpy_s(dst, max_amount, src)
name = "_mbscpy_s" // _mbscpy_s(dst, max_amount, src)
) and
// exclude the 2-parameter template versions
// that find the size of a fixed size destination buffer.
fn.getNumberOfParameters() = 3
or
// strncpy(dst, src, max_amount)
name = "strncpy"
or
// strncpy_l(dst, src, max_amount, locale)
name = "strncpy_l"
or
// wcsncpy(dst, src, max_amount)
name = "wcsncpy"
or
// _wcsncpy_l(dst, src, max_amount, locale)
name = "_wcsncpy_l"
or
// _mbsncpy(dst, src, max_amount)
name = "_mbsncpy"
or
// _mbsncpy_l(dst, src, max_amount, locale)
name = "_mbsncpy_l"
)
}
StrcpyFunction f;
StrCopyBW() { getTarget() = f.(TopLevelFunction) }
/**
* Gets the index of the parameter that is the maximum size of the copy (in characters).
*/
int getParamSize() {
exists(TopLevelFunction fn, string name |
fn = getTarget() and
name = fn.getName() and
(
if name.suffix(name.length() - 2) = "_s"
then result = 1
else
if exists(name.indexOf("ncpy"))
then result = 2
else none()
)
)
}
int getParamSize() { result = f.getParamSize() }
/**
* Gets the index of the parameter that is the source of the copy.
*/
int getParamSrc() {
exists(TopLevelFunction fn, string name |
fn = getTarget() and
name = fn.getName() and
(if name.suffix(name.length() - 2) = "_s" then result = 2 else result = 1)
)
}
int getParamSrc() { result = f.getParamSrc() }
override Type getBufferType() {
result = this.getTarget().getParameter(getParamSrc()).getUnspecifiedType()
@@ -181,7 +127,7 @@ class StrCopyBW extends BufferWriteCall {
override Expr getASource() { result = getArgument(getParamSrc()) }
override Expr getDest() { result = getArgument(0) }
override Expr getDest() { result = getArgument(f.getParamDest()) }
override predicate hasExplicitLimit() { exists(getParamSize()) }
@@ -198,17 +144,19 @@ class StrCopyBW extends BufferWriteCall {
* A call to a variant of `strcat`.
*/
class StrCatBW extends BufferWriteCall {
StrCatBW() { exists(TopLevelFunction fn | fn = getTarget() and fn instanceof StrcatFunction) }
StrcatFunction f;
StrCatBW() { getTarget() = f.(TopLevelFunction) }
/**
* Gets the index of the parameter that is the maximum size of the copy (in characters).
*/
int getParamSize() { if exists(getArgument(2)) then result = 2 else none() }
int getParamSize() { result = f.getParamSize() }
/**
* Gets the index of the parameter that is the source of the copy.
*/
int getParamSrc() { result = 1 }
int getParamSrc() { result = f.getParamSrc() }
override Type getBufferType() {
result = this.getTarget().getParameter(getParamSrc()).getUnspecifiedType()
@@ -216,7 +164,7 @@ class StrCatBW extends BufferWriteCall {
override Expr getASource() { result = getArgument(getParamSrc()) }
override Expr getDest() { result = getArgument(0) }
override Expr getDest() { result = getArgument(f.getParamDest()) }
override predicate hasExplicitLimit() { exists(getParamSize()) }