bug#8427: [SECURITY] sql.el -- comint process passwords are leaked to ps(1) listing

[Andrew Hyatt]

I've simplified an implementation along the lines you suggest, and
tested it via ert. I'm attaching the latest version of the patch.
Please let me know what you think.

>From 2d0632b08350d86049c2e20c50ce67d69ad52c6d Mon Sep 17 00:00:00 2001
From: Andrew Hyatt <ahyatt@gmail.com>
Date: Fri, 18 Oct 2019 21:56:52 -0400
Subject: [PATCH] Enable passwords to be sent in-process when possible.

* lisp/progmodes/sql.el (sql-comint, sql-comint-mysql):
  Add a way to handle passwords to be sent in the comint process.
  This is controlled by the sql-product variable :password-in-comint.
  When true, on the first password prompt, send argument to signal to
  the SQL process to read the password inside the process.  This
  removes the slight chance that someone can spy on the password from
  ps or via other methods.

* test/lisp/progmodes/sql-tests.el: New tests for the password
   interception.
---
 lisp/progmodes/sql.el            | 60 +++++++++++++++++++++++++------
 test/lisp/progmodes/sql-tests.el | 61 ++++++++++++++++++++++++++++++++
 2 files changed, 111 insertions(+), 10 deletions(-)

diff --git a/lisp/progmodes/sql.el b/lisp/progmodes/sql.el
index b17364b08f..f7cbec7130 100644
--- a/lisp/progmodes/sql.el
+++ b/lisp/progmodes/sql.el
@@ -160,13 +160,16 @@
 ;;       "Connect ti XyzDB in a comint buffer."
 ;;
 ;;         ;; Do something with `sql-user', `sql-password',
-;;         ;; `sql-database', and `sql-server'.
+;;         ;; `sql-database', and `sql-server'.  `sql-password' will
+;;         ;; be sent automatically if not sent in the command-line.
+;;         ;; It is recommended to avoid sending in the command-line
+;;         ;; if possible, since this can briefly expose passwords.
 ;;         (let ((params
 ;;                (append
 ;;           (if (not (string= "" sql-user))
 ;;                     (list "-U" sql-user))
 ;;                 (if (not (string= "" sql-password))
-;;                     (list "-P" sql-password))
+;;                     (list "-P"))
 ;;                 (if (not (string= "" sql-database))
 ;;                     (list "-D" sql-database))
 ;;                 (if (not (string= "" sql-server))
@@ -458,6 +461,7 @@ file.  Since that is a plaintext file, this could be 
dangerous."
      :sqli-comint-func sql-comint-mysql
      :list-all "SHOW TABLES;"
      :list-table "DESCRIBE %s;"
+     :password-in-comint t
      :prompt-regexp "^mysql> "
      :prompt-length 6
      :prompt-cont-regexp "^    -> "
@@ -624,6 +628,10 @@ may be any one of the following:
                         not-nil it is the name of a schema whose
                         objects should be listed.
 
+ :password-in-comint    true when the password is not passed in
+                        as a parameter, but instead requested in
+                        the comint session itself.
+
  :prompt-regexp         regular expression string that matches
                         the prompt issued by the product
                         interpreter.
@@ -1402,6 +1410,9 @@ You can change `sql-prompt-length' on 
`sql-interactive-mode-hook'.")
 
 Used by `sql-rename-buffer'.")
 
+(defvar-local sql-password-accepted-via-comint nil
+  "Set to true when the password was accepted via comint.")
+
 (defun sql-buffer-live-p (buffer &optional product connection)
   "Return non-nil if the process associated with buffer is live.
 
@@ -4681,9 +4692,9 @@ the call to \\[sql-product-interactive] with
                            (sql-generate-unique-sqli-buffer-name product nil))
                           ((consp new-name)
                            (sql-generate-unique-sqli-buffer-name product
-                            (read-string
-                             "Buffer name (\"*SQL: XXX*\"; enter `XXX'): "
-                             (sql-make-alternate-buffer-name product))))
+                             (read-string
+                              "Buffer name (\"*SQL: XXX*\"; enter `XXX'): "
+                              (sql-make-alternate-buffer-name product))))
                           ((stringp new-name)
                            (if (or (string-prefix-p " " new-name)
                                    (string-match-p "\\`[*].*[*]\\'" new-name))
@@ -4733,12 +4744,30 @@ the call to \\[sql-product-interactive] with
               (get-buffer new-sqli-buffer)))))
     (user-error "No default SQL product defined: set `sql-product'")))
 
