Merge remote-tracking branch 'upstream/master' into split

This commit is contained in:
Geoffrey White
2020-08-14 14:03:34 +01:00
61 changed files with 3622 additions and 1182 deletions

View File

@@ -18,6 +18,9 @@ Expr getTest() {
or
// boost tests; http://www.boost.org/
result.(FunctionCall).getTarget().hasQualifiedName("boost::unit_test", "make_test_case")
or
// googletest tests; https://github.com/google/googletest/
result.(FunctionCall).getTarget().hasQualifiedName("testing::internal", "MakeAndRegisterTestInfo")
}
from File f, int n

View File

@@ -1,5 +1,13 @@
/**
* Provides classes for identifying and reasoning about Microsoft source code
* annotation language (SAL) macros.
*/
import cpp
/**
* A SAL macro defined in `sal.h` or a similar header file.
*/
class SALMacro extends Macro {
SALMacro() {
exists(string filename | filename = this.getFile().getBaseName() |
@@ -20,27 +28,34 @@ class SALMacro extends Macro {
}
pragma[noinline]
predicate isTopLevelMacroAccess(MacroAccess ma) { not exists(ma.getParentInvocation()) }
private predicate isTopLevelMacroAccess(MacroAccess ma) { not exists(ma.getParentInvocation()) }
/**
* An invocation of a SAL macro (excluding invocations inside other macros).
*/
class SALAnnotation extends MacroInvocation {
SALAnnotation() {
this.getMacro() instanceof SALMacro and
isTopLevelMacroAccess(this)
}
/** Returns the `Declaration` annotated by `this`. */
/** Gets the `Declaration` annotated by `this`. */
Declaration getDeclaration() {
annotatesAt(this, result.getADeclarationEntry(), _, _) and
not result instanceof Type // exclude typedefs
}
/** Returns the `DeclarationEntry` annotated by `this`. */
/** Gets the `DeclarationEntry` annotated by `this`. */
DeclarationEntry getDeclarationEntry() {
annotatesAt(this, result, _, _) and
not result instanceof TypeDeclarationEntry // exclude typedefs
}
}
/**
* A SAL macro indicating that the return value of a function should always be
* checked.
*/
class SALCheckReturn extends SALAnnotation {
SALCheckReturn() {
exists(SALMacro m | m = this.getMacro() |
@@ -50,6 +65,10 @@ class SALCheckReturn extends SALAnnotation {
}
}
/**
* A SAL macro indicating that a pointer variable or return value should not be
* `NULL`.
*/
class SALNotNull extends SALAnnotation {
SALNotNull() {
exists(SALMacro m | m = this.getMacro() |
@@ -69,6 +88,9 @@ class SALNotNull extends SALAnnotation {
}
}
/**
* A SAL macro indicating that a value may be `NULL`.
*/
class SALMaybeNull extends SALAnnotation {
SALMaybeNull() {
exists(SALMacro m | m = this.getMacro() |
@@ -79,13 +101,29 @@ class SALMaybeNull extends SALAnnotation {
}
}
/**
* A parameter annotated by one or more SAL annotations.
*/
class SALParameter extends Parameter {
/** One of this parameter's annotations. */
SALAnnotation a;
SALParameter() { annotatesAt(a, this.getADeclarationEntry(), _, _) }
predicate isIn() { a.getMacroName().toLowerCase().matches("%\\_in%") }
predicate isOut() { a.getMacroName().toLowerCase().matches("%\\_out%") }
predicate isInOut() { a.getMacroName().toLowerCase().matches("%\\_inout%") }
}
///////////////////////////////////////////////////////////////////////////////
// Implementation details
/**
* Holds if `a` annotates the declaration entry `d` and
* its start position is the `idx`th position in `file` that holds a SAL element.
*/
predicate annotatesAt(SALAnnotation a, DeclarationEntry d, File file, int idx) {
private predicate annotatesAt(SALAnnotation a, DeclarationEntry d, File file, int idx) {
annotatesAtPosition(a.(SALElement).getStartPosition(), d, file, idx)
}
@@ -109,22 +147,6 @@ private predicate annotatesAtPosition(SALPosition pos, DeclarationEntry d, File
)
}
/**
* A parameter annotated by one or more SAL annotations.
*/
class SALParameter extends Parameter {
/** One of this parameter's annotations. */
SALAnnotation a;
SALParameter() { annotatesAt(a, this.getADeclarationEntry(), _, _) }
predicate isIn() { a.getMacroName().toLowerCase().matches("%\\_in%") }
predicate isOut() { a.getMacroName().toLowerCase().matches("%\\_out%") }
predicate isInOut() { a.getMacroName().toLowerCase().matches("%\\_inout%") }
}
/**
* A SAL element, that is, a SAL annotation or a declaration entry
* that may have SAL annotations.

View File

@@ -0,0 +1,32 @@
<!DOCTYPE qhelp PUBLIC
"-//Semmle//qhelp//EN"
"qhelp.dtd">
<qhelp>
<overview>
<p>Private data that is stored in a file or buffer unencrypted is accessible to an attacker who gains access to the
storage.</p>
</overview>
<recommendation>
<p>Ensure that private data is always encrypted before being stored.
It may be wise to encrypt information before it is put into a buffer that may be readable in memory.</p>
<p>In general, decrypt private data only at the point where it is necessary for it to be used in
cleartext.</p>
</recommendation>
<references>
<li><a href="https://owasp.org/www-project-top-ten/OWASP_Top_Ten_2017/Top_10-2017_A3-Sensitive_Data_Exposure">OWASP Sensitive_Data_Exposure</a></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>
<!-- LocalWords: CWE
-->
</references>
</qhelp>

View File

@@ -0,0 +1,21 @@
/**
* @name Exposure of private information
* @description If private information is written to an external location, it may be accessible by
* unauthorized persons.
* @kind path-problem
* @problem.severity error
* @id cpp/private-cleartext-write
* @tags security
* external/cwe/cwe-359
*/
import cpp
import experimental.semmle.code.cpp.security.PrivateCleartextWrite
import experimental.semmle.code.cpp.security.PrivateCleartextWrite::PrivateCleartextWrite
import DataFlow::PathGraph
from WriteConfig b, DataFlow::PathNode source, DataFlow::PathNode sink
where b.hasFlowPath(source, sink)
select sink.getNode(),
"This write into the external location '" + sink + "' may contain unencrypted data from $@",
source, "this source."

View File

@@ -0,0 +1,63 @@
/**
* Provides a taint-tracking configuration for reasoning about private information flowing unencrypted to an external location.
*/
import cpp
import semmle.code.cpp.dataflow.TaintTracking
import experimental.semmle.code.cpp.security.PrivateData
import semmle.code.cpp.security.FileWrite
import semmle.code.cpp.security.BufferWrite
import semmle.code.cpp.dataflow.TaintTracking
module PrivateCleartextWrite {
/**
* A data flow source for private information flowing unencrypted to an external location.
*/
abstract class Source extends DataFlow::ExprNode { }
/**
* A data flow sink for private information flowing unencrypted to an external location.
*/
abstract class Sink extends DataFlow::ExprNode { }
/**
* A sanitizer for private information flowing unencrypted to an external location.
*/
abstract class Sanitizer extends DataFlow::ExprNode { }
/** A call to any method whose name suggests that it encodes or encrypts the parameter. */
class ProtectSanitizer extends Sanitizer {
ProtectSanitizer() {
exists(Function m, string s |
this.getExpr().(FunctionCall).getTarget() = m and
m.getName().regexpMatch("(?i).*" + s + ".*")
|
s = "protect" or s = "encode" or s = "encrypt"
)
}
}
class WriteConfig extends TaintTracking::Configuration {
WriteConfig() { this = "Write configuration" }
override predicate isSource(DataFlow::Node source) { source instanceof Source }
override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer }
}
class PrivateDataSource extends Source {
PrivateDataSource() { this.getExpr() instanceof PrivateDataExpr }
}
class WriteSink extends Sink {
WriteSink() {
exists(FileWrite f, BufferWrite b |
this.asExpr() = f.getASource()
or
this.asExpr() = b.getAChild()
)
}
}
}

View File

@@ -0,0 +1,53 @@
/**
* Provides classes and predicates for identifying private data and functions for security.
*
* 'Private' data in general is anything that would compromise user privacy if exposed. This
* library tries to guess where private data may either be stored in a variable or produced by a
* function.
*
* This library is not concerned with credentials. See `SensitiveActions` for expressions related
* to credentials.
*/
import cpp
/** A string for `match` that identifies strings that look like they represent private data. */
private string privateNames() {
// Inspired by the list on https://cwe.mitre.org/data/definitions/359.html
// Government identifiers, such as Social Security Numbers
result = "%social%security%number%" or
// Contact information, such as home addresses and telephone numbers
result = "%postcode%" or
result = "%zipcode%" or
// result = "%telephone%" or
// Geographic location - where the user is (or was)
result = "%latitude%" or
result = "%longitude%" or
// Financial data - such as credit card numbers, salary, bank accounts, and debts
result = "%creditcard%" or
result = "%salary%" or
result = "%bankaccount%" or
// Communications - e-mail addresses, private e-mail messages, SMS text messages, chat logs, etc.
// result = "%email%" or
// result = "%mobile%" or
result = "%employer%" or
// Health - medical conditions, insurance status, prescription records
result = "%medical%"
}
/** An expression that might contain private data. */
abstract class PrivateDataExpr extends Expr { }
/** A functiond call that might produce private data. */
class PrivateFunctionCall extends PrivateDataExpr, FunctionCall {
PrivateFunctionCall() {
exists(string s | this.getTarget().getName().toLowerCase() = s | s.matches(privateNames()))
}
}
/** An access to a variable that might contain private data. */
class PrivateVariableAccess extends PrivateDataExpr, VariableAccess {
PrivateVariableAccess() {
exists(string s | this.getTarget().getName().toLowerCase() = s | s.matches(privateNames()))
}
}

View File

@@ -156,6 +156,10 @@ float safeFloor(float v) {
result = v
}
private class UnsignedMulExpr extends MulExpr {
UnsignedMulExpr() { this.getType().(IntegralType).isUnsigned() }
}
/** Set of expressions which we know how to analyze. */
private predicate analyzableExpr(Expr e) {
// The type of the expression must be arithmetic. We reuse the logic in
@@ -178,6 +182,8 @@ private predicate analyzableExpr(Expr e) {
or
e instanceof SubExpr
or
e instanceof UnsignedMulExpr
or
e instanceof AssignExpr
or
e instanceof AssignAddExpr
@@ -278,6 +284,10 @@ private predicate exprDependsOnDef(Expr e, RangeSsaDefinition srcDef, StackVaria
or
exists(SubExpr subExpr | e = subExpr | exprDependsOnDef(subExpr.getAnOperand(), srcDef, srcVar))
or
exists(UnsignedMulExpr mulExpr | e = mulExpr |
exprDependsOnDef(mulExpr.getAnOperand(), srcDef, srcVar)
)
or
exists(AssignExpr addExpr | e = addExpr | exprDependsOnDef(addExpr.getRValue(), srcDef, srcVar))
or
exists(AssignAddExpr addExpr | e = addExpr |
@@ -625,6 +635,13 @@ private float getLowerBoundsImpl(Expr expr) {
result = addRoundingDown(xLow, -yHigh)
)
or
exists(UnsignedMulExpr mulExpr, float xLow, float yLow |
expr = mulExpr and
xLow = getFullyConvertedLowerBounds(mulExpr.getLeftOperand()) and
yLow = getFullyConvertedLowerBounds(mulExpr.getRightOperand()) and
result = xLow * yLow
)
or
exists(AssignExpr assign |
expr = assign and
result = getFullyConvertedLowerBounds(assign.getRValue())
@@ -794,6 +811,13 @@ private float getUpperBoundsImpl(Expr expr) {
result = addRoundingUp(xHigh, -yLow)
)
or
exists(UnsignedMulExpr mulExpr, float xHigh, float yHigh |
expr = mulExpr and
xHigh = getFullyConvertedUpperBounds(mulExpr.getLeftOperand()) and
yHigh = getFullyConvertedUpperBounds(mulExpr.getRightOperand()) and
result = xHigh * yHigh
)
or
exists(AssignExpr assign |
expr = assign and
result = getFullyConvertedUpperBounds(assign.getRValue())

View File

@@ -0,0 +1,21 @@
edges
| test.cpp:77:16:77:22 | medical | test.cpp:78:24:78:27 | temp |
| test.cpp:81:17:81:20 | call to func | test.cpp:82:24:82:28 | buff5 |
| test.cpp:81:22:81:28 | medical | test.cpp:81:17:81:20 | call to func |
nodes
| test.cpp:57:9:57:18 | theZipcode | semmle.label | theZipcode |
| test.cpp:74:24:74:30 | medical | semmle.label | medical |
| test.cpp:77:16:77:22 | medical | semmle.label | medical |
| test.cpp:78:24:78:27 | temp | semmle.label | temp |
| test.cpp:81:17:81:20 | call to func | semmle.label | call to func |
| test.cpp:81:22:81:28 | medical | semmle.label | medical |
| test.cpp:82:24:82:28 | buff5 | semmle.label | buff5 |
| test.cpp:96:37:96:46 | theZipcode | semmle.label | theZipcode |
| test.cpp:99:42:99:51 | theZipcode | semmle.label | theZipcode |
#select
| test.cpp:57:9:57:18 | theZipcode | This write into the external location 'theZipcode' may contain unencrypted data from $@ | test.cpp:57:9:57:18 | theZipcode | this source. |
| test.cpp:74:24:74:30 | medical | This write into the external location 'medical' may contain unencrypted data from $@ | test.cpp:74:24:74:30 | medical | this source. |
| test.cpp:78:24:78:27 | temp | This write into the external location 'temp' may contain unencrypted data from $@ | test.cpp:77:16:77:22 | medical | this source. |
| test.cpp:82:24:82:28 | buff5 | This write into the external location 'buff5' may contain unencrypted data from $@ | test.cpp:81:22:81:28 | medical | this source. |
| test.cpp:96:37:96:46 | theZipcode | This write into the external location 'theZipcode' may contain unencrypted data from $@ | test.cpp:96:37:96:46 | theZipcode | this source. |
| test.cpp:99:42:99:51 | theZipcode | This write into the external location 'theZipcode' may contain unencrypted data from $@ | test.cpp:99:42:99:51 | theZipcode | this source. |

View File

@@ -0,0 +1 @@
experimental/Security/CWE/CWE-359/PrivateCleartextWrite.ql

View File

@@ -0,0 +1,105 @@
#define FILE int
#define wchar char
#define size_t int
typedef int streamsize;
size_t fwrite(const void *ptr, size_t size, size_t nmemb, FILE *stream);
int fputs(const char *s, FILE *stream);
int fputc(int c, FILE *stream);
int fprintf(FILE *stream, const char *format, ...);
int sprintf(char *s, const char *format, ...);
size_t strlen(const char *s);
namespace std
{
template <class charT>
struct char_traits;
template <class charT, class traits = char_traits<charT>>
class basic_ostream /*: virtual public basic_ios<charT,traits> - not needed for this test */
{
public:
typedef charT char_type;
basic_ostream<charT, traits> &write(const char_type *s, streamsize n);
};
template <class charT, class traits = char_traits<charT>>
class basic_ofstream : public basic_ostream<charT, traits>
{
public:
};
template <class charT, class traits>
basic_ostream<charT, traits> &operator<<(basic_ostream<charT, traits> &, const charT *);
typedef basic_ostream<char> ostream;
typedef basic_ofstream<char> ofstream;
}; // namespace std
using namespace std;
char *encrypt(char *buffer)
{
return buffer;
}
char *func(char *buffer)
{
return buffer;
}
// test for CleartextFileWrite
void file()
{
char *theZipcode = "cleartext zipcode!";
FILE *file;
// BAD: write zipcode to file in cleartext
fputs(theZipcode, file);
// GOOD: encrypt first
char *encrypted = encrypt(theZipcode);
fwrite(encrypted, sizeof(encrypted), 1, file);
}
// test for CleartextBufferWrite
int main(int argc, char **argv)
{
char *medical = "medical";
char *buff1;
char *buff2;
char *buff3;
char *buff4;
// BAD: write medical to buffer in cleartext
sprintf(buff1, "%s", medical);
// BAD: write medical to buffer in cleartext
char *temp = medical;
sprintf(buff2, "%s", temp);
// BAD: write medical to buffer in cleartext
char *buff5 = func(medical);
sprintf(buff3, "%s", buff5);
char *buff6 = encrypt(medical);
// GOOD: encrypt first
sprintf(buff4, "%s", buff6);
}
// test for CleartextFileWrite
void stream()
{
char *theZipcode = "cleartext zipcode!";
ofstream mystream;
// BAD: write zipcode to file in cleartext
mystream << "the zipcode is: " << theZipcode;
// BAD: write zipcode to file in cleartext
(mystream << "the zipcode is: ").write(theZipcode, strlen(theZipcode));
// GOOD: encrypt first
char *encrypted = encrypt(theZipcode);
mystream << "the zipcode is: " << encrypted;
(mystream << "the zipcode is: ").write(encrypted, strlen(encrypted));
}

View File

@@ -1,4 +1,4 @@
import semmle.code.cpp.rangeanalysis.SimpleRangeAnalysis
from VariableAccess expr
select expr, lowerBound(expr)
select expr, lowerBound(expr).toString()

View File

@@ -463,3 +463,30 @@ int test_unsigned_mult02(unsigned b) {
return total;
}
unsigned long mult_rounding() {
unsigned long x, y, xy;
x = y = 1000000003UL; // 1e9 + 3
xy = x * y;
return xy; // BUG: upper bound should be >= 1000000006000000009UL
}
unsigned long mult_overflow() {
unsigned long x, y, xy;
x = 274177UL;
y = 67280421310721UL;
xy = x * y;
return xy; // BUG: lower bound should be <= 18446744073709551617UL
}
unsigned long mult_lower_bound(unsigned int ui, unsigned long ul) {
if (ui >= 10) {
unsigned long result = (unsigned long)ui * ui;
return result; // BUG: upper bound should be >= 18446744065119617025 (possibly a pretty-printing bug)
}
if (ul >= 10) {
unsigned long result = ul * ul;
return result; // lower bound is correctly 0 (overflow is possible)
}
return 0;
}

View File

@@ -1,4 +1,4 @@
import semmle.code.cpp.rangeanalysis.SimpleRangeAnalysis
from VariableAccess expr
select expr, upperBound(expr)
select expr, upperBound(expr).toString()

View File

@@ -385,10 +385,39 @@ void bitwise_ands()
void unsigned_mult(unsigned int x, unsigned int y) {
if(x < 13 && y < 35) {
if(x * y > 1024) {} // always false [NOT DETECTED]
if(x * y > 1024) {} // always false
if(x * y < 204) {}
if(x >= 3 && y >= 2) {
if(x * y < 5) {} // always false [NOT DETECTED]
if(x * y < 5) {} // always false
}
}
}
}
void mult_rounding() {
unsigned long x, y, xy;
x = y = 1000000003UL; // 1e9 + 3
xy = 1000000006000000009UL; // x * y, precisely
// Even though the range analysis wrongly considers x*y to be xy - 9, there
// are no PointlessComparison false positives in these tests because alerts
// are suppressed when ulp() < 1, which roughly means that the number is
// larger than 2^53.
if (x * y < xy) {} // always false [NOT DETECTED]
if (x * y > xy) {} // always false [NOT DETECTED]
}
void mult_overflow() {
unsigned long x, y;
// The following two numbers multiply to 2^64 + 1, which is 1 when truncated
// to 64-bit unsigned.
x = 274177UL;
y = 67280421310721UL;
if (x * y == 1) {} // always true [BUG: reported as always false]
// This bug appears to be caused by
// `RangeAnalysisUtils::typeUpperBound(unsigned long)` having a result of
// 2**64 + 384, making the range analysis think that the multiplication can't
// overflow. The correct `typeUpperBound` would be 2**64 - 1, but we can't
// represent that with a QL float or int. We could make `typeUpperBound`
// exclusive instead of inclusive, but there is no exclusive upper bound for
// floats.
}

View File

@@ -41,6 +41,9 @@
| PointlessComparison.c:372:6:372:16 | ... >= ... | Comparison is always true because ... >> ... >= 1. |
| PointlessComparison.c:373:6:373:16 | ... >= ... | Comparison is always false because ... >> ... <= 1. |
| PointlessComparison.c:383:6:383:17 | ... >= ... | Comparison is always false because ... & ... <= 2. |
| PointlessComparison.c:388:10:388:21 | ... > ... | Comparison is always false because ... * ... <= 408. |
| PointlessComparison.c:391:12:391:20 | ... < ... | Comparison is always false because ... * ... >= 6. |
| PointlessComparison.c:414:7:414:16 | ... == ... | Comparison is always false because ... * ... >= 18446744073709552000. |
| PointlessComparison.cpp:36:6:36:33 | ... >= ... | Comparison is always false because ... >> ... <= 9223372036854776000. |
| PointlessComparison.cpp:41:6:41:29 | ... >= ... | Comparison is always false because ... >> ... <= 140737488355327.5. |
| PointlessComparison.cpp:42:6:42:29 | ... >= ... | Comparison is always false because ... >> ... <= 140737488355327.5. |