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

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

[Jari Aalto]

Package: emacs
Version: 23.2+1-7
Severity: serious
Tags: security

There is a big security problem with sql.el:

    M-x sql-mysql
    <Fill in the connection details: user, password ...>

At command line, anyone in multi-user environment can dig out the
passwords:

   $ ps -ef -o user,pid,args | grep mysql       # ps(1) under SUN/Solaris
   foo  9599 /usr/local/bin/mysql --user=foo --password=123456 --host=db.example.com
   bar  3732 /usr/local/bin/mysql --user=bar --password=abcdef --host=db.example.com

Jari

P.S mysql(1) mentions that you can set database options in ~/.my.cnf file.

MySQL case, there is in manual page:

-- System Information
Debian Release: wheezy/sid
  APT Prefers testing
  APT policy: (990, testing) (500, unstable) (1, experimental)
Architecture: amd64
Kernel: Linux picasso 2.6.32-5-amd64 #1 SMP Wed Jan 12 03:40:32 UTC 2011 x86_64 GNU/Linux
Locale: LANG=en_US.UTF-8, LC_ALL=

-- Versions of packages `emacs depends on'.
Depends:
emacs23         23.2+1-7        GNU Emacs is the extensible self-documenting
emacs23-lucid   23.2+1-7        GNU Emacs is the extensible self-documenting
emacs23-nox     23.2+1-7        GNU Emacs is the extensible self-documenting

bug#8427: (no subject)

[Michael Mauger]

[-- Attachment #1: Type: text/plain, Size: 975 bytes --]

This is not a problem with just sql-mysql, its an issue with all database products that require a password.  MySql is one of the few that covers their tracks after they start up. When sql.el starts up one of these product interpreters that require a password, it embeds the password in the command line.  If the operating system, such as GNU/Linux, displays the full command line of executing processes, the vulnerability exists.

The alternative is to rely upon the operating system's authentication and authorization so that explicit credentials do not need to be passed to the command interpreter on the command line.  The one other solution provided by a couple of database products allow the credentials to be sent via an I/O channel which would hide them from prying eyes, but may be more difficult to support cross platform.

I'm open to including a warning about the potential vulnerability -- wording suggestions appreciated.  Alternative solutions also welcome.

[-- Attachment #2: Type: text/html, Size: 1188 bytes --]

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

[Glenn Morris]

Jari Aalto wrote:

> There is a big security problem with sql.el:
>
>     M-x sql-mysql
>     <Fill in the connection details: user, password ...>
>
> At command line, anyone in multi-user environment can dig out the
> passwords:
>
>    $ ps -ef -o user,pid,args | grep mysql       # ps(1) under SUN/Solaris
>    foo  9599 /usr/local/bin/mysql --user=foo --password=123456 --host=db.example.com
>    bar  3732 /usr/local/bin/mysql --user=bar --password=abcdef --host=db.example.com

Apparently, no they cannot, since mysql replaces the password characters
with x's:

http://www.lenzg.net/archives/256-basic-mysql-security-providing-passwords-on-the-command-line.html

I tested it and it is so hidden for me.


Also, with recent Linux kernels, you can enable the procfs "hidepid"
feature to prevent this entire class of information leakage.


So I don't think Emacs needs to do anything but maybe add a warning
statement to the doc string.

Downgrading bug severity accordingly.

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

[Stefan Monnier]

> Apparently, no they cannot, since mysql replaces the password characters
> with x's:

Of course, that still leaves the chars exposed during a short time window.


        Stefan

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

[Andrew Hyatt]

This is fairly easy to fix - mysql can check to see if the user entered
a blank for the password prompt, and instead of not sending a password,
send just the "--password" argument so the user can enter it into the
process instead of the command line.  I have a fix ready to check in
that works for mysql (I'm not sure which other products support that).

Alternatively, we can just have a variable that controls whether
passwords are asked for on the command line at all (if sql-password is
unset), which could default to nil, making the security better by
default.

BTW, I guess the attack here is that another user process can use
something like strace to snoop on emacs's child processeses and obtain
the mysql password?

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> Apparently, no they cannot, since mysql replaces the password characters
>> with x's:
>
> Of course, that still leaves the chars exposed during a short time window.
>
>
>         Stefan

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

[Stefan Kangas]

Hi Andrew,

Andrew Hyatt <ahyatt@gmail.com> writes:

> This is fairly easy to fix - mysql can check to see if the user entered
> a blank for the password prompt, and instead of not sending a password,
> send just the "--password" argument so the user can enter it into the
> process instead of the command line.  I have a fix ready to check in
> that works for mysql (I'm not sure which other products support that).

I think using an empty "--pasword" parameter sounds like the right fix.
That makes mysql prompt for the password, and we could supply it there
instead.  I guess that's what you meant?

Could you perhaps send your patch here for review?

> Alternatively, we can just have a variable that controls whether
> passwords are asked for on the command line at all (if sql-password is
> unset), which could default to nil, making the security better by
> default.

I'm not sure what this means, but I guess the above fix should be
enough.  Perhaps I'm missing something.

> BTW, I guess the attack here is that another user process can use
> something like strace to snoop on emacs's child processeses and obtain
> the mysql password?

Well, according to the threads linked earlier this can still be a
problem on Solaris, where the password is visible to all users if they
just run "ps".  Perhaps it's been fixed since whenever these comments
were written though...

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>>> Apparently, no they cannot, since mysql replaces the password characters
>>> with x's:
>>
>> Of course, that still leaves the chars exposed during a short time window.

And as Stefan explains here the password is still exposed during a
short time window even on GNU/Linux.  AFAIU, it's a possible race
attack which it would be nice to avoid.

Best regards,
Stefan Kangas

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

[Andrew Hyatt]

[-- Attachment #1: Type: text/plain, Size: 2354 bytes --]

On Sat, Oct 5, 2019 at 11:28 PM Stefan Kangas <stefan@marxist.se> wrote:

> Hi Andrew,
>
> Andrew Hyatt <ahyatt@gmail.com> writes:
>
> > This is fairly easy to fix - mysql can check to see if the user entered
> > a blank for the password prompt, and instead of not sending a password,
> > send just the "--password" argument so the user can enter it into the
> > process instead of the command line.  I have a fix ready to check in
> > that works for mysql (I'm not sure which other products support that).
>
> I think using an empty "--pasword" parameter sounds like the right fix.
> That makes mysql prompt for the password, and we could supply it there
> instead.  I guess that's what you meant?
>
> Could you perhaps send your patch here for review?
>

I no longer know where my changes are.   It's been a while.  But I think I
can probably recreate them, which I'll try to do this week.


>
> > Alternatively, we can just have a variable that controls whether
> > passwords are asked for on the command line at all (if sql-password is
> > unset), which could default to nil, making the security better by
> > default.
>
> I'm not sure what this means, but I guess the above fix should be
> enough.  Perhaps I'm missing something.
>

The idea is that instead of connecting with the --password arg, it can be
left out entirely, in which case the program should ask for it (which is
secure).


>
> > BTW, I guess the attack here is that another user process can use
> > something like strace to snoop on emacs's child processeses and obtain
> > the mysql password?
>
> Well, according to the threads linked earlier this can still be a
> problem on Solaris, where the password is visible to all users if they
> just run "ps".  Perhaps it's been fixed since whenever these comments
> were written though...


> > Stefan Monnier <monnier@iro.umontreal.ca> writes:
> >
> >>> Apparently, no they cannot, since mysql replaces the password
> characters
> >>> with x's:
> >>
> >> Of course, that still leaves the chars exposed during a short time
> window.
>
> And as Stefan explains here the password is still exposed during a
> short time window even on GNU/Linux.  AFAIU, it's a possible race
> attack which it would be nice to avoid.
>

Yes, I think the solutions I presented should fix this.  Stay tuned for a
patch.


>
> Best regards,
> Stefan Kangas
>

[-- Attachment #2: Type: text/html, Size: 3669 bytes --]

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

[Stefan Kangas]

Andrew Hyatt <ahyatt@gmail.com> writes:

>> Could you perhaps send your patch here for review?
>
> I no longer know where my changes are.   It's been a while.  But I think I can probably recreate them, which I'll try to do this week.
[...]
> The idea is that instead of connecting with the --password arg, it can be left out entirely, in which case the program should ask for it (which is secure).

Sounds good, thanks.

Best regards,
Stefan Kangas

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

[Stefan Kangas]

[-- Attachment #1: Type: text/plain, Size: 1300 bytes --]

---------- Forwarded message ---------
From: Andrew Hyatt <ahyatt@gmail.com>
Date: lör 19 okt. 2019 kl 04:07
Subject: Re: bug#8427: [SECURITY] sql.el -- comint process passwords
are leaked to ps(1) listing
To: Stefan Kangas <stefan@marxist.se>

I'm attaching the fix.  The fix for MySQL was fairly straightforward.  I
tried it out, and it works.  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.

If this looks good to you, I will submit it (I have commit access).


Stefan Kangas <stefan@marxist.se> writes:

> Andrew Hyatt <ahyatt@gmail.com> writes:
>
>>> Could you perhaps send your patch here for review?
>>
>> I no longer know where my changes are.   It's been a while.  But I think I can probably recreate them, which I'll try to do this week.
> [...]
>> The idea is that instead of connecting with the --password arg, it can be left out entirely, in which case the program should ask for it (which is secure).
>
> Sounds good, thanks.
>
> Best regards,
> Stefan Kangas

[-- Attachment #2: 0001-Enable-password-less-connections-for-sql-where-possi.patch --]
[-- Type: application/x-patch, Size: 1933 bytes --]

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

[Stefan Kangas]

(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

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

[Andrew Hyatt]

[-- Attachment #1: Type: text/plain, Size: 397 bytes --]


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.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Draft 2 of mysql patch --]
[-- Type: text/x-patch, Size: 5512 bytes --]

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


[-- Attachment #3: Type: text/plain, Size: 1467 bytes --]


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

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

[Michael Mauger via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/plain, Size: 4682 bytes --]

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Sunday, October 20, 2019 8:56 PM, Andrew Hyatt <ahyatt@gmail.com> 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 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

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.

--
MICHAEL@MAUGER.COM // FSF and EFF member // GNU Emacs sql.el maintainer

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Enable-password-less-connections-for-sql-where-possi.patch --]
[-- Type: text/x-patch; name="0001-Enable-password-less-connections-for-sql-where-possi.patch", Size: 5512 bytes --]

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

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

[Andrew Hyatt]

Michael Mauger <mmauger@protonmail.com> writes:

> ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
> On Sunday, October 20, 2019 8:56 PM, Andrew Hyatt <ahyatt@gmail.com> 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 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
>
> 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.

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?

>
> * 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.
>
> --
> MICHAEL@MAUGER.COM // FSF and EFF member // GNU Emacs sql.el maintainer

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

[Michael Mauger via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

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

[Andrew Hyatt]

[-- Attachment #1: Type: text/plain, Size: 170 bytes --]


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.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Enable-passwords-to-be-sent-in-process-when-possible.patch --]
[-- Type: text/x-patch, Size: 10234 bytes --]

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)


[-- Attachment #3: Type: text/plain, Size: 4090 bytes --]



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

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

[Andrew Hyatt]

[-- Attachment #1: Type: text/plain, Size: 4692 bytes --]

Any input on this?  I believe this fixes the issue, and would prefer to
revise this while I still remember the details.  I'm happy to submit this
as well.

On Mon, Nov 11, 2019 at 12:31 AM Andrew Hyatt <ahyatt@gmail.com> wrote:

>
> 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.
>
>
>
> 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
>

[-- Attachment #2: Type: text/html, Size: 6102 bytes --]

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

[Michael Mauger via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/plain, Size: 1434 bytes --]

-------- Original Message --------
On Dec 15, 2019, 11:59 PM, Andrew Hyatt < ahyatt@gmail.com> wrote:
> Any input on this?  I believe this fixes the issue, and would prefer to
> revise this while I still remember the details.  I'm happy to submit this
> as well.

>> On Mon, Nov 11, 2019 at 12:31 AM Andrew Hyatt <ahyatt@gmail.com> wrote:

>> 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.

I apologise for not getting back to you sooner-- a new job and the holidays have consumed much of my time. My initial look at your latest patch raised some concerns but I haven't done any deeper look yet. I'll try to take a look in the next week or so. If you don't hear back from me after the new year, then let's merge it and we'll address the issues from there. (I know I mentioned this before but I don't remember the status-- do you have your copyright paperwork all set for Emacs contributions?)

I think my thought was that it may make sense to push some of this back onto comint rather than a convoluted sql-only solution, but that may require some more negotiation. As I looked at it, using a comint hook might serve auth services as well.

Sorry about the silence, you have not been forgotten just buried in end-of-year turmoil :)

--
MICHAEL@MAUGER.COM // FSF and EFF member // GNU Emacs sql.el maintainer

[-- Attachment #2: Type: text/html, Size: 1543 bytes --]

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

[Andrew Hyatt]

[-- Attachment #1: Type: text/plain, Size: 1972 bytes --]

Hi Michael,

I'm happy to merge this in.  I have FSF paperwork done and already have
commit access.

However, I agree with you about pushing logic into comint.  As I mentioned
before, it would help simplify the logic here.  It might be best to not
check this in and see what an alternate solution might be first, based on
comint.  I can work on that soon and get a patch out in the next week or so.

On Mon, Dec 16, 2019 at 10:12 AM Michael Mauger <mmauger@protonmail.com>
wrote:

>
> -------- Original Message --------
> On Dec 15, 2019, 11:59 PM, Andrew Hyatt < ahyatt@gmail.com> wrote:
> > Any input on this?  I believe this fixes the issue, and would prefer to
> > revise this while I still remember the details.  I'm happy to submit this
> > as well.
>
> >> On Mon, Nov 11, 2019 at 12:31 AM Andrew Hyatt <ahyatt@gmail.com> wrote:
>
> >> 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.
>
> I apologise for not getting back to you sooner-- a new job and the
> holidays have consumed much of my time. My initial look at your latest
> patch raised some concerns but I haven't done any deeper look yet. I'll try
> to take a look in the next week or so. If you don't hear back from me after
> the new year, then let's merge it and we'll address the issues from there.
> (I know I mentioned this before but I don't remember the status-- do you
> have your copyright paperwork all set for Emacs contributions?)
>
> I think my thought was that it may make sense to push some of this back
> onto comint rather than a convoluted sql-only solution, but that may
> require some more negotiation. As I looked at it, using a comint hook might
> serve auth services as well.
>
> Sorry about the silence, you have not been forgotten just buried in
> end-of-year turmoil :)
>
> --
> MICHAEL@MAUGER.COM // FSF and EFF member // GNU Emacs sql.el maintainer

[-- Attachment #2: Type: text/html, Size: 2563 bytes --]

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

[Michael Mauger via Bug reports for GNU Emacs, the Swiss army knife of text editors]

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Wednesday, December 18, 2019 6:15 AM, Andrew Hyatt <ahyatt@gmail.com> wrote:

> Hi Michael,
>
> I'm happy to merge this in.  I have FSF paperwork done and already have commit access.
>
> However, I agree with you about pushing logic into comint.  As I mentioned before, it would help simplify the logic here.  It might be best to not check this in and see what an alternate solution might be first, based on comint.  I can work on that soon and get a patch out in the next week or so.
>
> On Mon, Dec 16, 2019 at 10:12 AM Michael Mauger <mmauger@protonmail.com> wrote:
>
> > -------- Original Message --------
> > On Dec 15, 2019, 11:59 PM, Andrew Hyatt < ahyatt@gmail.com> wrote:
> > > Any input on this?  I believe this fixes the issue, and would prefer to
> > > revise this while I still remember the details.  I'm happy to submit this
> > > as well.
> >

I had a chance to look at this last night; I've had a couple of days away from home and took advantage of it. 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.

*** /usr/local/share/emacs/27.0.50/lisp/comint.el	2019-12-18 07:26:14.268274791 -0500
--- /home/michael/my-config/user-lisp/override/comint.el	2019-12-17 23:10:08.433852481 -0500
***************
*** 2356,2361 ****
--- 2356,2368 ----
  ;; saved -- typically passwords to ftp, telnet, or somesuch.
  ;; Just enter m-x comint-send-invisible and type in your line.

+ (defvar comint-password-function nil
+   "Abnormal hook run when prompted for a password.
+ This function gets one argument, a string containing the prompt.
+ It may return a string containing the password, or nil if normal
+ password prompting should occur.")
+ (put 'comint-password-function 'permanent-local t)
+
  (defun comint-send-invisible (&optional prompt)
    "Read a string without echoing.
  Then send it to the process running in the current buffer.
***************
*** 2370,2377 ****
  	   (format "(In buffer %s) "
  		   (current-buffer)))))
      (if proc
! 	(let ((str (read-passwd (concat prefix
! 					(or prompt "Non-echoed text: ")))))
  	  (if (stringp str)
  	      (progn
  		(comint-snapshot-last-prompt)
--- 2377,2389 ----
  	   (format "(In buffer %s) "
  		   (current-buffer)))))
      (if proc
! 	(let ((prefix-prompt (concat prefix
! 				     (or prompt "Non-echoed text: ")))
! 	      str)
! 	  (when comint-password-function
! 	    (setq str (funcall comint-password-function prefix-prompt)))
! 	  (unless str
! 	    (setq str (read-passwd prefix-prompt)))
  	  (if (stringp str)
  	      (progn
  		(comint-snapshot-last-prompt)


Let me know your thoughts

--
MICHAEL@MAUGER.COM // FSF and EFF member // GNU Emacs sql.el maintainer

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

[Eli Zaretskii]

> Cc: "8427@debbugs.gnu.org" <8427@debbugs.gnu.org>,
>  Stefan Kangas <stefan@marxist.se>
> Date: Wed, 18 Dec 2019 12:45:27 +0000
> From: Michael Mauger via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> I had a chance to look at this last night; I've had a couple of days away from home and took advantage of it. 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.

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

[Michael Mauger via Bug reports for GNU Emacs, the Swiss army knife of text editors]

‐‐‐‐‐‐‐ 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>

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

[Andrew Hyatt]

[-- Attachment #1: Type: text/plain, Size: 166 bytes --]


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.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: bugfix for 8427, using new comint hook --]
[-- Type: text/x-patch, Size: 3846 bytes --]

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)


[-- Attachment #3: Type: text/plain, Size: 1002 bytes --]


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>

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

[Michael Albinus]

Andrew Hyatt <ahyatt@gmail.com> writes:

> --- a/etc/NEWS
> +++ b/etc/NEWS
>
> +---
> +**** 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.

I would say non-nil instead of true. Or do you mean t?

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

[Andrew Hyatt]

[-- Attachment #1: Type: text/plain, Size: 603 bytes --]

I meant a true value, I agree, non-nil is a better way to say that.

Shall I just make that change and check in?

On Mon, Dec 30, 2019 at 1:34 PM Michael Albinus <michael.albinus@gmx.de>
wrote:

> Andrew Hyatt <ahyatt@gmail.com> writes:
>
> > --- a/etc/NEWS
> > +++ b/etc/NEWS
> >
> > +---
> > +**** 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.
>
> I would say non-nil instead of true. Or do you mean t?
>

[-- Attachment #2: Type: text/html, Size: 1037 bytes --]

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

[Eli Zaretskii]

> Date: Mon, 30 Dec 2019 14:26:21 -0500
> Cc: Michael Mauger <mmauger@protonmail.com>,
>  "8427@debbugs.gnu.org" <8427@debbugs.gnu.org>,
>  "stefan@marxist.se" <stefan@marxist.se>
> 
> Shall I just make that change and check in?

Please wait for a while, I'd like Michael Mauger to review this.

Thanks.

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

[Michael Mauger via Bug reports for GNU Emacs, the Swiss army knife of text editors]

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Monday, December 30, 2019 2:39 PM, Eli Zaretskii <eliz@gnu.org> wrote:

> > Date: Mon, 30 Dec 2019 14:26:21 -0500
>
> > Cc: Michael Mauger mmauger@protonmail.com,
> > "8427@debbugs.gnu.org" 8427@debbugs.gnu.org,
> > "stefan@marxist.se" stefan@marxist.se
> > Shall I just make that change and check in?
>
> Please wait for a while, I'd like Michael Mauger to review this.
>
> Thanks.

I will take a look after New Years. Thank you, Andrew and Eli.

--
MICHAEL@MAUGER.COM // FSF and EFF member // GNU Emacs sql.el maintainer

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

[Lars Ingebrigtsen]

Michael Mauger <mmauger@protonmail.com> writes:

>> Please wait for a while, I'd like Michael Mauger to review this.
>>
>> Thanks.
>
> I will take a look after New Years. Thank you, Andrew and Eli.

Michael, have you had time to look at the patches?  I had a brief skim,
and they looked reasonable to me, but I have neither read them in-depth
nor tried them.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

[Stefan Kangas]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Michael Mauger <mmauger@protonmail.com> writes:
>
>>> Please wait for a while, I'd like Michael Mauger to review this.
>>>
>>> Thanks.
>>
>> I will take a look after New Years. Thank you, Andrew and Eli.
>
> Michael, have you had time to look at the patches?  I had a brief skim,
> and they looked reasonable to me, but I have neither read them in-depth
> nor tried them.

Any news about this?  It would be great to get this into Emacs 28.

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

[Michael Mauger via Bug reports for GNU Emacs, the Swiss army knife of text editors]

On Tuesday, October 12th, 2021 at 1:05 AM, Stefan Kangas <stefan@marxist.se> wrote:

> Lars Ingebrigtsen larsi@gnus.org writes:
> > Michael Mauger mmauger@protonmail.com writes:
> > > > Please wait for a while, I'd like Michael Mauger to review this.
> > > >
> > > > Thanks.
> > > I will take a look after New Years. Thank you, Andrew and Eli.
> > Michael, have you had time to look at the patches? I had a brief skim,
> > and they looked reasonable to me, but I have neither read them in-depth
> > nor tried them.
> Any news about this? It would be great to get this into Emacs 28.

I'm fine with this.  I remember this vaguely and think we are all set. Let's
get it committed and we'll give it some attention/testing before the release

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

[Stefan Kangas]

Michael Mauger <mmauger@protonmail.com> writes:

> I'm fine with this.  I remember this vaguely and think we are all set. Let's
> get it committed and we'll give it some attention/testing before the release

Thanks, I guess we need one of our maintainers to approve this before
pushing this to emacs-28.  Otherwise, it should go to master.

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

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Wed, 13 Oct 2021 10:47:37 -0700
> Cc: Lars Ingebrigtsen <larsi@gnus.org>, "michael.albinus@gmx.de" <michael.albinus@gmx.de>, Eli Zaretskii <eliz@gnu.org>, 
> 	"8427@debbugs.gnu.org" <8427@debbugs.gnu.org>, Andrew Hyatt <ahyatt@gmail.com>
> 
> Michael Mauger <mmauger@protonmail.com> writes:
> 
> > I'm fine with this.  I remember this vaguely and think we are all set. Let's
> > get it committed and we'll give it some attention/testing before the release
> 
> Thanks, I guess we need one of our maintainers to approve this before
> pushing this to emacs-28.  Otherwise, it should go to master.

I'm okay with installing this on the release branch if Michael thinks
it's safe enough.

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

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

> I'm okay with installing this on the release branch if Michael thinks
> it's safe enough.

Michael, could you please push this to emacs-28 or master as you prefer?

Alternatively, just tell us where you'd like it to land and me or
someone else can push it.

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

[Michael Mauger via Bug reports for GNU Emacs, the Swiss army knife of text editors]

On Wednesday, October 13th, 2021 at 5:26 PM, Stefan Kangas <stefan@marxist.se> wrote:

> Eli Zaretskii eliz@gnu.org writes:
>
> > I'm okay with installing this on the release branch if Michael thinks
> > it's safe enough.
>
> Michael, could you please push this to emacs-28 or master as you prefer?
> Alternatively, just tell us where you'd like it to land and me or
> someone else can push it.

I've pushed to master for now. I'll test this week and decide then whether I push to emacs-28.

I think its fine and safe, but I effed up for emacs-27, so I'm a little cautious...

--
MICHAEL@MAUGER.COM // FSF and EFF member // GNU Emacs sql.el maintainer

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

[Eli Zaretskii]

> Date: Tue, 19 Oct 2021 04:37:23 +0000
> From: Michael Mauger <mmauger@protonmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>, larsi@gnus.org, michael.albinus@gmx.de, 8427@debbugs.gnu.org, ahyatt@gmail.com
> 
> I've pushed to master for now. I'll test this week and decide then whether I push to emacs-28.

If you decide to backport to emacs-28, please cherry-pick from master,
instead of making a separate independent commit, because cherry-picks
are automatically skipped when emacs-28 branch is merged to master
(which happens almost daily these days).

Thanks.

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

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

>> I've pushed to master for now. I'll test this week and decide then whether I push to emacs-28.
>
> If you decide to backport to emacs-28, please cherry-pick from master,
> instead of making a separate independent commit, because cherry-picks
> are automatically skipped when emacs-28 branch is merged to master
> (which happens almost daily these days).

Except the etc/NEWS change. This file differs heavily between emacs-28
and master branches. Furthermore, I have fixed some oddities in that
file wrt this change.

> Thanks.

Best regards, Michael.

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

[Stefan Kangas]

close 8427 29.1
thanks

Michael Mauger <mmauger@protonmail.com> writes:

> I've pushed to master for now. I'll test this week and decide then
> whether I push to emacs-28.

Since the fix has been installed on master, I'm closing this bug.  We
can still discuss here for up to 30 days before the bug is archived, in
case there is anything relating to cherry-picking the patch to the
emacs-28 branch.

Thank you Andrew for writing the patch and Michael for reviewing and
installing it!