Re: [Django] #35930: Database password visible on debug page in local variable

[Django]

#35930: Database password visible on debug page in local variable
-------------------------------------+-------------------------------------
Reporter: bytej4ck | Owner: Baha
| Sdtbekov
Type: Bug | Status: assigned
Component: Error reporting | Version:
Severity: Normal | Resolution:
Keywords: db, password, | Triage Stage:
exposed | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Baha Sdtbekov):

* owner: (none) => Baha Sdtbekov
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/35930#comment:5>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#35930: Database password visible on debug page in local variable
-------------------------------------+-------------------------------------
Reporter: bytej4ck | Owner: Baha
| Sdtbekov

Type: Bug | Status: closed
Component: Error reporting | Version: dev
Severity: Normal | Resolution: needsinfo

Keywords: db, password, | Triage Stage:
exposed | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Natalia Bidart):

* resolution: => needsinfo
* status: assigned => closed
* version: => dev

Comment:

Thank you Jacob for the extra details, though I'm not being able to
reproduce. Making `connect` fail does not help me get a traceback at
runtime, it only prevents the development server to run.
Can someone please provide, ideally, a regression test? Or at least a code
snippet for a view to make this more easily reproducible.

Side note: if we eventually accept this ticket, I think it should include
some audit of other potential places where the connection params could be
used and not marked as sensitive.
--
Ticket URL: <https://code.djangoproject.com/ticket/35930#comment:6>

[Django]

#35930: Database password visible on debug page in local variable
-------------------------------------+-------------------------------------
Reporter: bytej4ck | Owner: Baha
| Sdtbekov
Type: Bug | Status: closed
Component: Error reporting | Version: dev
Severity: Normal | Resolution: needsinfo
Keywords: db, password, | Triage Stage:
exposed | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Jacob Walls):

Yeah, the failure has to be "occasional" enough to not prevent the
development server from booting. I know this is lazier than writing a unit
test, but two quick reproducers:
- Start your db, boot the development server, stop the db, and visit
another page that makes a db query.
- Or, while keeping your db up, fiddle with simulating a "random" failure
like this:

{{{#!diff
diff --git a/django/db/backends/postgresql/base.py
b/django/db/backends/postgresql/base.py
index c864cab57a..8f653f0078 100644
--- a/django/db/backends/postgresql/base.py
+++ b/django/db/backends/postgresql/base.py
@@ -181,6 +181,9 @@ class DatabaseWrapper(BaseDatabaseWrapper):
_named_cursor_idx = 0
_connection_pools = {}

+ # Simulate an occasional failure.
+ denominator_will_become_zero = 5
+
@property
def pool(self):
pool_options = self.settings_dict["OPTIONS"].get("pool")
@@ -308,6 +311,11 @@ class DatabaseWrapper(BaseDatabaseWrapper):
# default when no value is explicitly specified in options.
# - before calling _set_autocommit() because if autocommit is on,
that
# will set connection.isolation_level to
ISOLATION_LEVEL_AUTOCOMMIT.
+ self.denominator_will_become_zero -= 1
+ # Simulate occasional failure.
+ 1 / self.denominator_will_become_zero
+ print(self.denominator_will_become_zero)
+
options = self.settings_dict["OPTIONS"]
set_isolation_level = False
try:
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35930#comment:7>

[Django]

#35930: Database password visible on debug page in local variable
-------------------------------------+-------------------------------------
Reporter: bytej4ck | Owner: Baha
| Sdtbekov

Type: Bug | Status: new

Component: Error reporting | Version: dev
Severity: Normal | Resolution:

Keywords: db, password, | Triage Stage:
exposed | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Jacob Walls):

* resolution: needsinfo =>
* status: closed => new

--
Ticket URL: <https://code.djangoproject.com/ticket/35930#comment:8>

[Django]

#35930: Database password visible on debug page in local variable
-------------------------------------+-------------------------------------
Reporter: bytej4ck | Owner: Baha
| Sdtbekov
Type: Bug | Status: new
Component: Error reporting | Version: dev
Severity: Normal | Resolution:
Keywords: db, password, | Triage Stage:
exposed | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Baha Sdtbekov):

I think there's a deeper problem here.
We can have a lot of cases where we can leak classified data.

For example, using salted_hmac from crypto
If we specify wrong algorithm we can leak the secret key of django, I
understand that this is because of wrong usage but I would like to hide
them or not show them at all.

Because leaked data in production is a big loss.
--
Ticket URL: <https://code.djangoproject.com/ticket/35930#comment:9>

[Django]

#35930: Database password visible on debug page in local variable
-------------------------------------+-------------------------------------
Reporter: bytej4ck | Owner: Baha
| Sdtbekov
Type: Bug | Status: new
Component: Error reporting | Version: dev
Severity: Normal | Resolution:
Keywords: db, password, | Triage Stage:
exposed | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Baha Sdtbekov):

