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

[Andrew Hyatt <ahyatt@gmail.com>]

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