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

[Andrew Hyatt]

Thanks for the insightful comments - yes, everything you say makes
sense. I've implemented what you describe. However, I'm a little unsure
of this one - I had to advise a comint primitive and even re-implement
part of an existing comint function. It feels like comint should perhaps
have a way to do this sort of thing within itself, but I couldn't find
any.

I've attached the latest revision.

>From 610d4d8c9bb5f04a86afc8a63b671bd035d24e36 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 password-less connections for sql where possible.

* lisp/progmodes/sql.el (sql-comint-mysql):
  When a blank password is provided (not entered by the user), send an
  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.

  We also watch for the password inside the SQL process and
  automatically fill it with `sql-password' (if it exists).
---
 lisp/progmodes/sql.el | 42 +++++++++++++++++++++++++++++++-----------
 1 file changed, 31 insertions(+), 11 deletions(-)

diff --git a/lisp/progmodes/sql.el b/lisp/progmodes/sql.el
index b17364b08f..c453de382d 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))
@@ -4664,8 +4667,8 @@ the call to \\[sql-product-interactive] with
                     (sql-database   (default-value 'sql-database))
                     (sql-port       (default-value 'sql-port))
                     (default-directory
-                                    (or sql-default-directory
-                                        default-directory)))
+                      (or sql-default-directory
+                          default-directory)))
 
                 ;; The password wallet returns a function which supplies the 
password.
                 (when (functionp sql-password)
@@ -4681,9 +4684,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 +4736,27 @@ the call to \\[sql-product-interactive] with
               (get-buffer new-sqli-buffer)))))
     (user-error "No default SQL product defined: set `sql-product'")))
 
+(define-advice comint-watch-for-password-prompt
+    (:around (inner-func string) sql-password-autopopulate)
+  "Intercept password prompts when we know the password. This
+must also do the job of detecting password prompts.  STRING is
+the potential password prompt.  INNER-FUNC is the previous
+definition of comint-watch-for-password-prompt, which is called
+only when there is no prefilled password."
+  (if (and
+       (eq major-mode 'sql-interactive-mode)
+       (not (string= "" sql-password))
+       (let ((case-fold-search t))
+         (string-match comint-password-prompt-regexp string)))
+      (funcall comint-input-sender (get-buffer-process (current-buffer)) 
sql-password)
+    (funcall inner-func 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
@@ -5188,7 +5206,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))
-- 
2.19.0.605.g01d371f741-goog

Stefan Kangas <stefan@marxist.se> writes:

> (Please keep the bug address in Cc.)
>
> Andrew Hyatt <ahyatt@gmail.com> writes:
>
>> I'm attaching the fix.  The fix for MySQL was fairly straightforward.  I
>> tried it out, and it works.
>
> I'm not sure this is the right fix.  How is the user to know that the
> correct thing is to provide an empty password when prompted for it?
> Why do we even prompt for the password then?
>
> Also, what if a user wants to login to an account that has no
> password?  Should we really pass the "--password" parameter in that
> case?  Does that work?
>
> I think something like this would be better:
>
> 1. Keep the password prompt.
> 2. Use the naked "--password" parameter only when the user *has*
> entered a password, and use nothing when the user entered nothing.
> 3. Never use the "--password=<foo>" parameter.
> 4. When mysql prompts for the password, send it to the process
> automatically, without user interaction.
>
>> I looked through sql.el for similar issues,
>> and was able to fix Vertica as well, although I've never heard of
>> Vertica before and couldn't test it out.  Parameters were set according
>> to the docs at
>> https://www.vertica.com/docs/9.2.x/HTML/Content/Authoring/ConnectingToVertica/vsql/CommandLineOptions.htm,
>> which does match the existing code.
>
> Unless someone can test it, perhaps we should leave out the Vertica part?
>
> Thanks for working on this.
>
> Best regards,
> Stefan Kangas