Merge branch 'github:main' into amammad-cpp-bombs

This commit is contained in:
Am
2024-07-28 18:49:15 +03:30
committed by GitHub
1751 changed files with 139062 additions and 46202 deletions

View File

@@ -1,3 +1,28 @@
## 1.1.0
### Query Metadata Changes
* The precision of `cpp/iterator-to-expired-container` ("Iterator to expired container") has been increased to `high`. As a result, it will be run by default as part of the Code Scanning suite.
* The precision of `cpp/unsafe-strncat` ("Potentially unsafe call to strncat") has been increased to `high`. As a result, it will be run by default as part of the Code Scanning suite.
### Minor Analysis Improvements
* The `cpp/unsigned-difference-expression-compared-zero` ("Unsigned difference expression compared to zero") query now produces fewer false positives.
## 1.0.3
No user-facing changes.
## 1.0.2
No user-facing changes.
## 1.0.1
### Minor Analysis Improvements
* The `cpp/dangerous-function-overflow` no longer produces a false positive alert when the `gets` function does not have exactly one parameter.
## 1.0.0
### Breaking Changes

View File

@@ -38,11 +38,18 @@ private string getEofValue() {
private predicate checkedForEof(ScanfFunctionCall call) {
exists(IRGuardCondition gc |
exists(Instruction i | i.getUnconvertedResultExpression() = call |
// call == EOF
gc.comparesEq(valueNumber(i).getAUse(), getEofValue().toInt(), _, _)
exists(int val | gc.comparesEq(valueNumber(i).getAUse(), val, _, _) |
// call == EOF
val = getEofValue().toInt()
or
// call == [any positive number]
val > 0
)
or
// call < 0 (EOF is guaranteed to be negative)
gc.comparesLt(valueNumber(i).getAUse(), 0, true, _)
exists(int val | gc.comparesLt(valueNumber(i).getAUse(), val, true, _) |
// call < [any non-negative number] (EOF is guaranteed to be negative)
val >= 0
)
)
)
}

View File

@@ -15,6 +15,7 @@
import cpp
import semmle.code.cpp.models.Models
import semmle.code.cpp.commons.Buffer
predicate baseType(AllocationExpr alloc, Type base) {
exists(PointerType pointer |
@@ -30,7 +31,8 @@ predicate baseType(AllocationExpr alloc, Type base) {
}
predicate decideOnSize(Type t, int size) {
// If the codebase has more than one type with the same name, it can have more than one size.
// If the codebase has more than one type with the same name, it can have more than one size. For
// most purposes in this query, we use the smallest.
size = min(t.getSize())
}
@@ -45,7 +47,8 @@ where
size = 0 or
(allocated / size) * size = allocated
) and
not basesize > allocated // covered by SizeCheck.ql
not basesize > allocated and // covered by SizeCheck.ql
not memberMayBeVarSize(base.getUnspecifiedType(), _) // exclude variable size types
select alloc,
"Allocated memory (" + allocated.toString() + " bytes) is not a multiple of the size of '" +
base.getName() + "' (" + basesize.toString() + " bytes)."

View File

@@ -172,5 +172,5 @@ where
not arg.isFromUninstantiatedTemplate(_) and
not actual.getUnspecifiedType() instanceof ErroneousType
select arg,
"This argument should be of type '" + expected.getName() + "' but is of type '" +
"This format specifier for type '" + expected.getName() + "' does not match the argument type '" +
actual.getUnspecifiedType().getName() + "'."

View File

@@ -1,7 +0,0 @@
Record* fixRecord(Record* r) {
Record myRecord = *r;
delete r;
myRecord.fix();
return &myRecord; //returns reference to myRecord, which is a stack-allocated object
}

View File

@@ -5,22 +5,23 @@
<overview>
<p>This rule finds return statements that return pointers to an object allocated on the stack.
The lifetime of a stack allocated memory location only lasts until the function returns, and
the contents of that memory become undefined after that. Clearly, using a pointer to stack
<p>This rule finds return statements that return pointers to an object allocated on the stack.
The lifetime of a stack allocated memory location only lasts until the function returns, and
the contents of that memory become undefined after that. Clearly, using a pointer to stack
memory after the function has already returned will have undefined results. </p>
</overview>
<recommendation>
<p>Use the functions of the <tt>malloc</tt> family to dynamically allocate memory on the heap for data that is used across function calls.</p>
<p>Use the functions of the <tt>malloc</tt> family, or <tt>new</tt>, to dynamically allocate memory on the heap for data that is used across function calls.</p>
</recommendation>
<example><sample src="ReturnStackAllocatedMemory.cpp" />
<example>
<p>The following example allocates an object on the stack and returns a pointer to it. This is incorrect because the object is deallocated
when the function returns, and the pointer becomes invalid.</p>
<sample src="ReturnStackAllocatedMemoryBad.cpp" />
<p>To fix this, allocate the object on the heap using <tt>new</tt> and return a pointer to the heap-allocated object.</p>
<sample src="ReturnStackAllocatedMemoryGood.cpp" />
</example>
<references>

View File

@@ -0,0 +1,5 @@
Record *mkRecord(int value) {
Record myRecord(value);
return &myRecord; // BAD: returns a pointer to `myRecord`, which is a stack-allocated object.
}

View File

@@ -0,0 +1,5 @@
Record *mkRecord(int value) {
Record *myRecord = new Record(value);
return myRecord; // GOOD: returns a pointer to a `myRecord`, which is a heap-allocated object.
}

View File

@@ -4,7 +4,7 @@
* @kind problem
* @problem.severity warning
* @security-severity 9.3
* @precision medium
* @precision high
* @id cpp/unsafe-strncat
* @tags reliability
* correctness

View File

@@ -10,11 +10,12 @@ contains sensitive data that could somehow be retrieved by an attacker.</p>
</overview>
<recommendation>
<p>Use alternative platform-supplied functions that will not get optimized away. Examples of such
functions include <code>memset_s</code>, <code>SecureZeroMemory</code>, and <code>bzero_explicit</code>.
Alternatively, passing the <code>-fno-builtin-memset</code> option to the GCC/Clang compiler usually
also prevents the optimization. Finally, you can use the public-domain <code>secure_memzero</code> function
(see references below). This function, however, is not guaranteed to work on all platforms and compilers.</p>
<p>Use <code>memset_s</code> (from C11) instead of <code>memset</code>, as <code>memset_s</code> will not
get optimized away. Alternatively use platform-supplied functions such as <code>SecureZeroMemory</code> or
<code>bzero_explicit</code> that make the same guarantee. Passing the <code>-fno-builtin-memset</code>
option to the GCC/Clang compiler usually also prevents the optimization. Finally, you can use the
public-domain <code>secure_memzero</code> function (see references below). This function, however, is not
guaranteed to work on all platforms and compilers.</p>
</recommendation>
<example>

View File

@@ -1,5 +1,14 @@
unsigned limit = get_limit();
unsigned total = 0;
while (limit - total > 0) { // wrong: if `total` is greater than `limit` this will underflow and continue executing the loop.
uint32_t limit = get_limit();
uint32_t total = 0;
while (limit - total > 0) { // BAD: if `total` is greater than `limit` this will underflow and continue executing the loop.
total += get_data();
}
}
while (total < limit) { // GOOD: never underflows here because there is no arithmetic.
total += get_data();
}
while ((int64_t)limit - total > 0) { // GOOD: never underflows here because the result always fits in an `int64_t`.
total += get_data();
}

View File

@@ -5,7 +5,7 @@
* @id cpp/unsigned-difference-expression-compared-zero
* @problem.severity warning
* @security-severity 9.8
* @precision medium
* @precision high
* @tags security
* correctness
* external/cwe/cwe-191
@@ -17,6 +17,7 @@ import semmle.code.cpp.rangeanalysis.SimpleRangeAnalysis
import semmle.code.cpp.rangeanalysis.RangeAnalysisUtils
import semmle.code.cpp.controlflow.Guards
import semmle.code.cpp.ir.dataflow.DataFlow
import semmle.code.cpp.valuenumbering.GlobalValueNumbering
/**
* Holds if `sub` is guarded by a condition which ensures that
@@ -33,46 +34,109 @@ predicate isGuarded(SubExpr sub, Expr left, Expr right) {
)
}
/**
* Gets an expression that is less than or equal to `sub.getLeftOperand()`.
* These serve as the base cases for `exprIsSubLeftOrLess`.
*/
Expr exprIsLeftOrLessBase(SubExpr sub) {
interestingSubExpr(sub, _) and // Manual magic
exists(Expr e | globalValueNumber(e).getAnExpr() = sub.getLeftOperand() |
// sub = e - x
// result = e
// so:
// result <= e
result = e
or
// sub = e - x
// result = e & y
// so:
// result = e & y <= e
result.(BitwiseAndExpr).getAnOperand() = e
or
exists(SubExpr s |
// sub = e - x
// result = s
// s = e - y
// y >= 0
// so:
// result = e - y <= e
result = s and
s.getLeftOperand() = e and
lowerBound(s.getRightOperand().getFullyConverted()) >= 0
)
or
exists(Expr other |
// sub = e - x
// result = a
// a = e + y
// y <= 0
// so:
// result = e + y <= e + 0 = e
result.(AddExpr).hasOperands(e, other) and
upperBound(other.getFullyConverted()) <= 0
)
or
exists(DivExpr d |
// sub = e - x
// result = d
// d = e / y
// y >= 1
// so:
// result = e / y <= e / 1 = e
result = d and
d.getLeftOperand() = e and
lowerBound(d.getRightOperand().getFullyConverted()) >= 1
)
or
exists(RShiftExpr rs |
// sub = e - x
// result = rs
// rs = e >> y
// so:
// result = e >> y <= e
result = rs and
rs.getLeftOperand() = e
)
)
}
/**
* Holds if `n` is known or suspected to be less than or equal to
* `sub.getLeftOperand()`.
*/
predicate exprIsSubLeftOrLess(SubExpr sub, DataFlow::Node n) {
interestingSubExpr(sub, _) and // Manual magic
(
n.asExpr() = sub.getLeftOperand()
or
exists(DataFlow::Node other |
// dataflow
exprIsSubLeftOrLess(sub, other) and
(
DataFlow::localFlowStep(n, other) or
DataFlow::localFlowStep(other, n)
)
)
or
exists(DataFlow::Node other |
// guard constraining `sub`
exprIsSubLeftOrLess(sub, other) and
isGuarded(sub, other.asExpr(), n.asExpr()) // other >= n
)
or
exists(DataFlow::Node other, float p, float q |
// linear access of `other`
exprIsSubLeftOrLess(sub, other) and
linearAccess(n.asExpr(), other.asExpr(), p, q) and // n = p * other + q
p <= 1 and
q <= 0
)
or
exists(DataFlow::Node other, float p, float q |
// linear access of `n`
exprIsSubLeftOrLess(sub, other) and
linearAccess(other.asExpr(), n.asExpr(), p, q) and // other = p * n + q
p >= 1 and
q >= 0
n.asExpr() = exprIsLeftOrLessBase(sub)
or
exists(DataFlow::Node other |
// dataflow
exprIsSubLeftOrLess(sub, other) and
(
DataFlow::localFlowStep(n, other) or
DataFlow::localFlowStep(other, n)
)
)
or
exists(DataFlow::Node other |
// guard constraining `sub`
exprIsSubLeftOrLess(sub, other) and
isGuarded(sub, other.asExpr(), n.asExpr()) // other >= n
)
or
exists(DataFlow::Node other, float p, float q |
// linear access of `other`
exprIsSubLeftOrLess(sub, other) and
linearAccess(n.asExpr(), other.asExpr(), p, q) and // n = p * other + q
p <= 1 and
q <= 0
)
or
exists(DataFlow::Node other, float p, float q |
// linear access of `n`
exprIsSubLeftOrLess(sub, other) and
linearAccess(other.asExpr(), n.asExpr(), p, q) and // other = p * n + q
p >= 1 and
q >= 0
)
}
predicate interestingSubExpr(SubExpr sub, RelationalOperation ro) {

View File

@@ -1,12 +1,31 @@
void writeCredentials() {
char *password = "cleartext password";
FILE* file = fopen("credentials.txt", "w");
#include <sodium.h>
#include <stdio.h>
#include <string.h>
void writeCredentialsBad(FILE *file, const char *cleartextCredentials) {
// BAD: write password to disk in cleartext
fputs(password, file);
// GOOD: encrypt password first
char *encrypted = encrypt(password);
fputs(encrypted, file);
fputs(cleartextCredentials, file);
}
int writeCredentialsGood(FILE *file, const char *cleartextCredentials, const unsigned char *key, const unsigned char *nonce) {
size_t credentialsLen = strlen(cleartextCredentials);
size_t ciphertext_len = crypto_secretbox_MACBYTES + credentialsLen;
unsigned char *ciphertext = malloc(ciphertext_len);
if (!ciphertext) {
logError();
return -1;
}
// encrypt the password first
if (crypto_secretbox_easy(ciphertext, (const unsigned char *)cleartextCredentials, credentialsLen, nonce, key) != 0) {
free(ciphertext);
logError();
return -1;
}
// GOOD: write encrypted password to disk
fwrite(ciphertext, 1, ciphertext_len, file);
free(ciphertext);
return 0;
}

View File

@@ -19,15 +19,20 @@ cleartext.</p>
<example>
<p>The following example shows two ways of storing user credentials in a file. In the 'BAD' case,
the credentials are simply stored in cleartext. In the 'GOOD' case, the credentials are encrypted before
the credentials are simply stored in cleartext. In the 'GOOD' case, the credentials are encrypted before
storing them.</p>
<sample src="CleartextStorage.c" />
<p>Note that for the 'GOOD' example to work we need to link against an encryption library (in this case libsodium),
initialize it with a call to <code>sodium_init</code>, and create the key and nonce with
<code>crypto_secretbox_keygen</code> and <code>randombytes_buf</code> respectively. We also need to store those
details securely so they can be used for decryption.</p>
</example>
<references>
<li>M. Dowd, J. McDonald and J. Schuhm, <i>The Art of Software Security Assessment</i>, 1st Edition, Chapter 2 - 'Common Vulnerabilities of Encryption', p. 43. Addison Wesley, 2006.</li>
<li>M. Dowd, J. McDonald and J. Schuhm, <i>The Art of Software Security Assessment</i>, 1st Edition, Chapter 2 - 'Common Vulnerabilities of Encryption', p. 43. Addison Wesley, 2006.</li>
<li>M. Howard and D. LeBlanc, <i>Writing Secure Code</i>, 2nd Edition, Chapter 9 - 'Protecting Secret Data', p. 299. Microsoft, 2002.</li>

View File

@@ -1,6 +1,6 @@
void bad(void) {
char *password = "cleartext password";
const char *password = "cleartext password";
sqlite3 *credentialsDB;
sqlite3_stmt *stmt;
@@ -16,14 +16,15 @@ void bad(void) {
}
}
void good(void) {
char *password = "cleartext password";
void good(const char *secretKey) {
const char *password = "cleartext password";
sqlite3 *credentialsDB;
sqlite3_stmt *stmt;
if (sqlite3_open("credentials.db", &credentialsDB) == SQLITE_OK) {
// GOOD: database encryption enabled:
sqlite3_exec(credentialsDB, "PRAGMA key = 'secretKey!'", NULL, NULL, NULL);
std::string setKeyString = std::string("PRAGMA key = '") + secretKey + "'";
sqlite3_exec(credentialsDB, setKeyString.c_str(), NULL, NULL, NULL);
sqlite3_exec(credentialsDB, "CREATE TABLE IF NOT EXISTS creds (password TEXT);", NULL, NULL, NULL);
if (sqlite3_prepare_v2(credentialsDB, "INSERT INTO creds(password) VALUES(?)", -1, &stmt, NULL) == SQLITE_OK) {
sqlite3_bind_text(stmt, 1, password, -1, SQLITE_TRANSIENT);
@@ -33,4 +34,3 @@ void good(void) {
}
}
}

View File

@@ -20,10 +20,12 @@ In the 'GOOD' case, the database (and thus the credentials) are encrypted.</p>
<sample src="CleartextSqliteDatabase.c" />
<p>Note that for the 'GOOD' example to work we need to provide a secret key. Secure key generation and storage is required.</p>
</example>
<references>
<li>M. Dowd, J. McDonald and J. Schuhm, <i>The Art of Software Security Assessment</i>, 1st Edition, Chapter 2 - 'Common Vulnerabilities of Encryption', p. 43. Addison Wesley, 2006.</li>
<li>M. Dowd, J. McDonald and J. Schuhm, <i>The Art of Software Security Assessment</i>, 1st Edition, Chapter 2 - 'Common Vulnerabilities of Encryption', p. 43. Addison Wesley, 2006.</li>
<li>M. Howard and D. LeBlanc, <i>Writing Secure Code</i>, 2nd Edition, Chapter 9 - 'Protecting Secret Data', p. 299. Microsoft, 2002.</li>

View File

@@ -1,15 +1,17 @@
char *file_name;
FILE *f_ptr;
/* Initialize file_name */
f_ptr = fopen(file_name, "w");
if (f_ptr == NULL) {
/* Handle error */
}
/* ... */
if (chmod(file_name, S_IRUSR) == -1) {
/* Handle error */
}
}
fclose(f_ptr);

View File

@@ -1,8 +1,8 @@
char *file_name;
int fd;
/* Initialize file_name */
fd = open(
file_name,
O_WRONLY | O_CREAT | O_EXCL,
@@ -11,9 +11,11 @@ fd = open(
if (fd == -1) {
/* Handle error */
}
/* ... */
if (fchmod(fd, S_IRUSR) == -1) {
/* Handle error */
}
}
close(fd);

View File

@@ -2,7 +2,7 @@
* @name Iterator to expired container
* @description Using an iterator owned by a container whose lifetime has expired may lead to unexpected behavior.
* @kind problem
* @precision medium
* @precision high
* @id cpp/iterator-to-expired-container
* @problem.severity warning
* @security-severity 8.8

View File

@@ -34,7 +34,7 @@ void good1(std::size_t length) noexcept {
// GOOD: the allocation failure is handled appropriately.
void good2(std::size_t length) noexcept {
int* dest = new int[length];
int* dest = new(std::nothrow) int[length];
if(!dest) {
return;
}

View File

@@ -215,18 +215,24 @@ predicate noThrowInTryBlock(NewOrNewArrayExpr newExpr, BadAllocCatchBlock catchB
*/
predicate nullCheckInThrowingNew(NewOrNewArrayExpr newExpr, GuardCondition guard) {
newExpr.getAllocator() instanceof ThrowingAllocator and
(
// Handles null comparisons.
guard.ensuresEq(globalValueNumber(newExpr).getAnExpr(), any(NullValue null), _, _, _)
or
// Handles `if(ptr)` and `if(!ptr)` cases.
guard = globalValueNumber(newExpr).getAnExpr()
)
// There can be many guard conditions that compares `newExpr` againgst 0.
// For example, for `if(!p)` both `p` and `!p` are guard conditions. To not
// produce duplicates results we pick the "first" guard condition according
// to some arbitrary ordering (i.e., location information). This means `!p` is the
// element that we use to construct the alert.
guard =
min(GuardCondition gc, int startline, int startcolumn, int endline, int endcolumn |
gc.comparesEq(globalValueNumber(newExpr).getAnExpr(), 0, _, _) and
gc.getLocation().hasLocationInfo(_, startline, startcolumn, endline, endcolumn)
|
gc order by startline, startcolumn, endline, endcolumn
)
}
from NewOrNewArrayExpr newExpr, Element element, string msg, string elementString
where
not newExpr.isFromUninstantiatedTemplate(_) and
not newExpr.isFromTemplateInstantiation(_) and
(
noThrowInTryBlock(newExpr, element) and
msg = "This allocation cannot throw. $@ is unnecessary." and

View File

@@ -1,11 +1,38 @@
void write_default_config_bad() {
// BAD - this is world-writable so any user can overwrite the config
int out = creat(OUTFILE, 0666);
dprintf(out, DEFAULT_CONFIG);
if (out < 0) {
// handle error
}
dprintf(out, "%s", DEFAULT_CONFIG);
close(out);
}
void write_default_config_good() {
// GOOD - this allows only the current user to modify the file
int out = creat(OUTFILE, S_IWUSR | S_IRUSR);
dprintf(out, DEFAULT_CONFIG);
if (out < 0) {
// handle error
}
dprintf(out, "%s", DEFAULT_CONFIG);
close(out);
}
void write_default_config_good_2() {
// GOOD - this allows only the current user to modify the file
int out = open(OUTFILE, O_WRONLY | O_CREAT, S_IWUSR | S_IRUSR);
if (out < 0) {
// handle error
}
FILE *fd = fdopen(out, "w");
if (fd == NULL) {
close(out);
// handle error
}
fprintf(fd, "%s", DEFAULT_CONFIG);
fclose(fd);
}

View File

@@ -29,10 +29,11 @@ so it is important that they cannot be controlled by an attacker.
</p>
<p>
The first example creates the default configuration file with the usual "default" Unix permissions, <code>0666</code>. This makes the
The first example creates the default configuration file with the usual "default" Unix permissions, <code>0666</code>. This makes the
file world-writable, so that an attacker could write in their own configuration that would be read by the program. The second example uses
more restrictive permissions: a combination of the standard Unix constants <code>S_IWUSR</code> and <code>S_IRUSR</code> which means that
only the current user will have read and write access to the file.
only the current user will have read and write access to the file. The third example shows another way to create a file with more restrictive
permissions if a <code>FILE *</code> stream pointer is required rather than a file descriptor.
</p>
<sample src="DoNotCreateWorldWritable.c" />

View File

@@ -0,0 +1,4 @@
---
category: queryMetadata
---
* The precision of `cpp/unsigned-difference-expression-compared-zero` ("Unsigned difference expression compared to zero") has been increased to `high`. As a result, it will be run by default as part of the Code Scanning suite.

View File

@@ -0,0 +1,4 @@
---
category: minorAnalysis
---
* The `cpp/incorrect-allocation-error-handling` ("Incorrect allocation-error handling") query no longer produces occasional false positive results inside template instantiations.

View File

@@ -0,0 +1,4 @@
---
category: minorAnalysis
---
* The `cpp/suspicious-allocation-size` ("Not enough memory allocated for array of pointer type") query no longer produces false positives on "variable size" `struct`s.

View File

@@ -0,0 +1,4 @@
---
category: minorAnalysis
---
* The `cpp/incorrectly-checked-scanf` ("Incorrect return-value check for a 'scanf'-like function") query now produces fewer false positive results.

View File

@@ -1,4 +1,5 @@
---
category: minorAnalysis
---
## 1.0.1
### Minor Analysis Improvements
* The `cpp/dangerous-function-overflow` no longer produces a false positive alert when the `gets` function does not have exactly one parameter.

View File

@@ -0,0 +1,3 @@
## 1.0.2
No user-facing changes.

View File

@@ -0,0 +1,3 @@
## 1.0.3
No user-facing changes.

View File

@@ -0,0 +1,10 @@
## 1.1.0
### Query Metadata Changes
* The precision of `cpp/iterator-to-expired-container` ("Iterator to expired container") has been increased to `high`. As a result, it will be run by default as part of the Code Scanning suite.
* The precision of `cpp/unsafe-strncat` ("Potentially unsafe call to strncat") has been increased to `high`. As a result, it will be run by default as part of the Code Scanning suite.
### Minor Analysis Improvements
* The `cpp/unsigned-difference-expression-compared-zero` ("Unsigned difference expression compared to zero") query now produces fewer false positives.

View File

@@ -1,2 +1,2 @@
---
lastReleaseVersion: 1.0.0
lastReleaseVersion: 1.1.0

View File

@@ -1,5 +1,5 @@
name: codeql/cpp-queries
version: 1.0.1-dev
version: 1.1.1-dev
groups:
- cpp
- queries