better qlhelp, ms_mask only changes msg

Author: GrosQuildu

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

@@ -57,7 +57,7 @@ predicate isSignal(FunctionCall signalCall, Function signalHandler) {
  * struct sigaction sigactVar = ...
  * sigaction(SIGX, &sigactVar, ...)
  */
-predicate isSigaction(FunctionCall sigactionCall, Function signalHandler) {
+predicate isSigaction(FunctionCall sigactionCall, Function signalHandler, boolean isReentrancyBlocked) {
   exists(Variable sigactVar, Struct sigactStruct, Field handlerField |
     sigactionCall.getTarget().getName() = "sigaction"
     and sigactionCall.getArgument(1).getAChild*() = sigactVar.getAnAccess()
@@ -84,25 +84,40 @@ predicate isSigaction(FunctionCall sigactionCall, Function signalHandler) {
         varDec.getVariable() = sigactVar
         and sigactVar.getInitializer().getExpr() = init
         and signalHandler.getAnAccess() = init.getAFieldExpr(handlerField).getAChild*()
-        // ignore if signals are blocked via sa_mask
-        and not exists(Field mask | mask.getName() = "sa_mask" and exists(init.getAFieldExpr(mask)))
+
+        // new signals are blocked via sa_mask
+        and if exists(Field mask | mask.getName() = "sa_mask" and exists(init.getAFieldExpr(mask))) then
+          isReentrancyBlocked = true
+        else
+          isReentrancyBlocked = false
       )
     )
-    // ignore if signals are blocked via sa_mask
-    and not exists(ValueFieldAccess dfa |
+
+    // new signals are blocked via sa_mask
+    and if (isReentrancyBlocked = true or exists(ValueFieldAccess dfa |
       dfa.getQualifier+() = sigactVar.getAnAccess()
       and dfa.getTarget().getName() = "sa_mask"
-    )
+    )) then
+      isReentrancyBlocked = true
+    else
+      isReentrancyBlocked = false
   )
 }
+
+string fmtMsg(boolean isReentrancyBlocked) {
+  (isReentrancyBlocked = true and result = "")
+  or
+  (isReentrancyBlocked = false and result = "Delivery of new signals may be not blocked when the handler executes. ")
+}
 
-from FunctionCall fc, Function signalHandler  
+from FunctionCall fc, Function signalHandler, boolean isReentrancyBlocked
 where
   isAsyncUnsafe(signalHandler)
   and (
-    isSignal(fc, signalHandler)
+    (isSignal(fc, signalHandler) and isReentrancyBlocked = false)
     or
-    isSigaction(fc, signalHandler)
+    isSigaction(fc, signalHandler, isReentrancyBlocked)
   )
-select signalHandler, "is a non-trivial signal handler that may be using not async-safe functions. Registered by $@", fc, fc.toString()
+select signalHandler, "is a non-trivial signal handler that uses not async-safe functions. " + fmtMsg(isReentrancyBlocked) +
+  "Handler is registered by $@", fc, fc.toString()
 

cpp/test/include/libc/signal.h

@@ -3,27 +3,29 @@
 #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
 
+{} // to silent error from codeql's extractor
 typedef void (*sighandler_t)(int);
 extern int signal(int, sighandler_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 +37,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

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

@@ -1,6 +1,8 @@
 #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");
@@ -36,6 +38,53 @@ void unsafe_handler3(int signal) {
     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);
+}
+
+sighandler_t register_signal(int signum, sighandler_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) {
@@ -93,6 +142,10 @@ int main() {
         _Exit(EXIT_FAILURE);
     }
 
+    // indirect handler registration
+    if(register_signal(SIGALRM, df_handler)) {
+        _exit(EXIT_FAILURE);
+    }
 
     return 0;
 }