Skip to content

Commit eb86767

Browse files
GrosQuilduGrosQuildu
authored
Merge branch 'main' into add-codeowners
2 parents 013cbd5 + 4a361e8 commit eb86767

File tree

18 files changed

+684
-52
lines changed

18 files changed

+684
-52
lines changed

‎.github/workflows/test.yml‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
name: CodeQL queries test
2+
on:
3+
pull_request:
4+
push:
5+
branches:
6+
- main
7+
jobs:
8+
test:
9+
name: Run CodeQL query tests
10+
runs-on: ubuntu-latest
11+
steps:
12+
- uses: actions/checkout@v4
13+
- id: init
14+
uses: github/codeql-action/init@v3
15+
- name: Run tests
16+
run: |
17+
${{ steps.init.outputs.codeql-path }} test run ./cpp/test/
18+
${{ steps.init.outputs.codeql-path }} test run ./go/test/

‎README.md‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ codeql database analyze database.db --format=sarif-latest --output=./tob.sarif -
3636
|[Memory leak related to custom allocator](./cpp/src/docs/crypto/CustomAllocatorLeak.md)|Finds memory leaks from custom allocated memory|warning|medium|
3737
|[Memory use after free related to custom allocator](./cpp/src/docs/crypto/CustomAllocatorUseAfterFree.md)|Finds use-after-frees related to custom allocators like `BN_new`|warning|medium|
3838
|[Missing OpenSSL engine initialization](./cpp/src/docs/crypto/MissingEngineInit.md)|Finds created OpenSSL engines that may not be properly initialized|warning|medium|
39-
|[Missing error handling](./cpp/src/docs/crypto/ErrorHandling.md)|Checks if returned error codes are properly checked|warning|high|
39+
|[Missing error handling](./cpp/src/docs/crypto/MissingErrorHandling.md)|Checks if returned error codes are properly checked|warning|high|
4040
|[Missing zeroization of potentially sensitive random BIGNUM](./cpp/src/docs/crypto/MissingZeroization.md)|Determines if random bignums are properly zeroized|warning|medium|
4141
|[Random buffer too small](./cpp/src/docs/crypto/RandomBufferTooSmall.md)|Finds buffer overflows in calls to CSPRNGs|warning|high|
4242
|[Use of legacy cryptographic algorithm](./cpp/src/docs/crypto/UseOfLegacyAlgorithm.md)|Detects potential instantiations of legacy cryptographic algorithms|warning|medium|
@@ -45,6 +45,7 @@ codeql database analyze database.db --format=sarif-latest --output=./tob.sarif -
4545

4646
| Name | Description | Severity | Precision |
4747
| --- | ----------- | :----: | :--------: |
48+
|[Async unsafe signal handler](./cpp/src/docs/security/AsyncUnsafeSignalHandler/AsyncUnsafeSignalHandler.md)|Async unsafe signal handler (like the one used in CVE-2024-6387)|warning|high|
4849
|[Invalid string size passed to string manipulation function](./cpp/src/docs/security/CStrnFinder/CStrnFinder.md)|Finds calls to functions that take as input a string and its size as separate arguments (e.g., `strncmp`, `strncat`, ...) and the size argument is wrong|error|low|
4950
|[Missing null terminator](./cpp/src/docs/security/NoNullTerminator/NoNullTerminator.md)|This query finds incorrectly initialized strings that are passed to functions expecting null-byte-terminated strings|error|high|
5051
|[Unsafe implicit integer conversion](./cpp/src/docs/security/UnsafeImplicitConversions/UnsafeImplicitConversions.md)|Finds implicit integer casts that may overflow or be truncated, with false positive reduction via Value Range Analysis|warning|low|