+(defun sql-watch-for-password-prompt (string)
+  "Intercept password prompts when we know the password.
+This must also do the job of detecting password prompts.  STRING
+is the potential password prompt."
+  (if (and
+       sql-password
+       (not (string= "" sql-password))
+       (not sql-password-accepted-via-comint))
+      ;; In this case, we are in charge of watching for the password
+      ;; prompt, so let's accept or reject.  If the sql-password
+      ;; fails, they would have to enter it manually next time.
+      (let ((case-fold-search t))
+        (when (string-match comint-password-prompt-regexp string)
+          (setq sql-password-accepted-via-comint t)
+          (funcall comint-input-sender (get-buffer-process (current-buffer))
+                   sql-password)))
+    (comint-watch-for-password-prompt string)))
+
 (defun sql-comint (product params &optional buf-name)
   "Set up a comint buffer to run the SQL processor.
 
-PRODUCT is the SQL product.  PARAMS is a list of strings which are
-passed as command line arguments.  BUF-NAME is the name of the new
-buffer.  If nil, a name is chosen for it."
+PRODUCT is the SQL product.  PARAMS is a list of strings which
+are passed as command line arguments.  BUF-NAME is the name of
+the new buffer.  If nil, a name is chosen for it."
 
   (let ((program (sql-get-product-feature product :sqli-program)))
     ;; Make sure we can find the program.  `executable-find' does not
@@ -4757,12 +4786,21 @@ buffer.  If nil, a name is chosen for it."
       (setq buf-name (sql-generate-unique-sqli-buffer-name product nil)))
     (set-text-properties 0 (length buf-name) nil buf-name)
 
+    ;; Create the buffer first, because we want to set it up before
+    ;; comint starts to run.
+    (set-buffer (get-buffer-create buf-name))
+    (when (sql-get-product-feature product :password-in-comint)
+      (setq sql-password-accepted-via-comint nil)
+      ;; Substitute our own password watcher function.
+      (add-hook 'comint-output-filter-functions 'sql-watch-for-password-prompt)
+      (remove-hook 'comint-output-filter-functions 
'comint-watch-for-password-prompt))
+
     ;; Start the command interpreter in the buffer
     ;;   PROC-NAME is BUF-NAME without enclosing asterisks
     (let ((proc-name (replace-regexp-in-string "\\`[*]\\(.*\\)[*]\\'" "\\1" 
buf-name)))
       (set-buffer
        (apply #'make-comint-in-buffer
-              proc-name buf-name program nil params)))))
+              proc-name (current-buffer) program nil params)))))
 
 ;;;###autoload
 (defun sql-oracle (&optional buffer)
@@ -5188,7 +5226,9 @@ The default comes from `process-coding-system-alist' and
           (if (not (string= "" sql-user))
               (list (concat "--user=" sql-user)))
           (if (not (string= "" sql-password))
-              (list (concat "--password=" sql-password)))
+              ;; Sending --password will make MySQL prompt for the
+              ;; password.
+              (list "--password"))
           (if (not (= 0 sql-port))
               (list (concat "--port=" (number-to-string sql-port))))
           (if (not (string= "" sql-server))
diff --git a/test/lisp/progmodes/sql-tests.el b/test/lisp/progmodes/sql-tests.el
index 3ac9fb10e4..278e5aba87 100644
--- a/test/lisp/progmodes/sql-tests.el
+++ b/test/lisp/progmodes/sql-tests.el
@@ -410,6 +410,67 @@ The ACTION will be tested after set-up of PRODUCT."
 
     (kill-buffer "*SQL: exist*")))
 
+(defmacro sql-watch-test-harness (expected &rest action)
+  "Set-up state and replace functions for SQL password test.
+
+EXPECTED could be:
+  - 'passthrough, to indicate that we expect that we pass through
+    to the normal comint function.
+  - 'both to indicate that we expected a password to be sent as well
+    as a prompt to passed through.
+  - nil, to indicate that nothing happens, including no passthrough.
+  - a string to indicate that the string is sent to the process
+    as a password.
+ACTION is the body of the test."
+  `(let ((sent-password)
+         (input-called 0)
+         (comint-watch-called 0))
+     (with-temp-buffer
+       (cl-letf ((comint-input-sender (lambda (_ password) (incf input-called) 
(setq sent-password password)))
+                 ((symbol-function 'comint-watch-for-password-prompt) (lambda 
(_) (incf comint-watch-called)))
+                 (sql-product 'sqltest)
+                 (sql-product-alist '((sqltest
+                                       :name "SqlTest"
+                                       :sql-password-accepted-via-comint t))))
+         ,@action))
+
+     (cond ((eq ,expected 'passthrough)
+            (should (= 1 comint-watch-called))
+            (should (= 0 input-called)))
+           ((eq ,expected 'both)
+            (should (= 1 comint-watch-called))
+            (should (= 1 input-called)))
+           ((null ,expected)
+            (should (= 0 comint-watch-called))
+            (should (= 0 input-called)))
+           ((stringp ,expected)
+            (should (string-equal ,expected sent-password))
+            (should (= 0 comint-watch-called))))))
+
+(ert-deftest sql-tests-watch-for-password-prompt-no-password ()
+  (sql-watch-test-harness
+   'passthrough
+   (setq sql-password nil)
+   (sql-watch-for-password-prompt "Password:"))
+  (sql-watch-test-harness
+   'passthrough
+   (setq sql-password "")
+   (sql-watch-for-password-prompt "Password:")))
+
+(ert-deftest sql-tests-watch-for-password-prompt-right-prompt ()
+  (sql-watch-test-harness
+   nil
+   (setq sql-password "password")
+   (sql-watch-for-password-prompt "SQL> ")))
+
+(ert-deftest sql-tests-watch-for-password-prompt-second-password ()
+  ;; The harness itself makes sure we don't send the password more
+  ;; than once.
+  (sql-watch-test-harness
+   'both
+   (setq sql-password "password")
+   (sql-watch-for-password-prompt "Password:")
+   (sql-watch-for-password-prompt "Password:")))
 
 (provide 'sql-tests)
 ;;; sql-tests.el ends here
-- 
2.20.1 (Apple Git-117)


Michael Mauger <mmauger@protonmail.com> writes:

> On Saturday, November 2, 2019 1:10 AM, Andrew Hyatt <ahyatt@gmail.com> wrote:
>
>> Michael Mauger mmauger@protonmail.com writes:
>>
>> > On Sunday, October 20, 2019 8:56 PM, Andrew Hyatt ahyatt@gmail.com wrote:
>> >
>>
>> Your advice is good, but following it led me to some complexity I can't
>> seem to get away from. Perhaps you have some insight, so let me explain.
>> The issue is that, yes, I can not advise the comint function. However,
>> if I supply my own function, then I have to remove the
>> comint-watch-for-password-prompt, supply my own function, then restore
>> it when the user has entered their password (so it can handle subsequent
>> password entries). This juggling of the normal
>> comint-watch-for-password-prompt method, plus the fact that we basically
>> have to reimplement part of it, gives me pause - I think it's probably
>> too hacky a solution.
>>
>> There's a few ways out. We could introduce a variable used in
>> sql-product-alist that tells SQL not to prompt for a password because
>> the db will just get it via the comint password function. That would
>> probably work well, but it wouldn't store the sql-password at all, that
>> variable would be unused. Maybe that's OK, maybe not - I don't have a
>> good sense for it.
>>
>> Or, we could make this auto-password-supplying per-buffer a part of
>> comint itself. That would widen the scope of the fix, but it would
>> probably be the best of both functionality and simplicity.
>>
>> What do you think?
>>
>
> I totally understand the complexity, but I don't think it has too be too
> complicated to address.
>
> First the sql.el only solution: If the sql-comint function decides to pass
> the password via stdin then it can set a buffer-local flag indicating this
> and then replace `coming-watch-for-password-prompt' on the
> `comint-output-filter-functions' list with the sql version of the function.
> The sql password function would be something along the lines of:
>
>     ;; TOTALLY NOT TESTED
>     (defun sql-watch-for-password-prompt (string)
>       "blah blah ;)"
>       (if sql-will-prompt-for-password
>           ;; (based on comint-watch-for-password-prompt)  vvv
>           (when (let ((case-fold-search t))
>                   (string-match (or (sql-get-product-feature sql-product 
> 'password-prompt-regexp string)
>                                     comint-password-prompt-regexp)))
>             (when (string-match "^[ \n\r\t\v\f\b\a]+" string)
>               (setq string (replace-match "" t t string)))
>             (let ((comint--prompt-recursion-depth (1+ 
> comint--prompt-recursion-depth)))
>               (if (> comint--prompt-recursion-depth 10)
>                   (message "Password prompt recursion too deep")
>                 ;;; ^^^
>                 ;;; automagically provide the password
>                 (let ((proc (get-buffer-process (current-buffer))))
>                   (when proc
>                     (funcall comint-input-sender proc sql-password))))))
>         ;; Back to default behavior
>         (comint-watch-for-password-prompt string))
>       ;; Make sure we don't supply again
>       (setq-local sql-will-prompt-password nil))
>
> That should get you close without too much difficulty. Of course, it requires 
> a
> that a password-prompt-regexp feature is defined for the sql product and that 
> the
> sql-comint function defines a buffer-local flag 
> `sql-will-prompt-for-password' in
> it is deferring to stdin.
>
> The other solution would involve modifying comint to call a hook if set to 
> supply
> a password or nil. This would probably be a simpler change but may get more
> broader attention. When the hook function is not set or returns nil then do 
> the
> default behavior of calling `comint-send-invisible' otherwise just send the 
> password
>
> There are some edge cases here, but this hopefully helps. Also, obviously, 
> test cases
> are needed given that if this breaks, we break the sql interactive world!
>
> --
> MICHAEL@MAUGER.COM // FSF and EFF member // GNU Emacs sql.el maintainer