you can try to make something like SafeExceptionReporterFilter for local
vars

but I am concerned about the performance of such a solution
--
Ticket URL: <https://code.djangoproject.com/ticket/35930#comment:10>

[Django]

#35930: Database password visible on debug page in local variable
-------------------------------------+-------------------------------------
Reporter: bytej4ck | Owner: Baha

Type: | Sdtbekov
Cleanup/optimization | Status: new

Component: Error reporting | Version: dev
Severity: Normal | Resolution:

Keywords: db, password, | Triage Stage: Accepted
exposed |

Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Natalia Bidart):

* stage: Unreviewed => Accepted
* type: Bug => Cleanup/optimization

Comment:

Replying to [comment:7 Jacob Walls]:

> Yeah, the failure has to be "occasional" enough to not prevent the
development server from booting. I know this is lazier than writing a unit
test, but two quick reproducers:
> - Start your db, boot the development server, stop the db, and visit
another page that makes a db query.
> - Or, while keeping your db up, fiddle with simulating a "random"
failure like this:

Thank you Jacob, I assumed so and I did try both of these before, but the
bit that I was missing was to actually "go" to the DB to fetch or write
something. In retrospect, this is obvious, but somehow I missed it. I
managed to reproduce the issue, but as anticipated, I fear that this
ticket, if accepted, has a scope beyond handling only `connect`.
Specifically, if accepted, we should at least:

* mark as sensitive every location that uses the connection params via
calling `get_connection_params`
* mark as sensitive `settings_dict` and `conn_params` in
`get_connection_params`
* audit the rest of the code for other parts using or accessing the DB
settings/password in any way

I'm not convinced this is within the scope of what Django core should
handle. It has always been stated that
[https://docs.djangoproject.com/en/5.1/howto/error-reporting/#filtering-
error-reports error report filtering is a best-effort solution] and
further measures should be taken to sanitize or guard the error logs.
Despite my reservations, I'm inclined to accept this, as it could provide
a clear improvement by carefully adding the `sensitive_variables`
decorator to the audited methods and functions.

Regarding Baha's comments, I think we should limit this ticket to "DB
connection params" and avoid expanding the scope, as it’s better to focus
on one specific issue. In summary, I'm accepting with the
rationale/caveats expressed above.
--
Ticket URL: <https://code.djangoproject.com/ticket/35930#comment:11>

[Django]

#35930: Database password visible on debug page in local variable
-------------------------------------+-------------------------------------

Reporter: bytej4ck | Owner: Ahmed
Type: | Nassar
Cleanup/optimization | Status: assigned

Component: Error reporting | Version: dev
Severity: Normal | Resolution:
Keywords: db, password, | Triage Stage: Accepted
exposed |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Ahmed Nassar):

* owner: Baha Sdtbekov => Ahmed Nassar

* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/35930#comment:12>

[Django]

#35930: Database password visible on debug page in local variable
-------------------------------------+-------------------------------------
Reporter: bytej4ck | Owner: Ahmed
Type: | Nassar
Cleanup/optimization | Status: assigned
Component: Error reporting | Version: dev
Severity: Normal | Resolution:
Keywords: db, password, | Triage Stage: Accepted
exposed |

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Ahmed Nassar):

* has_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/35930#comment:13>

[Django]

#35930: Database password visible on debug page in local variable
-------------------------------------+-------------------------------------
Reporter: bytej4ck | Owner: Ahmed
Type: | Nassar
Cleanup/optimization | Status: assigned
Component: Error reporting | Version: dev
Severity: Normal | Resolution:
Keywords: db, password, | Triage Stage: Accepted
exposed |
Has patch: 1 | Needs documentation: 0

Needs tests: 1 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Mariusz Felisiak):

* needs_better_patch: 0 => 1
* needs_tests: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/35930#comment:14>