Merge pull request #7 from trailofbits/signal-handler-enhance

Enhanced version of signal query

cpp/src/security/AsyncUnsafeSignalHandler/AsyncUnsafeSignalHandler.qhelp

@@ -7,27 +7,48 @@
   This is a CodeQL query constructed to find signal handlers that are performing async unsafe operations.
   </p>
 
+  <p>
+  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.
+  </p>
+
   <p>
   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>.
-  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.
+  </p>
+
+  <p>
+  Moreover, signal handlers may be re-entered. Handlers' logic should take that possibility into account.
+  </p>
+
+  <p>
+  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.
   </p>
 </overview>
 
 <recommendation>
 <p>
 Attempt to keep signal handlers as simple as possible. Only call async-safe functions from signal handlers.
 </p>
+<p>
+Block delivery of new signals inside signal handlers to prevent handler re-entrancy issues.
+</p>
 </recommendation>
 
 <example>
 <sample src="AsyncUnsafeSignalHandler.c" />
 
 <p>
-  In this example, while both syntatically valid, a correct handler is defined in the <code>correct_handler</code> function and sets a flag. The function calls <code>log_message</code>, a async unsafe function, within the main loop.
+  In this example, while both syntatically valid, a correct handler is defined in the <code>correct_handler</code> function and sets a flag.
+  The function calls <code>log_message</code>, a async unsafe function, within the main loop.
 </p>
 </example>
 
 <references>
+<li>
+  <a href="https://lcamtuf.coredump.cx/signals.txt">Michal Zalewski, "Delivering Signals for Fun and Profit"</a>
+</li>
 <li>
     <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>
 </li>

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<HandlerToSignalConfiguration>;
 
-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()

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
 

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 <stdarg.h>
+
+#endif // --- end USE_HEADERS

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);

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 <stdio.h>
 #include <stdlib.h>
 #include <wchar.h>
+#include <syslog.h>
 
 #define strcpy_s strcpy
 #define _mbsncat strncat

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 <unistd.h>
+
+#endif // --- end USE_HEADERS