[Django] #32061: Credential leaks on MySQL's dbshell crash
Django
-------------------------------------+-------------------------------------
Reporter: Simon | Owner: nobody
Charette |
Type: Bug | Status: new
Component: Core | Version: 3.1
(Management commands) |
Severity: Normal | Keywords:
Triage Stage: Accepted | Has patch: 1
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
''I reported this issue to the security team on September 9th 2020 but it
was deemed that it was better to handle it in public. On September 30th
Charlie Denton also reported the issue to the security team hence why I'm
finally creating a ticket for this so he also gets credit''
A variant of this issue was initially reported 5 years ago (#24999) where
the reporter mentioned that usage of the --password flag when spawning a
mysql shell process would show up in ps with the password in plain text.
What I'm reporting now is that if the dbshell management crash on backends
that pass the `--password` flag in plain text (e.g. MySQL) the
`CalledProcessError` exception raised will have the following signature
{{{
CalledProcessError: Command '['mycl', '--user=user', '--
password=p4ssword', '--host=host', '--port=3306', 'database']' returned
non-zero exit status 1.
}}}
That's not a big deal when no form of logging is in place but when it's
the case (e.g. Sentry, Elastic, Graylog) it's possible for some systems to
fail at obfuscating the `--password=` part due to the complex nature of
the string.
In order to prevent that from happening I suggest we override
[https://github.com/django/django/blob/9e8edc1e5511f128dec6bcd70a10ebd263b76280/django/db/backends/mysql/client.py#L52
MySQL backend's runshell method] to pass `env={'MYSQL_PWD': PASSWORD}`
instead of using the `--password` flag even if
[https://dev.mysql.com/doc/refman/8.0/en/environment-variables.html it
looks like this is discouraged] (and even meant to be deprecated?) for
reasons similar to the ones mentioned in #24999.
[https://github.com/django/django/blob/5b884d45ac5b76234eca614d90c83b347294c332/django/db/backends/postgresql/client.py#L35-L37
That's the exact approach currently used by the PostgreSQL backend
though].
An alternative could be using the [https://pypi.org/project/pexpect/
pexpect library] to provide the password when prompted for instead but I
fear this solution might not be suitable due to the external dependency it
would incur. I haven't investigated how complex it would be to implement a
cross-platform vendored solution without the use of `pexpect` but I fear
dragons may lurk in the details of dealing with that properly.
--
Ticket URL: <https://code.djangoproject.com/ticket/32061>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Core (Management | Version: 3.1
commands) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Old description:
New description:
''I reported this issue to the security team on September 9th 2020 but it
was deemed that it was better to handle it in public. On September 30th
Charlie Denton also reported the issue to the security team hence why I'm
finally creating a ticket for this so he also gets credit.''
A variant of this issue was initially reported 5 years ago (#24999) where
the reporter mentioned that usage of the `--password` flag when spawning a
mysql shell process would show up in ps with the password in plain text.
What I'm reporting now is that if the dbshell management crash on backends
that pass the `--password` flag in plain text (e.g. MySQL) the
`CalledProcessError` exception raised will have the following signature
{{{
CalledProcessError: Command '['mysql', '--user=user', '--
password=p4ssword', '--host=host', '--port=3306', 'database']' returned
non-zero exit status 1.
}}}
That's not a big deal when no form of logging is in place but when it's
the case (e.g. Sentry, Elastic, Graylog) it's possible for some systems to
fail at obfuscating the `--password=` part due to the complex nature of
the string.
In order to prevent that from happening I suggest we override
[https://github.com/django/django/blob/9e8edc1e5511f128dec6bcd70a10ebd263b76280/django/db/backends/mysql/client.py#L52
MySQL backend's runshell method] to pass `env={'MYSQL_PWD': PASSWORD}`
instead of using the `--password` flag even if
[https://dev.mysql.com/doc/refman/8.0/en/environment-variables.html it
looks like this is discouraged] (and even meant to be deprecated?) for
reasons similar to the ones mentioned in #24999.
[https://github.com/django/django/blob/5b884d45ac5b76234eca614d90c83b347294c332/django/db/backends/postgresql/client.py#L35-L37
That's the exact approach currently used by the PostgreSQL backend
though].
An alternative could be using the [https://pypi.org/project/pexpect/
pexpect library] to provide the password when prompted for instead but I
fear this solution might not be suitable due to the external dependency it
would incur. I haven't investigated how complex it would be to implement a
cross-platform vendored solution without the use of `pexpect` but I fear
dragons may lurk in the details of dealing with that properly.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/32061#comment:1>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Core (Management | Version: 3.1
commands) |
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"bbe6fbb8768e8fb1aecb96d51c049d7ceaf802d3" bbe6fbb8]:
{{{
#!CommitTicketReference repository=""
revision="bbe6fbb8768e8fb1aecb96d51c049d7ceaf802d3"
Refs #32061 -- Unified DatabaseClient.runshell() in db backends.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32061#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: Simon Charette | Owner: nobody
Component: Core (Management | Version: 3.1
commands) |
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
--
Ticket URL: <https://code.djangoproject.com/ticket/32061#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: Simon Charette | Owner: nobody
Component: Core (Management | Version: 3.1
commands) |
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"eb25fdb620f9fc3e2bcde916f9db2c06f53a3cd7" eb25fdb6]:
{{{
#!CommitTicketReference repository=""
revision="eb25fdb620f9fc3e2bcde916f9db2c06f53a3cd7"
Refs #32061 -- Added test for dbshell password leak on PostgreSQL.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32061#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Simon Charette | Owner: nobody
Component: Core (Management | Version: 3.1
commands) |
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"384ac0990ff414526ec47381845dae79b8e3ddfe" 384ac099]:
{{{
#!CommitTicketReference repository=""
revision="384ac0990ff414526ec47381845dae79b8e3ddfe"
Refs #32061 -- Prevented password leak on MySQL dbshell crash.
The usage of the --password flag when invoking the mysql CLI has the
potential of exposing the password in plain text if the command happens
to crash due to the inclusion of args provided to
subprocess.run(check=True) in the string representation of the
subprocess.CalledProcessError exception raised on non-zero return code.
Since this has the potential of leaking the password to logging
facilities configured to capture crashes (e.g. sys.excepthook, Sentry)
it's safer to rely on the MYSQL_PWD environment variable instead even
if its usage is discouraged due to potential leak through the ps
command on old flavors of Unix.
Thanks Charlie Denton for reporting the issue to the security team.
Refs #24999.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32061#comment:6>
Django
-------------------------------------+-------------------------------------
| Felisiak
Type: Bug | Status: assigned
Component: Core (Management | Version: 3.1
commands) |
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => Mariusz Felisiak
* status: new => assigned
* has_patch: 1 => 0
Comment:
I'm going to leave this ticket open because `dbshell` on Oracle is still
affected.
--
Ticket URL: <https://code.djangoproject.com/ticket/32061#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Simon Charette | Owner: Mariusz
| Felisiak
Type: Bug | Status: assigned
Component: Core (Management | Version: 3.1
commands) |
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by GitHub <noreply@…>):
In [changeset:"009fddc96b88a9aee1232d6a2637f2970fdcfb50" 009fddc9]:
{{{
#!CommitTicketReference repository=""
revision="009fddc96b88a9aee1232d6a2637f2970fdcfb50"
Refs #32061 -- Fixed test_crash_password_does_not_leak() crash on Windows.
When env is passed to subprocess.run() we should pass all existing
environment variables. This fixes crash on Windows:
Fatal Python error: failed to get random numbers to initialize Python
Fatal Python error: _Py_HashRandomization_Init: failed to get random
numbers to initialize Python
Python runtime state: preinitialized
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32061#comment:8>