Skip to content

Commit f4598ef

Browse files
GrosQuilduGrosQuildu
committed
sa_mask FP reduced, better sigaction detection
1 parent 04cae50 commit f4598ef

File tree

3 files changed

+122
-49
lines changed

3 files changed

+122
-49
lines changed

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

Lines changed: 86 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -10,56 +10,99 @@
1010
* @group security
1111
*/
1212

13-
import cpp
13+
import cpp
1414

15-
/* List from https://man7.org/linux/man-pages/man3/stdio.3.html */
16-
class StdioFunction extends Function {
17-
StdioFunction() {
18-
this.getName() in [
19-
"fopen", "freopen", "fflush", "fclose", "fread", "fwrite", "fgetc", "fgets", "fputc",
20-
"fputs", "getc", "getchar", "gets", "putc", "putchar", "puts", "ungetc", "fprintf",
21-
"fscanf", "printf", "scanf", "sprintf", "sscanf", "vprintf", "vfprintf", "vsprintf",
22-
"fgetpos", "fsetpos", "ftell", "fseek", "rewind", "clearerr", "feof", "ferror", "perror",
23-
"setbuf", "setvbuf", "remove", "rename", "tmpfile", "tmpnam",
24-
]
25-
}
26-
}
2715

28-
/* List from https://man7.org/linux/man-pages/man3/syslog.3.html */
29-
class SyslogFunction extends Function {
30-
SyslogFunction() { this.getName() in ["syslog", "vsyslog",] }
31-
}
16+
/* List from https://man7.org/linux/man-pages/man3/stdio.3.html */
17+
class StdioFunction extends Function {
18+
StdioFunction() {
19+
this.getName() in [
20+
"fopen", "freopen", "fflush", "fclose", "fread", "fwrite", "fgetc", "fgets", "fputc",
21+
"fputs", "getc", "getchar", "gets", "putc", "putchar", "puts", "ungetc", "fprintf",
22+
"fscanf", "printf", "scanf", "sprintf", "sscanf", "vprintf", "vfprintf", "vsprintf",
23+
"fgetpos", "fsetpos", "ftell", "fseek", "rewind", "clearerr", "feof", "ferror", "perror",
24+
"setbuf", "setvbuf", "remove", "rename", "tmpfile", "tmpnam",
25+
]
26+
}
27+
}
28+
29+
/* List from https://man7.org/linux/man-pages/man3/syslog.3.html */
30+
class SyslogFunction extends Function {
31+
SyslogFunction() { this.getName() in ["syslog", "vsyslog",] }
32+
}
33+
34+
/* List from https://man7.org/linux/man-pages/man0/stdlib.h.0p.html */
35+
class StdlibFunction extends Function {
36+
StdlibFunction() { this.getName() in ["malloc", "calloc", "realloc", "free",] }
37+
}
38+
39+
predicate isAsyncUnsafe(Function signalHandler) {
40+
exists(Function f |
41+
signalHandler.calls+(f) and
42+
(
43+
f instanceof StdioFunction or
44+
f instanceof SyslogFunction or
45+
f instanceof StdlibFunction
46+
)
47+
)
48+
}
3249

