github
/
codeql
зеркало из https://github.com/github/codeql.git
1
0
Форкнуть 0
Код Задачи Пакеты Проекты Релизы Вики Активность

Merge pull request #7272 from github/redsun82/cpp-overrunning-write-precision-split 

C++: refactor buffer overwrite queries with estimate reasons
This commit is contained in:
Mathias Vorreiter Pedersen 2021-12-14 08:50:30 +00:00 коммит произвёл GitHub
Родитель 1c79d1f985 0d7d60eebd
Коммит 6fda5e8f5b
Не найден ключ, соответствующий данной подписи
Идентификатор ключа GPG: 4AEE18F83AFDEB23
5 изменённых файлов: 296 добавлений и 94 удалений
Показать статистику Скачать Patch файл Скачать Diff файл Показать все файлы Свернуть все файлы

285
cpp/ql/lib/semmle/code/cpp/commons/Printf.qll
Просмотреть файл

@ -9,6 +9,83 @@ import semmle.code.cpp.models.interfaces.FormattingFunction
private import semmle.code.cpp.rangeanalysis.SimpleRangeAnalysis
private import semmle.code.cpp.rangeanalysis.RangeAnalysisUtils
private newtype TBufferWriteEstimationReason =
TNoSpecifiedEstimateReason() or
TTypeBoundsAnalysis() or
TValueFlowAnalysis()
/**
* A reason for a specific buffer write size estimate.
*/
abstract class BufferWriteEstimationReason extends TBufferWriteEstimationReason {
/**
* Returns the name of the concrete class.
*/
abstract string toString();
/**
* Returns a human readable representation of this reason.
*/
abstract string getDescription();
/**
* Combine estimate reasons. Used to give a reason for the size of a format string
* conversion given reasons coming from its individual specifiers.
*/
abstract BufferWriteEstimationReason combineWith(BufferWriteEstimationReason other);
}
/**
* No particular reason given. This is currently used for backward compatibility so that
* classes derived from BufferWrite and overriding `getMaxData/0` still work with the
* queries as intended.
*/
class NoSpecifiedEstimateReason extends BufferWriteEstimationReason, TNoSpecifiedEstimateReason {
override string toString() { result = "NoSpecifiedEstimateReason" }
override string getDescription() { result = "no reason specified" }
override BufferWriteEstimationReason combineWith(BufferWriteEstimationReason other) {
// this reason should not be used in format specifiers, so it should not be combined
// with other reasons
none()
}
}
/**
* The estimation comes from rough bounds just based on the type (e.g.
* `0 <= x < 2^32` for an unsigned 32 bit integer).
*/
class TypeBoundsAnalysis extends BufferWriteEstimationReason, TTypeBoundsAnalysis {
override string toString() { result = "TypeBoundsAnalysis" }
override string getDescription() { result = "based on type bounds" }
override BufferWriteEstimationReason combineWith(BufferWriteEstimationReason other) {
other != TNoSpecifiedEstimateReason() and result = TTypeBoundsAnalysis()
}
}
/**
* The estimation comes from non trivial bounds found via actual flow analysis.
* For example
* ```
* unsigned u = x;
* if (u < 1000) {
* //... <- estimation done here based on u
* }
* ```
*/
class ValueFlowAnalysis extends BufferWriteEstimationReason, TValueFlowAnalysis {
override string toString() { result = "ValueFlowAnalysis" }
override string getDescription() { result = "based on flow analysis of value bounds" }
override BufferWriteEstimationReason combineWith(BufferWriteEstimationReason other) {
other != TNoSpecifiedEstimateReason() and result = other
}
}
class PrintfFormatAttribute extends FormatAttribute {
PrintfFormatAttribute() { this.getArchetype() = ["printf", "__printf__"] }
}
@ -990,7 +1067,14 @@ class FormatLiteral extends Literal {
* conversion specifier of this format string; has no result if this cannot
* be determined.
*/
int getMaxConvertedLength(int n) {
int getMaxConvertedLength(int n) { result = max(getMaxConvertedLength(n, _)) }
/**
* Gets the maximum length of the string that can be produced by the nth
* conversion specifier of this format string, specifying the estimation reason;
* has no result if this cannot be determined.
*/
int getMaxConvertedLength(int n, BufferWriteEstimationReason reason) {
exists(int len |
(
(
@ -1002,10 +1086,12 @@ class FormatLiteral extends Literal {
) and
(
this.getConversionChar(n) = "%" and
len = 1
len = 1 and
reason = TValueFlowAnalysis()
or
this.getConversionChar(n).toLowerCase() = "c" and
len = 1 // e.g. 'a'
len = 1 and
reason = TValueFlowAnalysis() // e.g. 'a'
or
this.getConversionChar(n).toLowerCase() = "f" and
exists(int dot, int afterdot |
@ -1019,7 +1105,8 @@ class FormatLiteral extends Literal {
afterdot = 6
) and
len = 1 + 309 + dot + afterdot
) // e.g. -1e308="-100000"...
) and
reason = TTypeBoundsAnalysis() // e.g. -1e308="-100000"...
or
this.getConversionChar(n).toLowerCase() = "e" and
exists(int dot, int afterdot |
@ -1033,7 +1120,8 @@ class FormatLiteral extends Literal {
afterdot = 6
) and
len = 1 + 1 + dot + afterdot + 1 + 1 + 3
) // -1e308="-1.000000e+308"
) and
reason = TTypeBoundsAnalysis() // -1e308="-1.000000e+308"
or
this.getConversionChar(n).toLowerCase() = "g" and
exists(int dot, int afterdot |
@ -1056,67 +1144,80 @@ class FormatLiteral extends Literal {
// (e.g. 123456, 0.000123456 are just OK)
// so case %f can be at most P characters + 4 zeroes, sign, dot = P + 6
len = (afterdot.maximum(1) + 6).maximum(1 + 1 + dot + afterdot + 1 + 1 + 3)
) // (e.g. "-1.59203e-319")
) and
reason = TTypeBoundsAnalysis() // (e.g. "-1.59203e-319")
or
this.getConversionChar(n).toLowerCase() = ["d", "i"] and
// e.g. -2^31 = "-2147483648"
len =
min(float cand |
// The first case handles length sub-specifiers
// Subtract one in the exponent because one bit is for the sign.
// Add 1 to account for the possible sign in the output.
cand = 1 + lengthInBase10(2.pow(this.getIntegralDisplayType(n).getSize() * 8 - 1))
or
// The second case uses range analysis to deduce a length that's shorter than the length
// of the number -2^31.
exists(Expr arg, float lower, float upper |
arg = this.getUse().getConversionArgument(n) and
lower = lowerBound(arg.getFullyConverted()) and
upper = upperBound(arg.getFullyConverted())
|
cand =
max(int cand0 |
// Include the sign bit in the length if it can be negative
(
if lower < 0
then cand0 = 1 + lengthInBase10(lower.abs())
else cand0 = lengthInBase10(lower)
)
or
(
if upper < 0
then cand0 = 1 + lengthInBase10(upper.abs())
else cand0 = lengthInBase10(upper)
)
exists(float typeBasedBound, float valueBasedBound |
// The first case handles length sub-specifiers
// Subtract one in the exponent because one bit is for the sign.
// Add 1 to account for the possible sign in the output.
typeBasedBound =
1 + lengthInBase10(2.pow(this.getIntegralDisplayType(n).getSize() * 8 - 1)) and
// The second case uses range analysis to deduce a length that's shorter than the length
// of the number -2^31.
exists(Expr arg, float lower, float upper, float typeLower, float typeUpper |
arg = this.getUse().getConversionArgument(n) and
lower = lowerBound(arg.getFullyConverted()) and
upper = upperBound(arg.getFullyConverted()) and
typeLower = exprMinVal(arg.getFullyConverted()) and
typeUpper = exprMaxVal(arg.getFullyConverted())
|
valueBasedBound =
max(int cand |
// Include the sign bit in the length if it can be negative
(
if lower < 0
then cand = 1 + lengthInBase10(lower.abs())
else cand = lengthInBase10(lower)
)
or
(
if upper < 0
then cand = 1 + lengthInBase10(upper.abs())
else cand = lengthInBase10(upper)
)
) and
(
if lower > typeLower or upper < typeUpper
then reason = TValueFlowAnalysis()
else reason = TTypeBoundsAnalysis()
)
)
) and
len = valueBasedBound.minimum(typeBasedBound)
)
or
this.getConversionChar(n).toLowerCase() = "u" and
// e.g. 2^32 - 1 = "4294967295"
len =
min(float cand |
// The first case handles length sub-specifiers
cand = 2.pow(this.getIntegralDisplayType(n).getSize() * 8)
or
// The second case uses range analysis to deduce a length that's shorter than
// the length of the number 2^31 - 1.
exists(Expr arg, float lower |
arg = this.getUse().getConversionArgument(n) and
lower = lowerBound(arg.getFullyConverted())
|
cand =
max(float cand0 |
exists(float typeBasedBound, float valueBasedBound |
// The first case handles length sub-specifiers
typeBasedBound = lengthInBase10(2.pow(this.getIntegralDisplayType(n).getSize() * 8) - 1) and
// The second case uses range analysis to deduce a length that's shorter than
// the length of the number 2^31 - 1.
exists(Expr arg, float lower, float upper, float typeLower, float typeUpper |
arg = this.getUse().getConversionArgument(n) and
lower = lowerBound(arg.getFullyConverted()) and
upper = upperBound(arg.getFullyConverted()) and
typeLower = exprMinVal(arg.getFullyConverted()) and
typeUpper = exprMaxVal(arg.getFullyConverted())
|
valueBasedBound =
lengthInBase10(max(float cand |
// If lower can be negative we use `(unsigned)-1` as the candidate value.
lower < 0 and
cand0 = 2.pow(any(IntType t | t.isUnsigned()).getSize() * 8)
cand = 2.pow(any(IntType t | t.isUnsigned()).getSize() * 8)
or
cand0 = upperBound(arg.getFullyConverted())
)
cand = upper
)) and
(
if lower > typeLower or upper < typeUpper
then reason = TValueFlowAnalysis()
else reason = TTypeBoundsAnalysis()
)
|
lengthInBase10(cand)
)
) and
len = valueBasedBound.minimum(typeBasedBound)
)
or
this.getConversionChar(n).toLowerCase() = "x" and
// e.g. "12345678"
@ -1135,7 +1236,8 @@ class FormatLiteral extends Literal {
(
if this.hasAlternateFlag(n) then len = 2 + baseLen else len = baseLen // "0x"
)
)
) and
reason = TTypeBoundsAnalysis()
or
this.getConversionChar(n).toLowerCase() = "p" and
exists(PointerType ptrType, int baseLen |
@ -1144,7 +1246,8 @@ class FormatLiteral extends Literal {
(
if this.hasAlternateFlag(n) then len = 2 + baseLen else len = baseLen // "0x"
)
)
) and
reason = TValueFlowAnalysis()
or
this.getConversionChar(n).toLowerCase() = "o" and
// e.g. 2^32 - 1 = "37777777777"
@ -1163,14 +1266,16 @@ class FormatLiteral extends Literal {
(
if this.hasAlternateFlag(n) then len = 1 + baseLen else len = baseLen // "0"
)
)
) and
reason = TTypeBoundsAnalysis()
or
this.getConversionChar(n).toLowerCase() = "s" and
len =
min(int v |
v = this.getPrecision(n) or
v = this.getUse().getFormatArgument(n).(AnalysedString).getMaxLength() - 1 // (don't count null terminator)
)
) and
reason = TValueFlowAnalysis()
)
)
}
@ -1182,10 +1287,19 @@ class FormatLiteral extends Literal {
* determining whether a buffer overflow is caused by long float to string
* conversions.
*/
int getMaxConvertedLengthLimited(int n) {
int getMaxConvertedLengthLimited(int n) { result = max(getMaxConvertedLengthLimited(n, _)) }
/**
* Gets the maximum length of the string that can be produced by the nth
* conversion specifier of this format string, specifying the reason for the
* estimation, except that float to string conversions are assumed to be 8
* characters. This is helpful for determining whether a buffer overflow is
* caused by long float to string conversions.
*/
int getMaxConvertedLengthLimited(int n, BufferWriteEstimationReason reason) {
if this.getConversionChar(n).toLowerCase() = "f"
then result = this.getMaxConvertedLength(n).minimum(8)
else result = this.getMaxConvertedLength(n)
then result = this.getMaxConvertedLength(n, reason).minimum(8)
else result = this.getMaxConvertedLength(n, reason)
}
/**
@ -1225,29 +1339,35 @@ class FormatLiteral extends Literal {
)
}
private int getMaxConvertedLengthAfter(int n) {
private int getMaxConvertedLengthAfter(int n, BufferWriteEstimationReason reason) {
if n = this.getNumConvSpec()
then result = this.getConstantSuffix().length() + 1
then result = this.getConstantSuffix().length() + 1 and reason = TValueFlowAnalysis()
else
result =
this.getConstantPart(n).length() + this.getMaxConvertedLength(n) +
this.getMaxConvertedLengthAfter(n + 1)
exists(BufferWriteEstimationReason headReason, BufferWriteEstimationReason tailReason |
result =
this.getConstantPart(n).length() + this.getMaxConvertedLength(n, headReason) +
this.getMaxConvertedLengthAfter(n + 1, tailReason) and
reason = headReason.combineWith(tailReason)
)
}
private int getMaxConvertedLengthAfterLimited(int n) {
private int getMaxConvertedLengthAfterLimited(int n, BufferWriteEstimationReason reason) {
if n = this.getNumConvSpec()
then result = this.getConstantSuffix().length() + 1
then result = this.getConstantSuffix().length() + 1 and reason = TValueFlowAnalysis()
else
result =
this.getConstantPart(n).length() + this.getMaxConvertedLengthLimited(n) +
this.getMaxConvertedLengthAfterLimited(n + 1)
exists(BufferWriteEstimationReason headReason, BufferWriteEstimationReason tailReason |
result =
this.getConstantPart(n).length() + this.getMaxConvertedLengthLimited(n, headReason) +
this.getMaxConvertedLengthAfterLimited(n + 1, tailReason) and
reason = headReason.combineWith(tailReason)
)
}
/**
* Gets the maximum length of the string that can be produced by this format
* string. Has no result if this cannot be determined.
*/
int getMaxConvertedLength() { result = this.getMaxConvertedLengthAfter(0) }
int getMaxConvertedLength() { result = this.getMaxConvertedLengthAfter(0, _) }
/**
* Gets the maximum length of the string that can be produced by this format
@ -1255,5 +1375,24 @@ class FormatLiteral extends Literal {
* characters. This is helpful for determining whether a buffer overflow
* is caused by long float to string conversions.
*/
int getMaxConvertedLengthLimited() { result = this.getMaxConvertedLengthAfterLimited(0) }
int getMaxConvertedLengthLimited() { result = this.getMaxConvertedLengthAfterLimited(0, _) }
/**
* Gets the maximum length of the string that can be produced by this format
* string, specifying the reason for the estimate. Has no result if no estimate
* can be found.
*/
int getMaxConvertedLengthWithReason(BufferWriteEstimationReason reason) {
result = this.getMaxConvertedLengthAfter(0, reason)
}
/**
* Gets the maximum length of the string that can be produced by this format
* string, specifying the reason for the estimate, except that float to string
* conversions are assumed to be 8 characters. This is helpful for determining
* whether a buffer overflow is caused by long float to string conversions.
*/
int getMaxConvertedLengthLimitedWithReason(BufferWriteEstimationReason reason) {
result = this.getMaxConvertedLengthAfterLimited(0, reason)
}
}

87
cpp/ql/lib/semmle/code/cpp/security/BufferWrite.qll
Просмотреть файл

@ -71,13 +71,30 @@ abstract class BufferWrite extends Expr {
*/
int getMaxData() { none() }
/**
* Gets an upper bound to the amount of data that's being written (if one
* can be found), specifying the reason for the estimation.
*/
int getMaxData(BufferWriteEstimationReason reason) {
reason instanceof NoSpecifiedEstimateReason and result = getMaxData()
}
/**
* Gets an upper bound to the amount of data that's being written (if one
* can be found), except that float to string conversions are assumed to be
* much smaller (8 bytes) than their true maximum length. This can be
* helpful in determining the cause of a buffer overflow issue.
*/
int getMaxDataLimited() { result = this.getMaxData() }
int getMaxDataLimited() { result = getMaxData() }
/**
* Gets an upper bound to the amount of data that's being written (if one
* can be found), specifying the reason for the estimation, except that
* float to string conversions are assumed to be much smaller (8 bytes)
* than their true maximum length. This can be helpful in determining the
* cause of a buffer overflow issue.
*/
int getMaxDataLimited(BufferWriteEstimationReason reason) { result = getMaxData(reason) }
/**
* Gets the size of a single character of the type this
@ -135,10 +152,16 @@ class StrCopyBW extends BufferWriteCall {
result = this.getArgument(this.getParamSize()).getValue().toInt() * this.getCharSize()
}
override int getMaxData() {
private int getMaxDataImpl(BufferWriteEstimationReason reason) {
// when result exists, it is an exact flow analysis
reason instanceof ValueFlowAnalysis and
result =
this.getArgument(this.getParamSrc()).(AnalysedString).getMaxLength() * this.getCharSize()
}
override int getMaxData(BufferWriteEstimationReason reason) { result = getMaxDataImpl(reason) }
override int getMaxData() { result = max(getMaxDataImpl(_)) }
}
/**
@ -173,10 +196,16 @@ class StrCatBW extends BufferWriteCall {
result = this.getArgument(this.getParamSize()).getValue().toInt() * this.getCharSize()
}
override int getMaxData() {
private int getMaxDataImpl(BufferWriteEstimationReason reason) {
// when result exists, it is an exact flow analysis
reason instanceof ValueFlowAnalysis and
result =
this.getArgument(this.getParamSrc()).(AnalysedString).getMaxLength() * this.getCharSize()
}
override int getMaxData(BufferWriteEstimationReason reason) { result = getMaxDataImpl(reason) }
override int getMaxData() { result = max(getMaxDataImpl(_)) }
}
/**
@ -233,19 +262,29 @@ class SprintfBW extends BufferWriteCall {
override Expr getDest() { result = this.getArgument(f.getOutputParameterIndex(false)) }
override int getMaxData() {
private int getMaxDataImpl(BufferWriteEstimationReason reason) {
exists(FormatLiteral fl |
fl = this.(FormattingFunctionCall).getFormat() and
result = fl.getMaxConvertedLength() * this.getCharSize()
result = fl.getMaxConvertedLengthWithReason(reason) * this.getCharSize()
)
}
override int getMaxDataLimited() {
override int getMaxData(BufferWriteEstimationReason reason) { result = getMaxDataImpl(reason) }
override int getMaxData() { result = max(getMaxDataImpl(_)) }
private int getMaxDataLimitedImpl(BufferWriteEstimationReason reason) {
exists(FormatLiteral fl |
fl = this.(FormattingFunctionCall).getFormat() and
result = fl.getMaxConvertedLengthLimited() * this.getCharSize()
result = fl.getMaxConvertedLengthLimitedWithReason(reason) * this.getCharSize()
)
}
override int getMaxDataLimited(BufferWriteEstimationReason reason) {
result = getMaxDataLimitedImpl(reason)
}
override int getMaxDataLimited() { result = max(getMaxDataLimitedImpl(_)) }
}
/**
@ -336,19 +375,29 @@ class SnprintfBW extends BufferWriteCall {
result = this.getArgument(this.getParamSize()).getValue().toInt() * this.getCharSize()
}
override int getMaxData() {
private int getMaxDataImpl(BufferWriteEstimationReason reason) {
exists(FormatLiteral fl |
fl = this.(FormattingFunctionCall).getFormat() and
result = fl.getMaxConvertedLength() * this.getCharSize()
result = fl.getMaxConvertedLengthWithReason(reason) * this.getCharSize()
)
}
override int getMaxDataLimited() {
override int getMaxData(BufferWriteEstimationReason reason) { result = getMaxDataImpl(reason) }
override int getMaxData() { result = max(getMaxDataImpl(_)) }
private int getMaxDataLimitedImpl(BufferWriteEstimationReason reason) {
exists(FormatLiteral fl |
fl = this.(FormattingFunctionCall).getFormat() and
result = fl.getMaxConvertedLengthLimited() * this.getCharSize()
result = fl.getMaxConvertedLengthLimitedWithReason(reason) * this.getCharSize()
)
}
override int getMaxDataLimited(BufferWriteEstimationReason reason) {
result = getMaxDataLimitedImpl(reason)
}
override int getMaxDataLimited() { result = max(getMaxDataLimitedImpl(_)) }
}
/**
@ -436,7 +485,9 @@ class ScanfBW extends BufferWrite {
override Expr getDest() { result = this }
override int getMaxData() {
private int getMaxDataImpl(BufferWriteEstimationReason reason) {
// when this returns, it is based on exact flow analysis
reason instanceof ValueFlowAnalysis and
exists(ScanfFunctionCall fc, ScanfFormatLiteral fl, int arg |
this = fc.getArgument(arg) and
fl = fc.getFormat() and
@ -444,6 +495,10 @@ class ScanfBW extends BufferWrite {
)
}
override int getMaxData(BufferWriteEstimationReason reason) { result = getMaxDataImpl(reason) }
override int getMaxData() { result = max(getMaxDataImpl(_)) }
override string getBWDesc() {
exists(FunctionCall fc |
this = fc.getArgument(_) and
@ -474,8 +529,14 @@ class RealpathBW extends BufferWriteCall {
override Expr getASource() { result = this.getArgument(0) }
override int getMaxData() {
private int getMaxDataImpl(BufferWriteEstimationReason reason) {
// although there may be some unknown invariants guaranteeing that a real path is shorter than PATH_MAX, we can consider providing less than PATH_MAX a problem with high precision
reason instanceof ValueFlowAnalysis and
result = path_max() and
this = this // Suppress a compiler warning
}
override int getMaxData(BufferWriteEstimationReason reason) { result = getMaxDataImpl(reason) }
override int getMaxData() { result = max(getMaxDataImpl(_)) }
}

7
cpp/ql/src/Security/CWE/CWE-120/OverrunWrite.ql
Просмотреть файл

@ -21,14 +21,15 @@ import semmle.code.cpp.commons.Alloc
* See CWE-120/UnboundedWrite.ql for a summary of CWE-120 alert cases.
*/
from BufferWrite bw, Expr dest, int destSize
from BufferWrite bw, Expr dest, int destSize, int estimated
where
not bw.hasExplicitLimit() and // has no explicit size limit
dest = bw.getDest() and
destSize = getBufferSize(dest, _) and
estimated = bw.getMaxDataLimited(_) and
// we can deduce that too much data may be copied (even without
// long '%f' conversions)
bw.getMaxDataLimited() > destSize
estimated > destSize
select bw,
"This '" + bw.getBWDesc() + "' operation requires " + bw.getMaxData() +
"This '" + bw.getBWDesc() + "' operation requires " + estimated +
" bytes but the destination is only " + destSize + " bytes."

9
cpp/ql/src/Security/CWE/CWE-120/OverrunWriteFloat.ql
Просмотреть файл

@ -21,14 +21,15 @@ import semmle.code.cpp.security.BufferWrite
* See CWE-120/UnboundedWrite.ql for a summary of CWE-120 alert cases.
*/
from BufferWrite bw, int destSize
from BufferWrite bw, int destSize, int estimated, BufferWriteEstimationReason reason
where
not bw.hasExplicitLimit() and
// has no explicit size limit
destSize = getBufferSize(bw.getDest(), _) and
bw.getMaxData() > destSize and
estimated = bw.getMaxData(reason) and
estimated > destSize and
// and we can deduce that too much data may be copied
bw.getMaxDataLimited() <= destSize // but it would fit without long '%f' conversions
bw.getMaxDataLimited(reason) <= destSize // but it would fit without long '%f' conversions
select bw,
"This '" + bw.getBWDesc() + "' operation may require " + bw.getMaxData() +
"This '" + bw.getBWDesc() + "' operation may require " + estimated +
" bytes because of float conversions, but the target is only " + destSize + " bytes."

2
cpp/ql/src/Security/CWE/CWE-120/UnboundedWrite.ql
Просмотреть файл

@ -44,7 +44,7 @@ import TaintedWithPath
predicate isUnboundedWrite(BufferWrite bw) {
not bw.hasExplicitLimit() and // has no explicit size limit
not exists(bw.getMaxData()) // and we can't deduce an upper bound to the amount copied
not exists(bw.getMaxData(_)) // and we can't deduce an upper bound to the amount copied
}
/*