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)
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)
1.41 KB,
patch
|
bbaetz
:
review+
|
Details | Diff | Splinter Review |
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.
Comment 1•21 years ago
|
||
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.
Comment 2•21 years ago
|
||
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?
Assignee | ||
Comment 3•21 years ago
|
||
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
Comment 4•21 years ago
|
||
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
Comment 5•21 years ago
|
||
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?
Assignee | ||
Comment 6•21 years ago
|
||
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.
Assignee | ||
Comment 7•21 years ago
|
||
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.
Assignee | ||
Updated•21 years ago
|
Attachment #136657 -
Flags: review?(bbaetz)
Comment 8•21 years ago
|
||
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-
Comment 9•21 years ago
|
||
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).
Comment 10•21 years ago
|
||
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]
Updated•21 years ago
|
Assignee: justdave → bugreport
Assignee | ||
Comment 11•21 years ago
|
||
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?
Comment 12•21 years ago
|
||
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?)
Comment 13•21 years ago
|
||
*** Bug 237782 has been marked as a duplicate of this bug. ***
Updated•21 years ago
|
Flags: blocking2.18+
Flags: blocking2.16.6+
Target Milestone: --- → Bugzilla 2.16
Assignee | ||
Comment 14•21 years ago
|
||
bbaetz: You are the closest to the DBI/DBD team. Care to take this one over?
Assignee | ||
Comment 15•21 years ago
|
||
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.
Assignee | ||
Updated•21 years ago
|
Attachment #136657 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #144209 -
Flags: review?(justdave)
Comment 16•21 years ago
|
||
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+
Assignee | ||
Comment 17•21 years ago
|
||
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??
Comment 18•21 years ago
|
||
*** Bug 239590 has been marked as a duplicate of this bug. ***
Comment 19•21 years ago
|
||
(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.
Assignee | ||
Comment 20•21 years ago
|
||
Assignee | ||
Updated•21 years ago
|
Attachment #144209 -
Attachment is obsolete: true
Assignee | ||
Comment 21•21 years ago
|
||
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.
Assignee | ||
Updated•21 years ago
|
Attachment #145475 -
Flags: review?(justdave)
Assignee | ||
Updated•21 years ago
|
Attachment #145479 -
Flags: review?(justdave)
Assignee | ||
Updated•21 years ago
|
Attachment #145475 -
Attachment is obsolete: true
Attachment #145475 -
Flags: review?(justdave)
Assignee | ||
Updated•21 years ago
|
Attachment #145479 -
Attachment is obsolete: true
Attachment #145479 -
Flags: review?(justdave)
Assignee | ||
Comment 22•21 years ago
|
||
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.
Assignee | ||
Comment 23•21 years ago
|
||
Assignee | ||
Updated•21 years ago
|
Attachment #145483 -
Flags: review?(justdave)
Assignee | ||
Comment 24•21 years ago
|
||
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 25•21 years ago
|
||
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-
Assignee | ||
Comment 26•21 years ago
|
||
So, what is the suggestion... Carp about the whole message, but
s/\'($db_user|$db_password)\'/\*\*\*\*\*\*/g
??
Assignee | ||
Comment 27•21 years ago
|
||
Attachment #145483 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #145531 -
Flags: review?(bbaetz)
Comment 28•21 years ago
|
||
What was wrong with comment #7's patch, for HEAD?
Assignee | ||
Comment 29•21 years ago
|
||
OK, you choose.
Attachment 145531 [details] [diff] would mask both username and password
This one is cleaner, but masks only the password
Updated•21 years ago
|
Attachment #145545 -
Flags: review+
Assignee | ||
Updated•21 years ago
|
Attachment #145531 -
Attachment is obsolete: true
Attachment #145531 -
Flags: review?(bbaetz)
Updated•21 years ago
|
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]
Assignee | ||
Updated•21 years ago
|
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]
Assignee | ||
Updated•21 years ago
|
Attachment #145483 -
Flags: review?(justdave)
Assignee | ||
Comment 30•21 years ago
|
||
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.
Updated•20 years ago
|
Flags: approval+
Assignee | ||
Comment 31•20 years ago
|
||
checked in
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•20 years ago
|
Whiteboard: [does not affect 2.16.x] [ready for 2.18rc1] → [does not affect 2.16.x] [fixed in 2.18rc1]
Assignee | ||
Comment 33•20 years ago
|
||
*** Bug 251979 has been marked as a duplicate of this bug. ***
Updated•12 years ago
|
QA Contact: matty_is_a_geek → default-qa
You need to log in
before you can comment on or make changes to this bug.
Description
•