Closed Bug 227191 Opened 21 years ago Closed 20 years ago

If database is stopped, error message divulges DB password

Categories

(Bugzilla :: Bugzilla-General, defect, P2)

2.17.6
Other
Other
defect

Tracking

()

RESOLVED FIXED
Bugzilla 2.16

People

(Reporter: bugreport, Assigned: bugreport)

References

Details

(Whiteboard: [does not affect 2.16.x] [fixed in 2.18rc1])

Attachments

(1 file, 6 obsolete files)

This is a rather obscure security issue. If the databse is not running and a user attempts to acess index.cgi (probably anything), the error message displayed to the browser shows the database password. HOPEFULLY, admins have restricted the usefullness of that password to local users, but it is still something that should be squashed.
FWIW, I can reproduce this in Perl 5.6.1 but not in Perl 5.8.0, on Landfill. In Perl 5.8.0 we just get a rather generic "Internal Server Error" and even the error message spewed to the error_log has the password conveniently splatted in the log message. In Perl 5.6.1 we get a "Software Error" message with a call stack, and the call stack includes all the parameters to DBI->connect, including the password. I'm working on getting module versions now just in case.
checksetup.pl on bugzilla-tip on landfill reported that my CGI module was too old when I ran it under Perl 5.6.1. I installed Bugzilla's minimum CGI version on Perl 5.6.1 (version 2.93) and checksetup.pl verified that it was usable. Now Perl 5.6.1 also reports "Internal Server Error" to the user with no password disclosed. Joel: what version of CGI are you using on the install where this happened?
I'm using perl 5.8.0 Checking for CGI (v2.93) ok: found v3.00 Checking for Data::Dumper (any) ok: found v2.12 Checking for Date::Format (v2.21) ok: found v2.22 Checking for DBI (v1.32) ok: found v1.38 Checking for DBD::mysql (v2.1010) ok: found v2.9002 and I just updated to tip and still see the same behavior
On landfill, under 5.6.1, we have: Checking for CGI (v2.93) ok: found v2.93 Checking for Data::Dumper (any) ok: found v2.102 Checking for Date::Format (v2.21) ok: found v2.21 Checking for DBI (v1.32) ok: found v1.32 Checking for DBD::mysql (v2.1010) ok: found v2.1011 And under 5.8.0, we have: Checking for CGI (v2.93) ok: found v2.93 Checking for Data::Dumper (any) ok: found v2.12 Checking for Date::Format (v2.21) ok: found v2.21 Checking for DBI (v1.32) ok: found v1.32 Checking for DBD::mysql (v2.1010) ok: found v2.1025
OK, on landfill... Perl 5.8.0... Upgraded CGI to 3.00, still get ISE (Internal Server Error) Upgraded DBI to 1.39, still get ISE Upgraded DBD::mysql to 2.9003, still get ISE. Downgraded DBI to 1.38, still get ISE. Any other ideas?
There is a hack that would work... We could insert a line in sub _handle_error to filter out the paswords. Something like..... $_[0] =~ s/(connect\([^,]+,[^,]+,).+\)/$1*****)/g; It's not a very clean way to do it, though.
Attached patch Patch v1 (obsolete) — Splinter Review
OK, found a cleaner way to do this. Username and Password can be passed by hash member and that keeps hte password out of the error message.
Attachment #136657 - Flags: review?(bbaetz)
Comment on attachment 136657 [details] [diff] [review] Patch v1 Need DBI 1.36 for this according to DBI::Changes. Please bump the checksetup.pl version.
Attachment #136657 - Flags: review?(bbaetz) → review-
DBI 1.38 wound up being the cutoff for Perl 5.6.0 support. Since 1.36 < 1.38 we do not need to bump the minimum Perl version (yet).
reassigning to patch author. Does 2.16 have this problem? We still support Perl 5.00503 for 2.16, which means we can't require new versions of DBI there...
Whiteboard: [wanted for 2.16.6] [wanted for 2.18rc1]
Assignee: justdave → bugreport
This is kind of stuck. If we have to use an old DBI, I dont have a better suggestion. [I have a worse one... always first try to connect with the wrong password first and make sure the connection fails for the right reasone] Should we try to catch the result without letting the error message out at all?
For 2.17, we shouldn't have a problem with bumping the requirements. For 2.16, I recommend doing that too. Can someone confirm that implementing this doesn't require a recompile of the DBD drivers (eg to pick up a new Driver.xst?)
*** Bug 237782 has been marked as a duplicate of this bug. ***
Flags: blocking2.18+
Flags: blocking2.16.6+
Target Milestone: --- → Bugzilla 2.16
bbaetz: You are the closest to the DBI/DBD team. Care to take this one over?
Attached patch Patch v2 - just eval it (obsolete) — Splinter Review
Hopefully, people will be able to figure out why they cant connect by running checksetup. I'd rather have a secure but terse runtime error than a clear and insecure one. We could get the error string and mess with it, but it adds too much risk IMHO.
Attachment #136657 - Attachment is obsolete: true
Attachment #144209 - Flags: review?(justdave)
Comment on attachment 144209 [details] [diff] [review] Patch v2 - just eval it This is ok for 2.16, I guess. ther are alternatives (like not doing RaiseError until afterwards), but they're messy For 2.18, we want to just up teh required DBI version - ie the oher patch with teh checksetup.pl change.
Attachment #144209 - Flags: review?(justdave) → review+
I can't make this problem happen in 2.16 justdave, bbaetz: Should I go back to the first patch combined with a bump of the minimum DBI requirement or use the second patch??
*** Bug 239590 has been marked as a duplicate of this bug. ***
(In reply to comment #17) > justdave, bbaetz: Should I go back to the first patch combined with a bump of > the minimum DBI requirement or use the second patch?? Let's use something akin to the second patch for 2.16... I know none of us could reproduce it, but better safe than sorry and the patch won't hurt anything. For the tip I'd go with the first and bumping the DBI version.
Attachment #144209 - Attachment is obsolete: true
The 2.16 patch works even without bumping the DBI requirement. It gives a bit less information to the user when the connect fails, but the full message is still available when checksetup is run.
Attachment #145475 - Flags: review?(justdave)
Attachment #145479 - Flags: review?(justdave)
Attachment #145475 - Attachment is obsolete: true
Attachment #145475 - Flags: review?(justdave)
Attachment #145479 - Attachment is obsolete: true
Attachment #145479 - Flags: review?(justdave)
OK, I just figured out WHY we cannot make this happen on 2.16 and we don't even have to update the required versions. The problem is that 2.17 tries to be too helpful by generating a long error message. All we have to do is to stop doing that.
Attached patch Patch removing the leaky message (obsolete) — Splinter Review
Attachment #145483 - Flags: review?(justdave)
Comment on attachment 145483 [details] [diff] [review] Patch removing the leaky message I guess it's only fitting I fix this... it turns out that I reviewed its addition ion the first place.
Attachment #145483 - Flags: review?(bbaetz)
Comment on attachment 145483 [details] [diff] [review] Patch removing the leaky message We want the better error message, for debugging other SQL errors. Just don't print the username/password.
Attachment #145483 - Flags: review?(bbaetz) → review-
So, what is the suggestion... Carp about the whole message, but s/\'($db_user|$db_password)\'/\*\*\*\*\*\*/g ??
Attachment #145483 - Attachment is obsolete: true
Attachment #145531 - Flags: review?(bbaetz)
What was wrong with comment #7's patch, for HEAD?
OK, you choose. Attachment 145531 [details] [diff] would mask both username and password This one is cleaner, but masks only the password
Attachment #145545 - Flags: review+
Attachment #145531 - Attachment is obsolete: true
Attachment #145531 - Flags: review?(bbaetz)
Flags: blocking2.16.6+ → blocking2.16.6-
Whiteboard: [wanted for 2.16.6] [wanted for 2.18rc1] → [does not affect 2.16.x] [wanted for 2.18rc1]
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [does not affect 2.16.x] [wanted for 2.18rc1] → [does not affect 2.16.x] [ready for 2.18rc1]
Attachment #145483 - Flags: review?(justdave)
Proposed language for release notes: If the database is stopped but the webserver is running and Bugzilla is not shut down, the error message may reveal the database password.
Flags: approval+
checked in
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Whiteboard: [does not affect 2.16.x] [ready for 2.18rc1] → [does not affect 2.16.x] [fixed in 2.18rc1]
Clearing the security flag on disclosed bugs
Group: webtools-security
*** Bug 251979 has been marked as a duplicate of this bug. ***
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: