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

[Andrew Hyatt]

Thank you, your new comint changes make this change much simpler.  I'm
attaching the new patch, please take a look and let me know if this is
reasonable to submit.

>From cbca5e8b7be1fd1a10773ae0b22e6373e705007a Mon Sep 17 00:00:00 2001
From: Andrew Hyatt <ahyatt@gmail.com>
Date: Mon, 30 Dec 2019 10:09:23 -0500
Subject: [PATCH] Enable sql passwords to be sent in-process when possible.

* lisp/progmodes/sql.el (sql-comint, sql-comint-mysql):

  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: Testing of new password comint hook.
---
 etc/NEWS                         |  6 ++++++
 lisp/progmodes/sql.el            | 15 +++++++++++++++
 test/lisp/progmodes/sql-tests.el | 10 ++++++++++
 3 files changed, 31 insertions(+)

diff --git a/etc/NEWS b/etc/NEWS
index e630bb71fe..06526fb1ae 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1231,6 +1231,12 @@ default values.  If you have existing customizations to 
these
 variables, you should make sure that the new default entry is
 included.
 
+---
+**** sql now supports sending of passwords in-process.
+To improve security, if a sql product has ':password-in-comint' set to
+true, a password supplied via the minibuffer will be sent in-process,
+as opposed to via the command-line.
+
 ---
 *** Connection Wallet
 Database passwords can now by stored in NETRC or JSON data files that
diff --git a/lisp/progmodes/sql.el b/lisp/progmodes/sql.el
index 7a51739c5f..979d311064 100644
--- a/lisp/progmodes/sql.el
+++ b/lisp/progmodes/sql.el
@@ -4733,6 +4733,14 @@ the call to \\[sql-product-interactive] with
               (get-buffer new-sqli-buffer)))))
     (user-error "No default SQL product defined: set `sql-product'")))
 
+(defun sql-comint-automatic-password (_)
+  "Intercept password prompts when we know the password.
+This must also do the job of detecting password prompts."
+  (when (and
+         sql-password
+         (not (string= "" sql-password)))
+    sql-password))
+
 (defun sql-comint (product params &optional buf-name)
   "Set up a comint buffer to run the SQL processor.
 
@@ -4757,6 +4765,13 @@ 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))
+    ;; Set up the automatic population of passwords, if supported.
+    (when (sql-get-product-feature product :password-in-comint)
+      (setq comint-password-function #'sql-comint-automatic-password))
+
     ;; 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)))
diff --git a/test/lisp/progmodes/sql-tests.el b/test/lisp/progmodes/sql-tests.el
index 3ac9fb10e4..2f0a96b6c2 100644
--- a/test/lisp/progmodes/sql-tests.el
+++ b/test/lisp/progmodes/sql-tests.el
@@ -410,6 +410,16 @@ The ACTION will be tested after set-up of PRODUCT."
 
     (kill-buffer "*SQL: exist*")))
 
+(ert-deftest sql-tests-comint-automatic-password ()
+  (let ((sql-password nil))
+    (should-not (sql-comint-automatic-password "Password: ")))
+  (let ((sql-password ""))
+    (should-not (sql-comint-automatic-password "Password: ")))
+  (let ((sql-password "password"))
+    (should (equal "password" (sql-comint-automatic-password "Password: "))))
+  ;; Also, we shouldn't care what the password is - we rely on comint for that.
+  (let ((sql-password "password"))
+    (should (equal "password" (sql-comint-automatic-password "")))))
 
 (provide 'sql-tests)
 ;;; sql-tests.el ends here
-- 
2.20.1 (Apple Git-117)

Michael Mauger <mmauger@protonmail.com> writes:

> ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
> On Wednesday, December 18, 2019 11:57 AM, Eli Zaretskii <eliz@gnu.org> wrote:
>
>> On Wed, 18 Dec 2019 12:45:27 +0000, Michael Mauger wrote:
>> > Below is my first
>> > take on the changes to comint.el needed to add a hook that we
>> > could use in sql.el to supply the password. I think we ought
>> > to run this by emacs-devel and Eli before merging it.
>>
>> I'm okay with adding this hook, but please mention this hook and its
>> rationale in NEWS.
>>
>> Please also feel free to ask on emacs-devel for comments, if you want.
>>
>> Thanks.
>
> I'll put together the patch for comint.el and NEWS and commit it.
>
> Andrew, you can then simplify your sql.el patches appropriately along
> with corresponding NEWS entry and we can review before you commit. Thanks!
>
> --
> MICHAEL@MAUGER.COM // FSF and EFF member // GNU Emacs sql.el 
> maintainer</eliz@gnu.org>