From e4b86aa5d829c401ab431253964922dfa5b5b8b0 Mon Sep 17 00:00:00 2001
From: Brian Gianforcaro <bgianf@serenityos.org>
Date: Fri, 23 Jul 2021 09:21:43 -0700
Subject: [PATCH] Kernel: Fix bug where we half apply pledges in sys$pledge(..)

This bug manifests it self when the caller to sys$pledge() passes valid
promises, but invalid execpromises. The code would apply the promises
and then return an error for the execpromises. This leaves the user in
a confusing state, as the promises were silently applied, but we return
an error suggesting the operation has failed.

Avoid this situation by tweaking the implementation to only apply the
promises / execpromises after all validation has occurred.
---
 Kernel/Syscalls/pledge.cpp | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/Kernel/Syscalls/pledge.cpp b/Kernel/Syscalls/pledge.cpp
index ecd0a25b7b4..4b83e4e5413 100644
--- a/Kernel/Syscalls/pledge.cpp
+++ b/Kernel/Syscalls/pledge.cpp
@@ -52,22 +52,33 @@ KResultOr<FlatPtr> Process::sys$pledge(Userspace<const Syscall::SC_pledge_params
 
     ProtectedDataMutationScope scope { *this };
 
+    u32 new_promises = 0;
     if (promises) {
-        u32 new_promises = 0;
         if (!parse_pledge(promises->view(), new_promises))
             return EINVAL;
         if (m_has_promises && (new_promises & ~m_promises))
             return EPERM;
+    }
+
+    u32 new_execpromises = 0;
+    if (execpromises) {
+        if (!parse_pledge(execpromises->view(), new_execpromises))
+            return EINVAL;
+        if (m_has_execpromises && (new_execpromises & ~m_execpromises))
+            return EPERM;
+    }
+
+    // Only apply promises after all validation has occurred, this ensures
+    // we don't introduce logic bugs like applying the promises, and then
+    // erroring out when parsing the exec promises later. Such bugs silently
+    // leave the caller in an unexpected state.
+
+    if (promises) {
         m_has_promises = true;
         m_promises = new_promises;
     }
 
     if (execpromises) {
-        u32 new_execpromises = 0;
-        if (!parse_pledge(execpromises->view(), new_execpromises))
-            return EINVAL;
-        if (m_has_execpromises && (new_execpromises & ~m_execpromises))
-            return EPERM;
         m_has_execpromises = true;
         m_execpromises = new_execpromises;
     }