‎cpp/src/docs/security/AsyncUnsafeSignalHandler/AsyncUnsafeSignalHandler.md‎

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
# Async unsafe signal handler
2+
This is a CodeQL query constructed to find signal handlers that are performing async unsafe operations.
3+
4+
The kernel defines a list of async-safe signal functions in its [man page](https://man7.org/linux/man-pages/man7/signal-safety.7.html). Any signal handler that performs operations that are not safe asynchronously may be vulnerable.
5+
6+
7+
## Recommendation
8+
Attempt to keep signal handlers as simple as possible. Only call async-safe functions from signal handlers.
9+
10+
11+
## Example
12+
13+
```c
14+
#include <signal.h>
15+
#include <stdio.h>
16+
#include <stdlib.h>
17+
18+
enum { MAXLINE = 1024 };
19+
volatile sig_atomic_t eflag = 0;
20+
char *info = NULL;
21+
22+
void log_message(void) {
23+
fputs(info, stderr);
24+
}
25+
26+
void correct_handler(int signum) {
27+
eflag = 1;
28+
}
29+
30+
int main(void) {
31+
if (signal(SIGINT, correct_handler) == SIG_ERR) {
32+
/* Handle error */
33+
}
34+
info = (char *)malloc(MAXLINE);
35+
if (info == NULL) {
36+
/* Handle error */
37+
}
38+
39+
while (!eflag) {
40+
/* Main loop program code */
41+
42+
log_message();
43+
44+
/* More program code */
45+
}
46+
47+
log_message();
48+
free(info);
49+
info = NULL;
50+
51+
return 0;
52+
}
53+
```
54+
In this example, while both syntatically valid, a correct handler is defined in the `correct_handler` function and sets a flag. The function calls `log_message`, a async unsafe function, within the main loop.
55+
56+
57+
## References
58+
* [SEI CERT C Coding Standard "SIG30-C. Call only asynchronous-safe functions within signal handlers"](https://wiki.sei.cmu.edu/confluence/display/c/SIG30-C.+Call+only+asynchronous-safe+functions+within+signal+handlers)
59+
* [regreSSHion: RCE in OpenSSH's server, on glibc-based Linux systems](https://www.qualys.com/2024/07/01/cve-2024-6387/regresshion.txt)
60+
* [signal-safety - async-signal-safe functions](https://man7.org/linux/man-pages/man7/signal-safety.7.html)

‎cpp/src/security/AsyncUnsafeSignalHandler/AsyncUnsafeSignalHandler.c‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
#include <signal.h>
2+
#include <stdio.h>
3+
#include <stdlib.h>
4+
5+
enum { MAXLINE = 1024 };
6+
volatile sig_atomic_t eflag = 0;
7+
char *info = NULL;
8+
9+
void log_message(void) {
10+
fputs(info, stderr);
11+
}
12+
13+
void correct_handler(int signum) {
14+
eflag = 1;
15+
}
16+
17+
int main(void) {
18+
if (signal(SIGINT, correct_handler) == SIG_ERR) {
19+
/* Handle error */
20+
}
21+
info = (char *)malloc(MAXLINE);
22+
if (info == NULL) {
23+
/* Handle error */
24+
}
25+
26+
while (!eflag) {
27+
/* Main loop program code */
28+
29+
log_message();
30+
31+
/* More program code */
32+
}
33+
34+
log_message();
35+
free(info);
36+
info = NULL;
37+
38+
return 0;
39+
}

‎cpp/src/security/AsyncUnsafeSignalHandler/AsyncUnsafeSignalHandler.qhelp‎

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<overview>
6+
<p>
7+
This is a CodeQL query constructed to find signal handlers that are performing async unsafe operations.
8+
</p>
9+
10+
<p>
11+
Because a signal may be delivered at any moment, e.g., in the middle of a malloc, the handler shouldn't touch
12+
the program's internal state.
13+
</p>
14+
15+
<p>
16+
The kernel defines a list of async-safe signal functions in its <a href="https://man7.org/linux/man-pages/man7/signal-safety.7.html">man page</a>.
17+
Any signal handler that performs operations that are not safe for asynchronous execution may be vulnerable.
18+
</p>
19+
20+
<p>
21+
Moreover, signal handlers may be re-entered. Handlers' logic should take that possibility into account.
22+
</p>
23+
24+
<p>
25+
If the issue is exploitable depends on attacker's ability to deliver the signal.
26+
Remote attacks may be limitted to some signals (like SIGALRM and SIGURG), while local attacks could use all signals.
27+
</p>
28+
</overview>
29+
30+
<recommendation>
31+
<p>
32+
Attempt to keep signal handlers as simple as possible. Only call async-safe functions from signal handlers.
33+
</p>
34+
<p>
35+
Block delivery of new signals inside signal handlers to prevent handler re-entrancy issues.
36+
</p>
37+
</recommendation>
38+
39+
<example>
40+
<sample src="AsyncUnsafeSignalHandler.c" />
41+
42+
<p>
43+
In this example, while both syntatically valid, a correct handler is defined in the <code>correct_handler</code> function and sets a flag.
44+
The function calls <code>log_message</code>, a async unsafe function, within the main loop.
45+
</p>
46+
</example>
47+
48+
<references>
49+
<li>
50+
<a href="https://lcamtuf.coredump.cx/signals.txt">Michal Zalewski, "Delivering Signals for Fun and Profit"</a>
51+
</li>
52+
<li>
53+
<a href="https://wiki.sei.cmu.edu/confluence/display/c/SIG30-C.+Call+only+asynchronous-safe+functions+within+signal+handlers">SEI CERT C Coding Standard "SIG30-C. Call only asynchronous-safe functions within signal handlers"</a>
54+
</li>
55+
<li>
56+
<a href="https://www.qualys.com/2024/07/01/cve-2024-6387/regresshion.txt">regreSSHion: RCE in OpenSSH's server, on glibc-based Linux systems</a>
57+
</li>
58+
<li>
59+
<a href="https://man7.org/linux/man-pages/man7/signal-safety.7.html">signal-safety - async-signal-safe functions</a>
60+
</li>
61+
</references>
62+
63+
</qhelp>
64+

‎cpp/src/security/AsyncUnsafeSignalHandler/AsyncUnsafeSignalHandler.ql‎

Lines changed: 153 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,153 @@
1+
/**
2+
* @name Async unsafe signal handler
3+
* @id tob/cpp/async-unsafe-signal-handler
4+
* @description Async unsafe signal handler (like the one used in CVE-2024-6387)
5+
* @kind problem
6+
* @tags security
7+
* @problem.severity warning
8+
* @precision high
9+
* @security-severity 7.0
10+
* @group security
11+
*/
12+
13+
import cpp
14+
import semmle.code.cpp.dataflow.new.DataFlow
15+
16+
17+
/* List from https://man7.org/linux/man-pages/man3/stdio.3.html */
18+
class StdioFunction extends Function {
19+
StdioFunction() {
20+
this.getName() in [
21+
"fopen", "freopen", "fflush", "fclose", "fread", "fwrite", "fgetc", "fgets", "fputc",
22+
"fputs", "getc", "getchar", "gets", "putc", "putchar", "puts", "ungetc", "fprintf",
23+
"fscanf", "printf", "scanf", "sprintf", "sscanf", "vprintf", "vfprintf", "vsprintf",
24+
"fgetpos", "fsetpos", "ftell", "fseek", "rewind", "clearerr", "feof", "ferror", "perror",
25+
"setbuf", "setvbuf", "remove", "rename", "tmpfile", "tmpnam",
26+
]
27+
}
28+
}
29+
30+
/* List from https://man7.org/linux/man-pages/man3/syslog.3.html */
31+
class SyslogFunction extends Function {
32+
SyslogFunction() { this.getName() in ["syslog", "vsyslog",] }
33+
}
34+
35+
/* List from https://man7.org/linux/man-pages/man0/stdlib.h.0p.html */
36+
class StdlibFunction extends Function {
37+
StdlibFunction() { this.getName() in ["malloc", "calloc", "realloc", "free",] }
38+
}
39+
40+
predicate isAsyncUnsafe(Function signalHandler) {
41+
exists(Function f |
42+
signalHandler.calls+(f) and
43+
(
44+
f instanceof StdioFunction or
45+
f instanceof SyslogFunction or
46+
f instanceof StdlibFunction
47+
)
48+
)
49+
}
50+
51+
/**
52+
* Flows from any function ptr
53+
*/
54+
module HandlerToSignalConfiguration implements DataFlow::ConfigSig {
55+
predicate isSource(DataFlow::Node source) {
56+
source.asExpr() = any(Function f || f.getAnAccess())
57+
}
58+
59+
predicate isSink(DataFlow::Node sink) {
60+
sink = sink
61+
}
62+
}
63+
module HandlerToSignal = DataFlow::Global<HandlerToSignalConfiguration>;
64+
65+
/**
66+
* signal(SIGX, signalHandler)
67+
*/
68+
predicate isSignal(FunctionCall signalCall, Function signalHandler) {
69+
signalCall.getTarget().getName() = "signal"
70+
and exists(DataFlow::Node source, DataFlow::Node sink |
71+
HandlerToSignal::flow(source, sink)
72+
and source.asExpr() = signalHandler.getAnAccess()
73+
and sink.asExpr() = signalCall.getArgument(1)
74+
)
75+
}
76+
77+
/**
78+
* struct sigaction sigactVar = ...
79+
* sigaction(SIGX, &sigactVar, ...)
80+
*/
81+
predicate isSigaction(FunctionCall sigactionCall, Function signalHandler, Variable sigactVar) {
82+
exists(Struct sigactStruct, Field handlerField, DataFlow::Node source, DataFlow::Node sink |
83+
sigactionCall.getTarget().getName() = "sigaction"
84+
and sigactionCall.getArgument(1).getAChild*() = sigactVar.getAnAccess()
85+
86+
// struct sigaction with the handler func
87+
and sigactStruct.getName() = "sigaction"
88+
and handlerField.getName() = ["sa_handler", "sa_sigaction", "__sa_handler", "__sa_sigaction", "__sigaction_u"]
89+
and (
90+
handlerField = sigactStruct.getAField()
91+
or
92+
exists(Union u | u.getAField() = handlerField and u = sigactStruct.getAField().getType())
93+
)
94+
95+
and (
96+
// sigactVar.sa_handler = &signalHandler
97+
exists(Assignment a, ValueFieldAccess vfa |
98+
vfa.getTarget() = handlerField
99+
and vfa.getQualifier+() = sigactVar.getAnAccess()
100+
and a.getLValue() = vfa.getAChild*()
101+
102+
and source.asExpr() = signalHandler.getAnAccess()
103+
and sink.asExpr() = a.getRValue()
104+
and HandlerToSignal::flow(source, sink)
105+
)
106+
or
107+
// struct sigaction sigactVar = {.sa_sigaction = signalHandler};
108+
exists(ClassAggregateLiteral initLit |
109+
sigactVar.getInitializer().getExpr() = initLit
110+
and source.asExpr() = signalHandler.getAnAccess()
111+
and sink.asExpr() = initLit.getAFieldExpr(handlerField).getAChild*()
112+
and HandlerToSignal::flow(source, sink)
113+
)
114+
)
115+
)
116+
}
117+
118+
/**
119+
* Determine if new signals are blocked
120+
* TODO: should only find writes and only for correct (or all) signals
121+
* TODO: should detect other block mechanisms
122+
*/
123+
predicate isSignalDeliveryBlocked(Variable sigactVar) {
124+
exists(ValueFieldAccess dfa |
125+
dfa.getQualifier+() = sigactVar.getAnAccess() and dfa.getTarget().getName() = "sa_mask"
126+
)
127+
or
128+
exists(Field mask |
129+
mask.getName() = "sa_mask"
130+
and exists(sigactVar.getInitializer().getExpr().(ClassAggregateLiteral).getAFieldExpr(mask))
131+
)
132+
}
133+
134+
string deliveryNotBlockedMsg() {
135+
result = "Moreover, delivery of new signals may be not blocked. "
136+
}
137+
138+
from FunctionCall fc, Function signalHandler, string msg
139+
where
140+
isAsyncUnsafe(signalHandler)
141+
and (
142+
(isSignal(fc, signalHandler) and msg = deliveryNotBlockedMsg())
143+
or
144+
exists(Variable sigactVar |
145+
isSigaction(fc, signalHandler, sigactVar)
146+
and if isSignalDeliveryBlocked(sigactVar) then
147+
msg = ""
148+
else
149+
msg = deliveryNotBlockedMsg()
150+
)
151+
)
152+
select signalHandler, "$@ is a non-trivial signal handler that uses not async-safe functions. " + msg +
153+
"Handler is registered by $@.", signalHandler, signalHandler.toString(), fc, fc.toString()

0 commit comments

Comments
 (0)