blocked signals fixed

Author: GrosQuildu

cpp/src/security/AsyncUnsafeSignalHandler/AsyncUnsafeSignalHandler.ql

@@ -57,8 +57,8 @@ predicate isSignal(FunctionCall signalCall, Function signalHandler) {
  * struct sigaction sigactVar = ...
  * sigaction(SIGX, &sigactVar, ...)
  */
-predicate isSigaction(FunctionCall sigactionCall, Function signalHandler, boolean isReentrancyBlocked) {
-  exists(Variable sigactVar, Struct sigactStruct, Field handlerField |
+predicate isSigaction(FunctionCall sigactionCall, Function signalHandler, Variable sigactVar) {
+  exists(Struct sigactStruct, Field handlerField |
     sigactionCall.getTarget().getName() = "sigaction"
     and sigactionCall.getArgument(1).getAChild*() = sigactVar.getAnAccess()
 
@@ -79,45 +79,46 @@ predicate isSigaction(FunctionCall sigactionCall, Function signalHandler, boolea
         and dfa.getQualifier+() = sigactVar.getAnAccess()
       )
       or
-      exists(VariableDeclarationEntry varDec, ClassAggregateLiteral init |
+      exists(ClassAggregateLiteral initLit |
         // struct sigaction sigactVar = {.sa_sigaction = signalHandler};
-        varDec.getVariable() = sigactVar
-        and sigactVar.getInitializer().getExpr() = init
-        and signalHandler.getAnAccess() = init.getAFieldExpr(handlerField).getAChild*()
-
-        // 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
+        // varDec.getVariable() = sigactVar
+        sigactVar.getInitializer().getExpr() = initLit
+        and signalHandler.getAnAccess() = initLit.getAFieldExpr(handlerField).getAChild*()
       )
     )
-
-    // 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 = "")
+predicate isSignalDeliveryBlocked(Variable sigactVar) {
+  // TODO: should only find writes and for specific signals 
+  exists(ValueFieldAccess dfa |
+    dfa.getQualifier+() = sigactVar.getAnAccess() and dfa.getTarget().getName() = "sa_mask"
+  )
   or
-  (isReentrancyBlocked = false and result = "Delivery of new signals may be not blocked when the handler executes. ")
+  exists(Field mask |
+    mask.getName() = "sa_mask"
+    and exists(sigactVar.getInitializer().getExpr().(ClassAggregateLiteral).getAFieldExpr(mask))
+  )
+}
+
+string deliveryNotBlockedMsg() {
+  result = "Delivery of new signals may be not blocked when the handler executes. "
 }
 
-from FunctionCall fc, Function signalHandler, boolean isReentrancyBlocked
+from FunctionCall fc, Function signalHandler, string msg
 where
   isAsyncUnsafe(signalHandler)
   and (
-    (isSignal(fc, signalHandler) and isReentrancyBlocked = false)
+    (isSignal(fc, signalHandler) and msg = deliveryNotBlockedMsg())
     or
-    isSigaction(fc, signalHandler, isReentrancyBlocked)
+    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. " + fmtMsg(isReentrancyBlocked) +
+select signalHandler, "is a non-trivial signal handler that uses not async-safe functions. " + msg +
   "Handler is registered by $@", fc, fc.toString()
 

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

@@ -92,13 +92,13 @@ int main() {
         _Exit(EXIT_FAILURE);
     }
 
-    // Unsafe example
+    // 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;
@@ -108,14 +108,14 @@ int main() {
         _Exit(EXIT_FAILURE);
     }
 
-    // Yet another unsafe
+    // 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);
     }
 
-    // Let's call these safe, because some signals are blocked
+    // Unsafe example 4
     // TODO: not every masked signals should indicate safe signal handling
     sigset_t sigset;
     sigemptyset(&sigset);
@@ -126,14 +126,15 @@ int main() {
         _Exit(EXIT_FAILURE);
     }
 
+    // Unsafe example 5
     struct sigaction act4 = {.sa_flags = SA_SIGINFO, .sa_sigaction = unsafe_handler2};
     act4.sa_mask = sigset;
     if (sigaction(SIGSEGV, &act4, NULL) == -1) {
         perror("sigaction 4");
         _Exit(EXIT_FAILURE);
     }
 
-    // TODO: this example should be not marked as finding
+    // Unsafe example 6
     struct sigaction act5 = {.sa_mask = sigset};
     act5.sa_flags = SA_SIGINFO;
     act5.sa_sigaction = &unsafe_handler2;
@@ -142,7 +143,7 @@ int main() {
         _Exit(EXIT_FAILURE);
     }
 
-    // indirect handler registration
+    // Unsafe example 7, indirect handler registration
     if(register_signal(SIGALRM, df_handler)) {
         _exit(EXIT_FAILURE);
     }