Skip to content

Commit f325003

Browse files
jketemajketema
committed
Address review comments
1 parent 0773ab3 commit f325003

6 files changed

Lines changed: 43 additions & 31 deletions

File tree

‎cpp/ql/lib/change-notes/2022-02-02-detect-creat-in-openat.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
11
---
22
category: minorAnalysis
33
---
4-
* The `cpp/world-writable-file-creation` query now only detects `openat` calls with the `O_CREAT` flag, irrespective of whether the `mode` argument is present.
4+
* The `cpp/world-writable-file-creation` query now only detects `open` and `openat` calls with both the `O_CREAT` and `O_TMPFILE` flag, irrespective of whether the `mode` argument is present.

‎cpp/ql/lib/change-notes/2022-02-02-detect-missing-mode-argument-for-open.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
11
---
22
category: newQuery
33
---
4-
* Added a new query, `open-call-with-mode-argument`, to detect when `open` or `openat` is called with the `O_CREAT` flag but when the `mode` argument is omitted.
4+
* Added a new query, `cpp/open-call-with-mode-argument`, to detect when `open` or `openat` is called with the `O_CREAT` or `O_TMPFILE` flag but when the `mode` argument is omitted.

‎cpp/ql/lib/semmle/code/cpp/commons/unix/Constants.qll‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,10 +11,10 @@ import cpp
1111
*/
1212
bindingset[input]
1313
int parseOctal(string input) {
14-
input.charAt(0) = "0" and
14+
input.regexpMatch("0[0-7]+") and
1515
result =
16-
strictsum(int ix |
17-
ix in [0 .. input.length()]
16+
sum(int ix |
17+
ix in [1 .. input.length()]
1818
|
1919
8.pow(input.length() - (ix + 1)) * input.charAt(ix).toInt()
2020
)

‎cpp/ql/src/Security/CWE/CWE-732/FilePermissions.qll‎

Lines changed: 26 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,27 +1,29 @@
11
import cpp
2-
import semmle.code.cpp.commons.unix.Constants
2+
import semmle.code.cpp.commons.unix.Constants as UnixConstants
33

44
bindingset[input]
55
int parseHex(string input) {
6-
input.matches("0x%") and
7-
result =
8-
strictsum(int ix |
9-
ix in [2 .. input.length()]
10-
|
11-
16.pow(input.length() - (ix + 1)) * "0123456789abcdef".indexOf(input.charAt(ix).toLowerCase())
12-
)
6+
exists(string lowerCaseInput | lowerCaseInput = input.toLowerCase() |
7+
lowerCaseInput.regexpMatch("0x[0-9a-f]+") and
8+
result =
9+
sum(int ix |
10+
ix in [2 .. input.length()]
11+
|
12+
16.pow(input.length() - (ix + 1)) * "0123456789abcdef".indexOf(lowerCaseInput.charAt(ix))
13+
)
14+
)
1315
}
1416

15-
int o_creat_new() {
16-
exists(Macro m | m.getName() = "O_CREAT" | result = parseHex(m.getBody()))
17-
or
18-
exists(Macro m | m.getName() = "O_CREAT" | result = parseOctal(m.getBody()))
17+
int o_creat() {
18+
exists(Macro m | m.getName() = "O_CREAT" |
19+
result = parseHex(m.getBody()) or result = UnixConstants::parseOctal(m.getBody())
20+
)
1921
}
2022

2123
int o_tmpfile() {
22-
exists(Macro m | m.getName() = "O_TMPFILE" | result = parseHex(m.getBody()))
23-
or
24-
exists(Macro m | m.getName() = "O_TMPFILE" | result = parseOctal(m.getBody()))
24+
exists(Macro m | m.getName() = "O_TMPFILE" |
25+
result = parseHex(m.getBody()) or result = UnixConstants::parseOctal(m.getBody())
26+
)
2527
}
2628

2729
bindingset[n, digit]
@@ -114,7 +116,7 @@ class OpenCreationExpr extends FileCreationWithOptionalModeExpr {
114116
OpenCreationExpr() {
115117
this.getTarget().getName() = ["open", "_open", "_wopen"] and
116118
exists(int flag | flag = this.getArgument(1).getValue().toInt() |
117-
sets(flag, o_creat_new()) or sets(flag, o_tmpfile())
119+
sets(flag, o_creat()) or sets(flag, o_tmpfile())
118120
)
119121
}
120122

@@ -143,7 +145,7 @@ class OpenatCreationExpr extends FileCreationWithOptionalModeExpr {
143145
OpenatCreationExpr() {
144146
this.getTarget().getName() = "openat" and
145147
exists(int flag | flag = this.getArgument(2).getValue().toInt() |
146-
sets(flag, o_creat_new()) or sets(flag, o_tmpfile())
148+
sets(flag, o_creat()) or sets(flag, o_tmpfile())
147149
)
148150
}
149151

@@ -162,7 +164,12 @@ class OpenatCreationExpr extends FileCreationWithOptionalModeExpr {
162164

163165
private int fopenMode() {
164166
result =
165-
s_irusr().bitOr(s_irgrp()).bitOr(s_iroth()).bitOr(s_iwusr()).bitOr(s_iwgrp()).bitOr(s_iwoth())
167+
UnixConstants::s_irusr()
168+
.bitOr(UnixConstants::s_irgrp())
169+
.bitOr(UnixConstants::s_iroth())
170+
.bitOr(UnixConstants::s_iwusr())
171+
.bitOr(UnixConstants::s_iwgrp())
172+
.bitOr(UnixConstants::s_iwoth())
166173
}
167174

168175
class FopenCreationExpr extends FileCreationExpr {
@@ -194,6 +201,6 @@ class FopensCreationExpr extends FileCreationExpr {
194201
// fopen_s has restrictive permissions unless you have "u" in the mode
195202
if this.getArgument(2).getValue().charAt(_) = "u"
196203
then result = fopenMode()
197-
else result = s_irusr().bitOr(s_iwusr())
204+
else result = UnixConstants::s_irusr().bitOr(UnixConstants::s_iwusr())
198205
}
199206
}

‎cpp/ql/test/query-tests/Security/CWE/CWE-732/OpenCallMissingModeArgument.c‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,11 @@
11
typedef unsigned int mode_t;
22

3-
#define O_APPEND 0x0020
4-
#define O_CREAT 0x0200
5-
#define O_TMPFILE 0x2000
3+
#define O_RDWR 0x0002
4+
#define O_CLOEXEC 0x0040
5+
#define O_NONBLOCK 0x0080
6+
#define O_CREAT 0x0200
7+
#define O_APPEND 0x0800
8+
#define O_TMPFILE 0x2000
69

710
int open(const char *pathname, int flags, ...);
811

@@ -11,6 +14,8 @@ int openat(int dirfd, const char *pathname, int flags, ...);
1114
const char *a_file = "/a_file";
1215

1316
void test_open() {
17+
open(a_file, O_NONBLOCK); // GOOD
18+
open(a_file, O_RDWR | O_CLOEXEC); // GOOD
1419
open(a_file, O_APPEND); // GOOD
1520
open(a_file, O_CREAT); // BAD
1621
open(a_file, O_CREAT, 0); // GOOD

‎cpp/ql/test/query-tests/Security/CWE/CWE-732/OpenCallMissingModeArgument.expected‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
| OpenCallMissingModeArgument.c:15:3:15:6 | call to open | A file is created here without providing a mode argument, which may leak bits from the stack. |
2-
| OpenCallMissingModeArgument.c:17:3:17:6 | call to open | A file is created here without providing a mode argument, which may leak bits from the stack. |
3-
| OpenCallMissingModeArgument.c:20:3:20:8 | call to openat | A file is created here without providing a mode argument, which may leak bits from the stack. |
4-
| OpenCallMissingModeArgument.c:22:3:22:8 | call to openat | A file is created here without providing a mode argument, which may leak bits from the stack. |
1+
| OpenCallMissingModeArgument.c:20:3:20:6 | call to open | A file is created here without providing a mode argument, which may leak bits from the stack. |
2+
| OpenCallMissingModeArgument.c:22:3:22:6 | call to open | A file is created here without providing a mode argument, which may leak bits from the stack. |
3+
| OpenCallMissingModeArgument.c:25:3:25:8 | call to openat | A file is created here without providing a mode argument, which may leak bits from the stack. |
4+
| OpenCallMissingModeArgument.c:27:3:27:8 | call to openat | A file is created here without providing a mode argument, which may leak bits from the stack. |

0 commit comments

Comments
 (0)