diff --git a/cpp/src/security/AsyncUnsafeSignalHandler/AsyncUnsafeSignalHandler.qhelp b/cpp/src/security/AsyncUnsafeSignalHandler/AsyncUnsafeSignalHandler.qhelp
index cc1130f..3394cf5 100644
--- a/cpp/src/security/AsyncUnsafeSignalHandler/AsyncUnsafeSignalHandler.qhelp
+++ b/cpp/src/security/AsyncUnsafeSignalHandler/AsyncUnsafeSignalHandler.qhelp
@@ -7,9 +7,23 @@
This is a CodeQL query constructed to find signal handlers that are performing async unsafe operations.
+
+ Because a signal may be delivered at any moment, e.g., in the middle of a malloc, the handler shouldn't touch
+ the program's internal state.
+
+
The kernel defines a list of async-safe signal functions in its man page.
- Any signal handler that performs operations that are not safe asynchronously may be vulnerable.
+ Any signal handler that performs operations that are not safe for asynchronous execution may be vulnerable.
+
+
+
+ Moreover, signal handlers may be re-entered. Handlers' logic should take that possibility into account.
+
+
+
+ If the issue is exploitable depends on attacker's ability to deliver the signal.
+ Remote attacks may be limitted to some signals (like SIGALRM and SIGURG), while local attacks could use all signals.
@@ -17,17 +31,24 @@
Attempt to keep signal handlers as simple as possible. Only call async-safe functions from signal handlers.
+
+Block delivery of new signals inside signal handlers to prevent handler re-entrancy issues.
+
- 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.
+ 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.
+
+ Michal Zalewski, "Delivering Signals for Fun and Profit"
+
SEI CERT C Coding Standard "SIG30-C. Call only asynchronous-safe functions within signal handlers"
diff --git a/cpp/src/security/AsyncUnsafeSignalHandler/AsyncUnsafeSignalHandler.ql b/cpp/src/security/AsyncUnsafeSignalHandler/AsyncUnsafeSignalHandler.ql
index 9ee350f..ae5d5b1 100644
--- a/cpp/src/security/AsyncUnsafeSignalHandler/AsyncUnsafeSignalHandler.ql
+++ b/cpp/src/security/AsyncUnsafeSignalHandler/AsyncUnsafeSignalHandler.ql
@@ -11,6 +11,8 @@
*/
import cpp
+import semmle.code.cpp.dataflow.new.DataFlow
+
/* List from https://man7.org/linux/man-pages/man3/stdio.3.html */
class StdioFunction extends Function {
@@ -46,22 +48,106 @@ predicate isAsyncUnsafe(Function signalHandler) {
)
}
-predicate isSignalHandlerField(FieldAccess fa) {
- fa.getTarget().getName() in ["__sa_handler", "__sa_sigaction", "sa_handler", "sa_sigaction"]
+/**
+ * Flows from any function ptr
+ */
+module HandlerToSignalConfiguration implements DataFlow::ConfigSig {
+ predicate isSource(DataFlow::Node source) {
+ source.asExpr() = any(Function f || f.getAnAccess())
+ }
+
+ predicate isSink(DataFlow::Node sink) {
+ sink = sink
+ }
}
+module HandlerToSignal = DataFlow::Global;
-from FunctionCall fc, Function signalHandler, FieldAccess fa
-where
- (
- fc.getTarget().getName().matches("%_signal") or
- fc.getTarget().getName() = "signal"
- ) and
- signalHandler.getName() = fc.getArgument(1).toString() and
- isAsyncUnsafe(signalHandler)
+/**
+ * signal(SIGX, signalHandler)
+ */
+predicate isSignal(FunctionCall signalCall, Function signalHandler) {
+ signalCall.getTarget().getName() = "signal"
+ and exists(DataFlow::Node source, DataFlow::Node sink |
+ HandlerToSignal::flow(source, sink)
+ and source.asExpr() = signalHandler.getAnAccess()
+ and sink.asExpr() = signalCall.getArgument(1)
+ )
+}
+
+/**
+ * struct sigaction sigactVar = ...
+ * sigaction(SIGX, &sigactVar, ...)
+ */
+predicate isSigaction(FunctionCall sigactionCall, Function signalHandler, Variable sigactVar) {
+ exists(Struct sigactStruct, Field handlerField, DataFlow::Node source, DataFlow::Node sink |
+ sigactionCall.getTarget().getName() = "sigaction"
+ and sigactionCall.getArgument(1).getAChild*() = sigactVar.getAnAccess()
+
+ // struct sigaction with the handler func
+ and sigactStruct.getName() = "sigaction"
+ and handlerField.getName() = ["sa_handler", "sa_sigaction", "__sa_handler", "__sa_sigaction", "__sigaction_u"]
+ and (
+ handlerField = sigactStruct.getAField()
+ or
+ exists(Union u | u.getAField() = handlerField and u = sigactStruct.getAField().getType())
+ )
+
+ and (
+ // sigactVar.sa_handler = &signalHandler
+ exists(Assignment a, ValueFieldAccess vfa |
+ vfa.getTarget() = handlerField
+ and vfa.getQualifier+() = sigactVar.getAnAccess()
+ and a.getLValue() = vfa.getAChild*()
+
+ and source.asExpr() = signalHandler.getAnAccess()
+ and sink.asExpr() = a.getRValue()
+ and HandlerToSignal::flow(source, sink)
+ )
+ or
+ // struct sigaction sigactVar = {.sa_sigaction = signalHandler};
+ exists(ClassAggregateLiteral initLit |
+ sigactVar.getInitializer().getExpr() = initLit
+ and source.asExpr() = signalHandler.getAnAccess()
+ and sink.asExpr() = initLit.getAFieldExpr(handlerField).getAChild*()
+ and HandlerToSignal::flow(source, sink)
+ )
+ )
+ )
+}
+
+/**
+ * Determine if new signals are blocked
+ * TODO: should only find writes and only for correct (or all) signals
+ * TODO: should detect other block mechanisms
+ */
+predicate isSignalDeliveryBlocked(Variable sigactVar) {
+ exists(ValueFieldAccess dfa |
+ dfa.getQualifier+() = sigactVar.getAnAccess() and dfa.getTarget().getName() = "sa_mask"
+ )
or
- fc.getTarget().getName() = "sigaction" and
- isSignalHandlerField(fa) and
- signalHandler = fa.getTarget().getAnAssignedValue().(AddressOfExpr).getAddressable() and
+ exists(Field mask |
+ mask.getName() = "sa_mask"
+ and exists(sigactVar.getInitializer().getExpr().(ClassAggregateLiteral).getAFieldExpr(mask))
+ )
+}
+
+string deliveryNotBlockedMsg() {
+ result = "Moreover, delivery of new signals may be not blocked. "
+}
+
+from FunctionCall fc, Function signalHandler, string msg
+where
isAsyncUnsafe(signalHandler)
-select signalHandler,
- "is a non-trivial signal handler that may be using functions that are not async-safe."
+ and (
+ (isSignal(fc, signalHandler) and msg = deliveryNotBlockedMsg())
+ or
+ exists(Variable sigactVar |
+ isSigaction(fc, signalHandler, sigactVar)
+ and if isSignalDeliveryBlocked(sigactVar) then
+ msg = ""
+ else
+ msg = deliveryNotBlockedMsg()
+ )
+ )
+select signalHandler, "$@ is a non-trivial signal handler that uses not async-safe functions. " + msg +
+ "Handler is registered by $@.", signalHandler, signalHandler.toString(), fc, fc.toString()
diff --git a/cpp/test/include/libc/signal.h b/cpp/test/include/libc/signal.h
index 5ade866..fcc0bf8 100644
--- a/cpp/test/include/libc/signal.h
+++ b/cpp/test/include/libc/signal.h
@@ -3,27 +3,27 @@
#ifndef HEADER_SIGNAL_STUB_H
#define HEADER_SIGNAL_STUB_H
-#ifdef __cplusplus
-extern "C" {
-#endif
-
#define SIGALRM 14
#define SIGSEGV 11
+#define SIGTERM 15
#define SIG_ERR -1
#define EXIT_FAILURE 2
-#define SA_SIGINFO 0x00000004
+#define SA_SIGINFO 4
-typedef void (*sighandler_t)(int);
-extern int signal(int, sighandler_t);
+{} // to silent error from codeql's extractor
+typedef void (*sig_t)(int);
+extern int signal(int, sig_t);
typedef struct {
unsigned long sig[64];
} sigset_t;
+
typedef struct {
int si_signo; /* Signal number */
int si_errno; /* An errno value */
int si_code; /* Signal code */
} siginfo_t;
+
struct sigaction {
void (*sa_handler)(int);
void (*sa_sigaction)(int, siginfo_t *, void *);
@@ -35,9 +35,6 @@ struct sigaction {
extern int sigaction(int signum, const struct sigaction *act, struct sigaction *oldact);
extern int kill(int pid, int sig);
-#ifdef __cplusplus
-}
-#endif
#endif
diff --git a/cpp/test/include/libc/stdarg.h b/cpp/test/include/libc/stdarg.h
new file mode 100644
index 0000000..c9f353f
--- /dev/null
+++ b/cpp/test/include/libc/stdarg.h
@@ -0,0 +1,25 @@
+#ifndef USE_HEADERS
+
+#ifndef HEADER_STDARG_STUB_H
+#define HEADER_STDARG_STUB_H
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+typedef void *va_list;
+#define va_start(ap, parmN)
+#define va_end(ap)
+#define va_arg(ap, type) ((type)0)
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif
+
+#else // --- else USE_HEADERS
+
+#include
+
+#endif // --- end USE_HEADERS
\ No newline at end of file
diff --git a/cpp/test/include/libc/stdlib.h b/cpp/test/include/libc/stdlib.h
index cbacdf8..cc18de3 100644
--- a/cpp/test/include/libc/stdlib.h
+++ b/cpp/test/include/libc/stdlib.h
@@ -16,6 +16,7 @@ long lrand48(void) {
}
void _Exit(int);
+void exit(int);
void free(void*);
void *malloc(unsigned long);
diff --git a/cpp/test/include/libc/string_stubs.h b/cpp/test/include/libc/string_stubs.h
index e7e83ce..9310e50 100644
--- a/cpp/test/include/libc/string_stubs.h
+++ b/cpp/test/include/libc/string_stubs.h
@@ -24,6 +24,10 @@ extern int wprintf(const wchar_t * format, ...);
extern wchar_t* wcscpy(wchar_t * s1, const wchar_t * s2);
extern void perror(const char *s);
+extern void openlog(const char*, int, int);
+extern void syslog(int, const char*, ...);
+extern void closelog(void)
+
#ifdef __cplusplus
}
#endif
@@ -37,6 +41,7 @@ extern void perror(const char *s);
#include
#include
#include
+#include
#define strcpy_s strcpy
#define _mbsncat strncat
diff --git a/cpp/test/include/libc/unistd.h b/cpp/test/include/libc/unistd.h
new file mode 100644
index 0000000..7163a23
--- /dev/null
+++ b/cpp/test/include/libc/unistd.h
@@ -0,0 +1,22 @@
+#ifndef USE_HEADERS
+
+#ifndef HEADER_UNISTD_STUB_H
+#define HEADER_UNISTD_STUB_H
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+void _exit(int);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif
+
+#else // --- else USE_HEADERS
+
+#include
+
+#endif // --- end USE_HEADERS
\ No newline at end of file
diff --git a/cpp/test/query-tests/security/AsyncUnsafeSignalHandler/AsyncUnsafeSignalHandler.c b/cpp/test/query-tests/security/AsyncUnsafeSignalHandler/AsyncUnsafeSignalHandler.c
index 32b6b43..72c0b9c 100644
--- a/cpp/test/query-tests/security/AsyncUnsafeSignalHandler/AsyncUnsafeSignalHandler.c
+++ b/cpp/test/query-tests/security/AsyncUnsafeSignalHandler/AsyncUnsafeSignalHandler.c
@@ -1,11 +1,14 @@
#include "../../../include/libc/string_stubs.h"
#include "../../../include/libc/signal.h"
#include "../../../include/libc/stdlib.h"
+#include "../../../include/libc/unistd.h"
+#include "../../../include/libc/stdarg.h"
void transitive_call() {
printf("UNSAFE");
}
void transitive_call3() {
+ // TODO: decide which functions are exploitable
int *x = (int*)malloc(16);
free(x);
}
@@ -20,6 +23,12 @@ void safe_handler(int sig) {
}
}
+void safe_handler2(int signo, siginfo_t *info, void *context) {
+ if (signo == SIGALRM) {
+ kill(1, SIGALRM);
+ }
+}
+
void unsafe_handler(int sig) {
transitive_call();
}
@@ -31,10 +40,57 @@ void unsafe_handler2(int signo, siginfo_t *info, void *context) {
transitive_call2();
}
-void unsafe_handler3(int signal) {
+void unsafe_handler3(int signo, siginfo_t *info, void *context) {
transitive_call3();
}
+// data flow case, regresshion-like
+#define sigdie(...) do_logging_and_die(__FILE__, NULL, __VA_ARGS__)
+
+static void do_log(int level, const char *suffix, const char *fmt, va_list args) {
+ int pri = 0;
+ openlog(suffix, 1, 2);
+ // the unsafe call from the handler
+ syslog(pri, "%.500s", fmt);
+ closelog();
+}
+
+static const int level = 0;
+void logv(const char *file, const char *suffix, const char *fmt, va_list args) {
+ char tmp[] = "sufsuf: ";
+ suffix = tmp;
+ if(!args) {
+ suffix = &tmp[3];
+ }
+ do_log(level, suffix, fmt, args);
+}
+
+void do_logging_and_die(const char *file, const char *suffix, const char *fmt, ...) {
+ va_list args;
+ va_start(args, fmt);
+ logv(file, suffix, fmt, args);
+ va_end(args);
+ _exit(1);
+}
+
+static void df_handler(int sig) {
+ if (2*sig % 5 == 3) {
+ _exit(1);
+ }
+ sigdie("some log %d", sig);
+}
+
+sig_t register_signal(int signum, sig_t handler) {
+ struct sigaction sa, osa;
+ memset(&sa, 0, sizeof(sa));
+ sa.sa_handler = handler;
+ sigfillset(&sa.sa_mask);
+ if (sigaction(signum, &sa, &osa) == -1) {
+ return NULL;
+ }
+ return osa.sa_handler;
+}
+
int main() {
// Register the safe signal handler
if (signal(SIGALRM, safe_handler) == SIG_ERR) {
@@ -42,13 +98,23 @@ int main() {
_Exit(EXIT_FAILURE);
}
- // Unsafe example
+ // Safe handler 2
+ struct sigaction actG2 = {0};
+ actG2.sa_flags = SA_SIGINFO;
+ actG2.sa_sigaction = &safe_handler2;
+ if (sigaction(SIGSEGV, &actG2, NULL) == -1) {
+ perror("sigaction");
+ _Exit(EXIT_FAILURE);
+ }
+
+ // Unsafe example 1
if (signal(SIGALRM, unsafe_handler) == SIG_ERR) {
perror("Unable to catch SIGALRM");
_Exit(EXIT_FAILURE);
}
- // Another unsafe example
+ // Unsafe example 2
+ // TODO: decide which signals are exploitable and should produce findings
struct sigaction act = { 0 };
act.sa_flags = SA_SIGINFO;
act.sa_sigaction = &unsafe_handler2;
@@ -57,13 +123,52 @@ int main() {
_Exit(EXIT_FAILURE);
}
- struct sigaction act2 = { 0 };
- act2.sa_flags = SA_SIGINFO;
- act2.sa_handler = &unsafe_handler3;
+ // Unsafe example 3
+ struct sigaction act2 = {.sa_flags = SA_SIGINFO, .sa_sigaction = unsafe_handler2};
if (sigaction(SIGALRM, &act2, NULL) == -1) {
perror("sigaction 2");
_Exit(EXIT_FAILURE);
}
+ // Unsafe example 4
+ // TODO: not every masked signals should indicate safe signal handling
+ sigset_t sigset;
+ sigemptyset(&sigset);
+ sigaddset(&sigset, SIGALRM);
+ struct sigaction act3 = {.sa_flags = SA_SIGINFO, .sa_sigaction = unsafe_handler2, .sa_mask = sigset};
+ if (sigaction(SIGALRM, &act3, NULL) == -1) {
+ perror("sigaction 3");
+ _Exit(EXIT_FAILURE);
+ }
+
+ // Unsafe example 5
+ struct sigaction act4 = {.sa_flags = SA_SIGINFO, .sa_sigaction = unsafe_handler3};
+ act4.sa_mask = sigset;
+ if (sigaction(SIGSEGV, &act4, NULL) == -1) {
+ perror("sigaction 4");
+ _Exit(EXIT_FAILURE);
+ }
+
+ // Unsafe example 6
+ struct sigaction act5 = {.sa_mask = sigset};
+ act5.sa_flags = SA_SIGINFO;
+ act5.sa_sigaction = &unsafe_handler3;
+ if (sigaction(SIGSEGV, &act5, NULL) == -1) {
+ perror("sigaction 5");
+ _Exit(EXIT_FAILURE);
+ }
+
+ // Unsafe example 7
+ sig_t wrapper = unsafe_handler;
+ if (signal(SIGALRM, wrapper) == SIG_ERR) {
+ perror("Unable to catch SIGALRM");
+ _Exit(EXIT_FAILURE);
+ }
+
+ // Unsafe example 8, indirect handler registration
+ if(register_signal(SIGALRM, df_handler)) {
+ _exit(EXIT_FAILURE);
+ }
+
return 0;
}
\ No newline at end of file
diff --git a/cpp/test/query-tests/security/AsyncUnsafeSignalHandler/AsyncUnsafeSignalHandler.expected b/cpp/test/query-tests/security/AsyncUnsafeSignalHandler/AsyncUnsafeSignalHandler.expected
index e51cab2..da13543 100644
--- a/cpp/test/query-tests/security/AsyncUnsafeSignalHandler/AsyncUnsafeSignalHandler.expected
+++ b/cpp/test/query-tests/security/AsyncUnsafeSignalHandler/AsyncUnsafeSignalHandler.expected
@@ -1,3 +1,8 @@
-| AsyncUnsafeSignalHandler.c:23:6:23:19 | unsafe_handler | is a non-trivial signal handler that may be using functions that are not async-safe. |
-| AsyncUnsafeSignalHandler.c:27:6:27:20 | unsafe_handler2 | is a non-trivial signal handler that may be using functions that are not async-safe. |
-| AsyncUnsafeSignalHandler.c:34:6:34:20 | unsafe_handler3 | is a non-trivial signal handler that may be using functions that are not async-safe. |
+| AsyncUnsafeSignalHandler.c:32:6:32:19 | unsafe_handler | $@ is a non-trivial signal handler that uses not async-safe functions. Moreover, delivery of new signals may be not blocked. Handler is registered by $@. | AsyncUnsafeSignalHandler.c:32:6:32:19 | unsafe_handler | unsafe_handler | AsyncUnsafeSignalHandler.c:111:9:111:14 | call to signal | call to signal |
+| AsyncUnsafeSignalHandler.c:32:6:32:19 | unsafe_handler | $@ is a non-trivial signal handler that uses not async-safe functions. Moreover, delivery of new signals may be not blocked. Handler is registered by $@. | AsyncUnsafeSignalHandler.c:32:6:32:19 | unsafe_handler | unsafe_handler | AsyncUnsafeSignalHandler.c:163:9:163:14 | call to signal | call to signal |
+| AsyncUnsafeSignalHandler.c:36:6:36:20 | unsafe_handler2 | $@ is a non-trivial signal handler that uses not async-safe functions. Handler is registered by $@. | AsyncUnsafeSignalHandler.c:36:6:36:20 | unsafe_handler2 | unsafe_handler2 | AsyncUnsafeSignalHandler.c:139:9:139:17 | call to sigaction | call to sigaction |
+| AsyncUnsafeSignalHandler.c:36:6:36:20 | unsafe_handler2 | $@ is a non-trivial signal handler that uses not async-safe functions. Moreover, delivery of new signals may be not blocked. Handler is registered by $@. | AsyncUnsafeSignalHandler.c:36:6:36:20 | unsafe_handler2 | unsafe_handler2 | AsyncUnsafeSignalHandler.c:121:9:121:17 | call to sigaction | call to sigaction |
+| AsyncUnsafeSignalHandler.c:36:6:36:20 | unsafe_handler2 | $@ is a non-trivial signal handler that uses not async-safe functions. Moreover, delivery of new signals may be not blocked. Handler is registered by $@. | AsyncUnsafeSignalHandler.c:36:6:36:20 | unsafe_handler2 | unsafe_handler2 | AsyncUnsafeSignalHandler.c:128:9:128:17 | call to sigaction | call to sigaction |
+| AsyncUnsafeSignalHandler.c:43:6:43:20 | unsafe_handler3 | $@ is a non-trivial signal handler that uses not async-safe functions. Handler is registered by $@. | AsyncUnsafeSignalHandler.c:43:6:43:20 | unsafe_handler3 | unsafe_handler3 | AsyncUnsafeSignalHandler.c:147:9:147:17 | call to sigaction | call to sigaction |
+| AsyncUnsafeSignalHandler.c:43:6:43:20 | unsafe_handler3 | $@ is a non-trivial signal handler that uses not async-safe functions. Handler is registered by $@. | AsyncUnsafeSignalHandler.c:43:6:43:20 | unsafe_handler3 | unsafe_handler3 | AsyncUnsafeSignalHandler.c:156:9:156:17 | call to sigaction | call to sigaction |
+| AsyncUnsafeSignalHandler.c:76:13:76:22 | df_handler | $@ is a non-trivial signal handler that uses not async-safe functions. Handler is registered by $@. | AsyncUnsafeSignalHandler.c:76:13:76:22 | df_handler | df_handler | AsyncUnsafeSignalHandler.c:88:6:88:14 | call to sigaction | call to sigaction |