33-
/* List from https://man7.org/linux/man-pages/man0/stdlib.h.0p.html */
34-
class StdlibFunction extends Function {
35-
StdlibFunction() { this.getName() in ["malloc", "calloc", "realloc", "free",] }
50+
// signal(SIGX, signalHandler)
51+
predicate isSignal(FunctionCall signalCall, Function signalHandler) {
52+
signalCall.getTarget().getName() = "signal"
53+
and signalHandler.getAnAccess() = signalCall.getArgument(1).getAChild*()
3654
}
3755

38-
predicate isAsyncUnsafe(Function signalHandler) {
39-
exists(Function f |
40-
signalHandler.calls+(f) and
41-
(
42-
f instanceof StdioFunction or
43-
f instanceof SyslogFunction or
44-
f instanceof StdlibFunction
56+
/**
57+
* struct sigaction sigactVar = ...
58+
* sigaction(SIGX, &sigactVar, ...)
59+
*/
60+
predicate isSigaction(FunctionCall sigactionCall, Function signalHandler) {
61+
exists(Variable sigactVar, Struct sigactStruct, Field handlerField |
62+
sigactionCall.getTarget().getName() = "sigaction"
63+
and sigactionCall.getArgument(1).getAChild*() = sigactVar.getAnAccess()
64+
65+
// struct sigaction with the handler func
66+
and sigactStruct.getName() = "sigaction"
67+
and handlerField.getName() = ["sa_handler", "sa_sigaction", "__sa_handler", "__sa_sigaction", "__sigaction_u"]
68+
and (
69+
handlerField = sigactStruct.getAField()
70+
or
71+
exists(Union u | u.getAField() = handlerField and u = sigactStruct.getAField().getType())
72+
)
73+
and (
74+
exists(Assignment a, ValueFieldAccess dfa |
75+
// sigactVar.sa_handler = &signalHandler
76+
a.getLValue() = dfa.getAChild*()
77+
and a.getRValue().getAChild*() = signalHandler.getAnAccess()
78+
and dfa.getTarget() = handlerField
79+
and dfa.getQualifier+() = sigactVar.getAnAccess()
80+
)
81+
or
82+
exists(VariableDeclarationEntry varDec, ClassAggregateLiteral init |
83+
// struct sigaction sigactVar = {.sa_sigaction = signalHandler};
84+
varDec.getVariable() = sigactVar
85+
and sigactVar.getInitializer().getExpr() = init
86+
and signalHandler.getAnAccess() = init.getAFieldExpr(handlerField).getAChild*()
87+
// ignore if signals are blocked via sa_mask
88+
and not exists(Field mask | mask.getName() = "sa_mask" and exists(init.getAFieldExpr(mask)))
89+
)
90+
)
91+
// ignore if signals are blocked via sa_mask
92+
and not exists(ValueFieldAccess dfa |
93+
dfa.getQualifier+() = sigactVar.getAnAccess()
94+
and dfa.getTarget().getName() = "sa_mask"
4595
)
4696
)
4797
}
48-
49-
predicate isSignalHandlerField(FieldAccess fa) {
50-
fa.getTarget().getName() in ["__sa_handler", "__sa_sigaction", "sa_handler", "sa_sigaction"]
51-
}
52-
53-
54-
from FunctionCall fc, Function signalHandler, FieldAccess fa
98+
99+
from FunctionCall fc, Function signalHandler
55100
where
56-
fc.getTarget().getName() = "signal" and
57-
signalHandler.getName() = fc.getArgument(1).toString() and
58101
isAsyncUnsafe(signalHandler)
59-
or
60-
fc.getTarget().getName() = "sigaction" and
61-
isSignalHandlerField(fa) and
62-
signalHandler = fa.getTarget().getAnAssignedValue().(AddressOfExpr).getAddressable() and
63-
isAsyncUnsafe(signalHandler)
64-
select signalHandler,
65-
"is a non-trivial signal handler that may be using functions that are not async-safe."
102+
and (
103+
isSignal(fc, signalHandler)
104+
or
105+
isSigaction(fc, signalHandler)
106+
)
107+
select signalHandler, "is a non-trivial signal handler that may be using not async-safe functions. Registered by $@", fc, fc.toString()
108+

‎cpp/test/query-tests/security/AsyncUnsafeSignalHandler/AsyncUnsafeSignalHandler.c‎

Lines changed: 32 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ void transitive_call() {
66
printf("UNSAFE");
77
}
88
void transitive_call3() {
9+
// TODO: decide which functions are exploitable
910
int *x = (int*)malloc(16);
1011
free(x);
1112
}
@@ -49,6 +50,7 @@ int main() {
4950
}
5051

5152
// Another unsafe example
53+
// TODO: decide which signals are exploitable and should produce findings
5254
struct sigaction act = { 0 };
5355
act.sa_flags = SA_SIGINFO;
5456
act.sa_sigaction = &unsafe_handler2;
@@ -57,13 +59,40 @@ int main() {
5759
_Exit(EXIT_FAILURE);
5860
}
5961

60-
struct sigaction act2 = { 0 };
61-
act2.sa_flags = SA_SIGINFO;
62-
act2.sa_handler = &unsafe_handler3;
62+
// Yet another unsafe
63+
struct sigaction act2 = {.sa_flags = SA_SIGINFO, .sa_sigaction = unsafe_handler2};
6364
if (sigaction(SIGALRM, &act2, NULL) == -1) {
6465
perror("sigaction 2");
6566
_Exit(EXIT_FAILURE);
6667
}
6768

69+
// Let's call these safe, because some signals are blocked
70+
// TODO: not every masked signals should indicate safe signal handling
71+
sigset_t sigset;
72+
sigemptyset(&sigset);
73+
sigaddset(&sigset, SIGALRM);
74+
struct sigaction act3 = {.sa_flags = SA_SIGINFO, .sa_sigaction = unsafe_handler2, .sa_mask = sigset};
75+
if (sigaction(SIGALRM, &act3, NULL) == -1) {
76+
perror("sigaction 3");
77+
_Exit(EXIT_FAILURE);
78+
}
79+
80+
struct sigaction act4 = {.sa_flags = SA_SIGINFO, .sa_sigaction = unsafe_handler2};
81+
act4.sa_mask = sigset;
82+
if (sigaction(SIGSEGV, &act4, NULL) == -1) {
83+
perror("sigaction 4");
84+
_Exit(EXIT_FAILURE);
85+
}
86+
87+
// TODO: this example should be not marked as finding
88+
struct sigaction act5 = {.sa_mask = sigset};
89+
act5.sa_flags = SA_SIGINFO;
90+
act5.sa_sigaction = &unsafe_handler2;
91+
if (sigaction(SIGSEGV, &act5, NULL) == -1) {
92+
perror("sigaction 5");
93+
_Exit(EXIT_FAILURE);
94+
}
95+
96+
6897
return 0;
6998
}

‎cpp/test/query-tests/security/AsyncUnsafeSignalHandler/AsyncUnsafeSignalHandler.expected‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1-
| AsyncUnsafeSignalHandler.c:23:6:23:19 | unsafe_handler | is a non-trivial signal handler that may be using functions that are not async-safe. |
2-
| AsyncUnsafeSignalHandler.c:27:6:27:20 | unsafe_handler2 | is a non-trivial signal handler that may be using functions that are not async-safe. |
3-
| AsyncUnsafeSignalHandler.c:34:6:34:20 | unsafe_handler3 | is a non-trivial signal handler that may be using functions that are not async-safe. |
1+
| AsyncUnsafeSignalHandler.c:24:6:24:19 | unsafe_handler | is a non-trivial signal handler that may be using not async-safe functions. Registered by $@ | AsyncUnsafeSignalHandler.c:47:9:47:14 | call to signal | call to signal |
2+
| AsyncUnsafeSignalHandler.c:28:6:28:20 | unsafe_handler2 | is a non-trivial signal handler that may be using not async-safe functions. Registered by $@ | AsyncUnsafeSignalHandler.c:57:9:57:17 | call to sigaction | call to sigaction |
3+
| AsyncUnsafeSignalHandler.c:28:6:28:20 | unsafe_handler2 | is a non-trivial signal handler that may be using not async-safe functions. Registered by $@ | AsyncUnsafeSignalHandler.c:64:9:64:17 | call to sigaction | call to sigaction |
4+
| AsyncUnsafeSignalHandler.c:28:6:28:20 | unsafe_handler2 | is a non-trivial signal handler that may be using not async-safe functions. Registered by $@ | AsyncUnsafeSignalHandler.c:91:9:91:17 | call to sigaction | call to sigaction |

0 commit comments

Comments
 (0)