Skip to content

Commit d374935

Browse files
geoffw0geoffw0
authored
Merge pull request #17384 from microsoft/brodes/overflow-buffer-fixes-upstream
Brodes/overflow buffer fixes upstream
2 parents 4c8aec0 + 58779e1 commit d374935

File tree

7 files changed

+63
-17
lines changed

7 files changed

+63
-17
lines changed

‎cpp/ql/lib/semmle/code/cpp/commons/Buffer.qll

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ private int isSource(Expr bufferExpr, Element why) {
5757
exists(Type bufferType |
5858
// buffer is the address of a variable
5959
why = bufferExpr.(AddressOfExpr).getAddressable() and
60-
bufferType = why.(Variable).getType() and
60+
bufferType = why.(Variable).getUnspecifiedType() and
6161
result = bufferType.getSize() and
6262
not bufferType instanceof ReferenceType and
6363
not any(Union u).getAMemberVariable() = why

‎cpp/ql/lib/semmle/code/cpp/security/BufferAccess.qll

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,11 @@ int getPointedSize(Type t) {
1414
* BufferWrite differ.
1515
*/
1616
abstract class BufferAccess extends Expr {
17-
BufferAccess() { not this.isUnevaluated() }
17+
BufferAccess() {
18+
not this.isUnevaluated() and
19+
//A buffer access must be reachable (not in dead code)
20+
reachable(this)
21+
}
1822

1923
abstract string getName();
2024

@@ -26,6 +30,8 @@ abstract class BufferAccess extends Expr {
2630
* - 1 = buffer range [0, getSize) is accessed entirely.
2731
* - 2 = buffer range [0, getSize) may be accessed partially or entirely.
2832
* - 3 = buffer is accessed at offset getSize - 1.
33+
* - 4 = buffer is accessed with null terminator read protections
34+
* (does not read past null terminator, regardless of access size)
2935
*/
3036
abstract Expr getBuffer(string bufferDesc, int accessType);
3137

@@ -128,7 +134,7 @@ class StrncpyBA extends BufferAccess {
128134
or
129135
result = this.(FunctionCall).getArgument(1) and
130136
bufferDesc = "source buffer" and
131-
accessType = 2
137+
accessType = 4
132138
}
133139

134140
override Expr getSizeExpr() { result = this.(FunctionCall).getArgument(2) }

‎cpp/ql/src/Critical/OverflowStatic.ql

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,9 @@ class BufferAccess extends ArrayExpr {
4242
not exists(Macro m |
4343
m.getName() = "strcmp" and
4444
m.getAnInvocation().getAnExpandedElement() = this
45-
)
45+
) and
46+
//A buffer access must be reachable (not in dead code)
47+
reachable(this)
4648
}
4749

4850
int bufferSize() { staticBuffer(this.getArrayBase(), _, result) }

‎cpp/ql/src/Security/CWE/CWE-119/OverflowBuffer.ql

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ from
2626
BufferAccess ba, string bufferDesc, int accessSize, int accessType, Element bufferAlloc,
2727
int bufferSize, string message
2828
where
29+
accessType != 4 and
2930
accessSize = ba.getSize() and
3031
bufferSize = getBufferSize(ba.getBuffer(bufferDesc, accessType), bufferAlloc) and
3132
(

‎cpp/ql/src/change-notes/2024-09-04-overflow-buffer-false-positives.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* Removed false positives caused by buffer accesses in unreachable code
5+
* Removed false positives caused by inconsistent type checking

‎cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/UnboundedWrite.expected

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -31,9 +31,9 @@ edges
3131
| main.cpp:9:29:9:32 | *argv | tests_restrict.c:15:41:15:44 | *argv | provenance | |
3232
| main.cpp:9:29:9:32 | tests_restrict_main output argument | main.cpp:10:20:10:23 | **argv | provenance | |
3333
| main.cpp:9:29:9:32 | tests_restrict_main output argument | main.cpp:10:20:10:23 | *argv | provenance | |
34-
| main.cpp:10:20:10:23 | **argv | tests.cpp:657:32:657:35 | **argv | provenance | |
35-
| main.cpp:10:20:10:23 | *argv | tests.cpp:657:32:657:35 | **argv | provenance | |
36-
| main.cpp:10:20:10:23 | *argv | tests.cpp:657:32:657:35 | *argv | provenance | |
34+
| main.cpp:10:20:10:23 | **argv | tests.cpp:689:32:689:35 | **argv | provenance | |
35+
| main.cpp:10:20:10:23 | *argv | tests.cpp:689:32:689:35 | **argv | provenance | |
36+
| main.cpp:10:20:10:23 | *argv | tests.cpp:689:32:689:35 | *argv | provenance | |
3737
| overflowdestination.cpp:23:45:23:48 | **argv | overflowdestination.cpp:23:45:23:48 | **argv | provenance | |
3838
| overflowdestination.cpp:23:45:23:48 | **argv | overflowdestination.cpp:23:45:23:48 | *argv | provenance | |
3939
| test_buffer_overrun.cpp:32:46:32:49 | **argv | test_buffer_overrun.cpp:32:46:32:49 | **argv | provenance | |
@@ -46,12 +46,12 @@ edges
4646
| tests.cpp:628:14:628:14 | *s [*home] | tests.cpp:628:14:628:19 | *home | provenance | |
4747
| tests.cpp:628:14:628:14 | *s [*home] | tests.cpp:628:16:628:19 | *home | provenance | |
4848
| tests.cpp:628:16:628:19 | *home | tests.cpp:628:14:628:19 | *home | provenance | |
49-
| tests.cpp:657:32:657:35 | **argv | tests.cpp:682:9:682:15 | *access to array | provenance | |
50-
| tests.cpp:657:32:657:35 | **argv | tests.cpp:683:9:683:15 | *access to array | provenance | |
51-
| tests.cpp:657:32:657:35 | *argv | tests.cpp:682:9:682:15 | *access to array | provenance | |
52-
| tests.cpp:657:32:657:35 | *argv | tests.cpp:683:9:683:15 | *access to array | provenance | |
53-
| tests.cpp:682:9:682:15 | *access to array | tests.cpp:613:19:613:24 | *source | provenance | |
54-
| tests.cpp:683:9:683:15 | *access to array | tests.cpp:622:19:622:24 | *source | provenance | |
49+
| tests.cpp:689:32:689:35 | **argv | tests.cpp:714:9:714:15 | *access to array | provenance | |
50+
| tests.cpp:689:32:689:35 | **argv | tests.cpp:715:9:715:15 | *access to array | provenance | |
51+
| tests.cpp:689:32:689:35 | *argv | tests.cpp:714:9:714:15 | *access to array | provenance | |
52+
| tests.cpp:689:32:689:35 | *argv | tests.cpp:715:9:715:15 | *access to array | provenance | |
53+
| tests.cpp:714:9:714:15 | *access to array | tests.cpp:613:19:613:24 | *source | provenance | |
54+
| tests.cpp:715:9:715:15 | *access to array | tests.cpp:622:19:622:24 | *source | provenance | |
5555
| tests_restrict.c:15:41:15:44 | **argv | tests_restrict.c:15:41:15:44 | **argv | provenance | |
5656
| tests_restrict.c:15:41:15:44 | *argv | tests_restrict.c:15:41:15:44 | *argv | provenance | |
5757
nodes
@@ -85,10 +85,10 @@ nodes
8585
| tests.cpp:628:14:628:14 | *s [*home] | semmle.label | *s [*home] |
8686
| tests.cpp:628:14:628:19 | *home | semmle.label | *home |
8787
| tests.cpp:628:16:628:19 | *home | semmle.label | *home |
88-
| tests.cpp:657:32:657:35 | **argv | semmle.label | **argv |
89-
| tests.cpp:657:32:657:35 | *argv | semmle.label | *argv |
90-
| tests.cpp:682:9:682:15 | *access to array | semmle.label | *access to array |
91-
| tests.cpp:683:9:683:15 | *access to array | semmle.label | *access to array |
88+
| tests.cpp:689:32:689:35 | **argv | semmle.label | **argv |
89+
| tests.cpp:689:32:689:35 | *argv | semmle.label | *argv |
90+
| tests.cpp:714:9:714:15 | *access to array | semmle.label | *access to array |
91+
| tests.cpp:715:9:715:15 | *access to array | semmle.label | *access to array |
9292
| tests_restrict.c:15:41:15:44 | **argv | semmle.label | **argv |
9393
| tests_restrict.c:15:41:15:44 | **argv | semmle.label | **argv |
9494
| tests_restrict.c:15:41:15:44 | *argv | semmle.label | *argv |

‎cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/tests.cpp

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -654,6 +654,38 @@ void test26(bool cond)
654654
if (ptr[-1] == 0) { return; } // GOOD: accesses buffer[1]
655655
}
656656

657+
#define IND 100
658+
#define MAX_SIZE 100
659+
void test27(){
660+
char *src = "";
661+
char *dest = "abcdefgh";
662+
int ind = 100;
663+
char buffer[MAX_SIZE];
664+
665+
strncpy(dest, src, 8); // GOOD, strncpy will not read past null terminator of source
666+
667+
if(IND < MAX_SIZE){
668+
buffer[IND] = 0; // GOOD: out of bounds, but inaccessible code
669+
}
670+
}
671+
672+
typedef struct _MYSTRUCT {
673+
unsigned long a;
674+
unsigned short b;
675+
unsigned char z[ 100 ];
676+
} MYSTRUCT;
677+
678+
679+
const MYSTRUCT _myStruct = { 0 };
680+
typedef const MYSTRUCT& MYSTRUCTREF;
681+
682+
// False positive case due to use of typedefs
683+
int test28(MYSTRUCTREF g)
684+
{
685+
return memcmp(&g, &_myStruct, sizeof(MYSTRUCT)); // GOOD
686+
}
687+
688+
657689
int tests_main(int argc, char *argv[])
658690
{
659691
long long arr17[19];

0 commit comments

Comments
 (0)