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

[Andrew Hyatt]

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