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

From: Michael Mauger <[email-masked]>
Date: Mon, 21 Oct 2019 20:33:09 +0000

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Sunday, October 20, 2019 8:56 PM, Andrew Hyatt <[email-masked]> wrote:

>
>
> 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.
>
> Stefan Kangas [email-masked] writes:
>
> > (Please keep the bug address in Cc.)
> > Andrew Hyatt [email-masked] 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

I have tried a couple of different versions of this in the past but have found a lot of corner cases that made me back off.

Some thoughts:

* The login-params function will set `sql-password' to nil if it isn't a parameter being prompted for and is not set otherwise. If it is prompted for and empty the variable will be an empty string not nil. We need some test cases written to confirm that the behavior is as we expect. Small shell scripts can be created to simulate the SQL processor for the general flow. The test scripts should be included in the commit for this feature.

* Only supply the password using the comint password filter if support for passing the password on stdin is supported and expected in this instance. This would probably be a flag set in the `sql-PRODUCT-comint-func' (based on the command line logic) and set as a buffer local in the buffer. The comint filter would then check the flag before trying to stuff the password into the stream. That avoids sending a database password to a prompt that is for other purposes. Also does this have to be advice to the comint filter or just another filter installed on the comint hook? (Policy is that standard Emacs packages do not use advice and the hook is present and used by the existing hook.)

* Only send the password to the first time a password is asked for. Some interactive sql processes allow changing the connection mid-session and the password for the original username may not be appropriate for the new connection they made. This is especially true in enterprise environments where password failures can be set to disable the database user.

* Please do not alter indenting of existing code not involved in the change; the indentation is deliberate in some cases and the spurious changes just generate noise in the diffs. Thanks.

* Are we adding a flag to the `sql-product-alist' to indicate that passwords may be passed via stdin? I would recommend that we do so because that way it can be globally disabled if the environment calls for it. For example, a user may want to supply it on the command line because it is not a concen in their environment. Some database products support passing the password this way, but also alter the command line parameters to mask out the actual password so that the `ps' exposure is fairly small.

Thanks working on this but I'm still concerned that we could break existing use of sql-interactive-mode unintentionally.

--
[email-masked] // FSF and EFF member // GNU Emacs sql.el maintainer
From 610d4d8c9bb5f04a86afc8a63b671bd035d24e36 Mon Sep 17 00:00:00 2001
From: Andrew Hyatt <[email-masked]>
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