Merge pull request #4 from trailofbits/dm/signal-handler

Tentative query for CVE-2024-6387

Author: mschwager

README.md

@@ -36,7 +36,7 @@ codeql database analyze database.db --format=sarif-latest --output=./tob.sarif -
 |[Memory leak related to custom allocator](./cpp/src/docs/crypto/CustomAllocatorLeak.md)|Finds memory leaks from custom allocated memory|warning|medium|
 |[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|
 |[Missing OpenSSL engine initialization](./cpp/src/docs/crypto/MissingEngineInit.md)|Finds created OpenSSL engines that may not be properly initialized|warning|medium|
-|[Missing error handling](./cpp/src/docs/crypto/ErrorHandling.md)|Checks if returned error codes are properly checked|warning|high|
+|[Missing error handling](./cpp/src/docs/crypto/MissingErrorHandling.md)|Checks if returned error codes are properly checked|warning|high|
 |[Missing zeroization of potentially sensitive random BIGNUM](./cpp/src/docs/crypto/MissingZeroization.md)|Determines if random bignums are properly zeroized|warning|medium|
 |[Random buffer too small](./cpp/src/docs/crypto/RandomBufferTooSmall.md)|Finds buffer overflows in calls to CSPRNGs|warning|high|
 |[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 -
 
 | Name | Description | Severity | Precision  |
 | ---  | ----------- | :----:   | :--------: |
+|[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|
 |[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|
 |[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|
 |[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

@@ -0,0 +1,60 @@
+# Async unsafe signal handler
+This is a CodeQL query constructed to find signal handlers that are performing async unsafe operations.
+
+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.
+
+
+## Recommendation
+Attempt to keep signal handlers as simple as possible. Only call async-safe functions from signal handlers.
+
+
+## Example
+
+```c
+#include <signal.h>
+#include <stdio.h>
+#include <stdlib.h>
+ 
+enum { MAXLINE = 1024 };
+volatile sig_atomic_t eflag = 0;
+char *info = NULL;
+ 
+void log_message(void) {
+  fputs(info, stderr);
+}
+ 
+void correct_handler(int signum) {
+  eflag = 1;
+}
+ 
+int main(void) {
+  if (signal(SIGINT, correct_handler) == SIG_ERR) {
+    /* Handle error */
+  }
+  info = (char *)malloc(MAXLINE);
+  if (info == NULL) {
+    /* Handle error */
+  }
+ 
+  while (!eflag) {
+    /* Main loop program code */
+ 
+    log_message();
+ 
+    /* More program code */
+  }
+ 
+  log_message();
+  free(info);
+  info = NULL;
+ 
+  return 0;
+}
+```
+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.
+
+
+## References
+* [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)
+* [regreSSHion: RCE in OpenSSH's server, on glibc-based Linux systems](https://www.qualys.com/2024/07/01/cve-2024-6387/regresshion.txt)
+* [signal-safety - async-signal-safe functions](https://man7.org/linux/man-pages/man7/signal-safety.7.html)

cpp/src/security/AsyncUnsafeSignalHandler/AsyncUnsafeSignalHandler.c

@@ -0,0 +1,39 @@
+#include <signal.h>
+#include <stdio.h>
+#include <stdlib.h>
+ 
+enum { MAXLINE = 1024 };
+volatile sig_atomic_t eflag = 0;
+char *info = NULL;
+ 
+void log_message(void) {
+  fputs(info, stderr);
+}
+ 
+void correct_handler(int signum) {
+  eflag = 1;
+}
+ 
+int main(void) {
+  if (signal(SIGINT, correct_handler) == SIG_ERR) {
+    /* Handle error */
+  }
+  info = (char *)malloc(MAXLINE);
+  if (info == NULL) {
+    /* Handle error */
+  }
+ 
+  while (!eflag) {
+    /* Main loop program code */
+ 
+    log_message();
+ 
+    /* More program code */
+  }
+ 
+  log_message();
+  free(info);
+  info = NULL;
+ 
+  return 0;
+}

cpp/src/security/AsyncUnsafeSignalHandler/AsyncUnsafeSignalHandler.qhelp

@@ -0,0 +1,43 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+  <p>
+  This is a CodeQL query constructed to find signal handlers that are performing async unsafe operations.
+  </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.
+  </p>
+</overview>
+
+<recommendation>
+<p>
+Attempt to keep signal handlers as simple as possible. Only call async-safe functions from signal handlers.
+</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.
+</p>
+</example>
+
+<references>
+<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>
+<li>
+    <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>
+</li>
+<li>
+    <a href="https://man7.org/linux/man-pages/man7/signal-safety.7.html">signal-safety - async-signal-safe functions</a>
+</li>
+</references>
+
+</qhelp>
+

cpp/src/security/AsyncUnsafeSignalHandler/AsyncUnsafeSignalHandler.ql

@@ -0,0 +1,65 @@
+/**
+ * @name Async unsafe signal handler
+ * @id tob/cpp/async-unsafe-signal-handler
+ * @description Async unsafe signal handler (like the one used in CVE-2024-6387)
+ * @kind problem
+ * @tags security
+ * @problem.severity warning
+ * @precision high
+ * @security-severity 7.0
+ * @group security
+ */
+
+import cpp
+
+/* List from https://man7.org/linux/man-pages/man3/stdio.3.html */
+class StdioFunction extends Function {
+  StdioFunction() {
+    this.getName() in [
+        "fopen", "freopen", "fflush", "fclose", "fread", "fwrite", "fgetc", "fgets", "fputc",
+        "fputs", "getc", "getchar", "gets", "putc", "putchar", "puts", "ungetc", "fprintf",
+        "fscanf", "printf", "scanf", "sprintf", "sscanf", "vprintf", "vfprintf", "vsprintf",
+        "fgetpos", "fsetpos", "ftell", "fseek", "rewind", "clearerr", "feof", "ferror", "perror",
+        "setbuf", "setvbuf", "remove", "rename", "tmpfile", "tmpnam",
+      ]
+  }
+}
+
+/* List from https://man7.org/linux/man-pages/man3/syslog.3.html */
+class SyslogFunction extends Function {
+  SyslogFunction() { this.getName() in ["syslog", "vsyslog",] }
+}
+
+/* List from https://man7.org/linux/man-pages/man0/stdlib.h.0p.html */
+class StdlibFunction extends Function {
+  StdlibFunction() { this.getName() in ["malloc", "calloc", "realloc", "free",] }
+}
+
+predicate isAsyncUnsafe(Function signalHandler) {
+  exists(Function f |
+    signalHandler.calls+(f) and
+    (
+      f instanceof StdioFunction or
+      f instanceof SyslogFunction or
+      f instanceof StdlibFunction
+    )
+  )
+}
+
+predicate isSignalHandlerField(FieldAccess fa) {
+  fa.getTarget().getName() in ["__sa_handler", "__sa_sigaction", "sa_handler", "sa_sigaction"]
+}
+
+
+from FunctionCall fc, Function signalHandler, FieldAccess fa
+where
+  fc.getTarget().getName() = "signal" and
+  signalHandler.getName() = fc.getArgument(1).toString() and
+  isAsyncUnsafe(signalHandler)
+  or
+  fc.getTarget().getName() = "sigaction" and
+  isSignalHandlerField(fa) and
+  signalHandler = fa.getTarget().getAnAssignedValue().(AddressOfExpr).getAddressable() and
+  isAsyncUnsafe(signalHandler)
+select signalHandler,
+  "is a non-trivial signal handler that may be using functions that are not async-safe."

cpp/test/include/libc/signal.h

@@ -0,0 +1,48 @@
+#ifndef USE_HEADERS
+
+#ifndef HEADER_SIGNAL_STUB_H
+#define HEADER_SIGNAL_STUB_H
+
+#ifdef  __cplusplus
+extern "C" {
+#endif
+
+#define SIGALRM 14
+#define SIGSEGV 11
+#define SIG_ERR -1
+#define EXIT_FAILURE 2
+#define SA_SIGINFO	0x00000004
+
+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 *);
+    sigset_t   sa_mask;
+    int        sa_flags;
+    void     (*sa_restorer)(void);
+};
+
+extern int sigaction(int signum, const struct sigaction *act, struct sigaction *oldact);
+extern int kill(int pid, int sig);
+
+#ifdef  __cplusplus
+}
+#endif
+
+#endif
+
+#else // --- else USE_HEADERS
+
+#include <signal.h>
+
+#endif // --- end USE_HEADERS

cpp/test/include/libc/stdlib.h

@@ -3,9 +3,9 @@
 #ifndef HEADER_STDLIB_STUB_H
 #define HEADER_STDLIB_STUB_H
 
-# ifdef  __cplusplus
+#ifdef  __cplusplus
 extern "C" {
-# endif
+#endif
 
 int rand(void) {
   return 42;
@@ -15,9 +15,14 @@ long lrand48(void) {
   return 42;
 }
 
-# ifdef  __cplusplus
+void _Exit(int);
+
+void free(void*);
+void *malloc(unsigned long);
+
+#ifdef  __cplusplus
 }
-# endif
+#endif
 
 #endif
 

cpp/test/include/libc/string_stubs.h

@@ -1,5 +1,12 @@
 #ifndef USE_HEADERS
 
+#ifndef HEADER_STRINGSTUB_STUB_H
+#define HEADER_STRINGSTUB_STUB_H
+
+#ifdef  __cplusplus
+extern "C" {
+#endif
+
 #ifndef NULL
     #define NULL 0
 #endif
@@ -10,11 +17,18 @@ extern int _mbsncat(char* dst, char* src, int count);
 extern int _mbsncmp(char* dst, char* src, int count);
 extern int _memicmp(char* a, char* b, long count);
 extern int _mbsnbcmp(char* a, char* b, int count);
-extern void *printf(const char * format, ...);
-extern int strlen(const char *s);
+extern int printf(const char * format, ...);
+extern unsigned long strlen(const char *s);
 extern char* strcpy(char * dst, const char * src);
 extern int wprintf(const wchar_t * format, ...);
 extern wchar_t* wcscpy(wchar_t * s1, const wchar_t * s2);
+extern void perror(const char *s);
+
+#ifdef  __cplusplus
+}
+#endif
+
+#endif
 
 #else