Bugzilla shouldn't disclose tracebacks on fatal errors

RESOLVED INVALID

Status

()

Bugzilla
Bugzilla-General
--
minor
RESOLVED INVALID
12 years ago
2 months ago

People

(Reporter: Wurblzap, Assigned: dkl)

Tracking

(Blocks: 1 bug, {sec-moderate})

2.21
sec-moderate
Dependency tree / graph
Bug Flags:
blocking4.4 -

Details

(Whiteboard: [infrasec:errorhandle][ws:moderate][wh-7183290][wh-7679357])

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

12 years ago
Bugzilla uses CGI.pm's fatalsToBrowser functionality to handle fatal errors. This displays full perl error messages to the user experiencing the error, possibly containing information that should not be visible to end users, like absolute file paths.
(Reporter)

Comment 1

12 years ago
Created attachment 205022 [details] [diff] [review]
Patch

Writes fatal errors to a file instead of to the browser.

It would be nice to able to write to STDERR so that it ends up in Apache's error log, but we can't do this because IIS sends STDERR to the browser.
Attachment #205022 - Flags: review?

Comment 2

12 years ago
Comment on attachment 205022 [details] [diff] [review]
Patch

What about a relative path only (I mean relative to the 'bugzilla' directory)? And in case Param('error_log') is off, we should still display the information in the web browser IMO.

From that point of view, that's a r- from me.
(Reporter)

Comment 3

12 years ago
Created attachment 205027 [details] [diff] [review]
Patch 1.1

I don't know how to make it display relative paths exactly when needed. A regexp won't cut it. Some paths will need to be cut, others not. The urlbase param is of no help either.

As proposed on IRC, the new patch shows the error on-screen if the error_log param is empty.
Attachment #205022 - Attachment is obsolete: true
Attachment #205027 - Flags: review?
Attachment #205022 - Flags: review?

Comment 4

12 years ago
Comment on attachment 205027 [details] [diff] [review]
Patch 1.1

>Index: Bugzilla/CGI.pm

>+        if ($error_log) {
>+            $further_information = 
>+                "<p>Further information is unfortunately not available.</p>\n";

That's wrong! It's simply not displayed on the screen. As a developer, I would like to know that the error message has been saved on the disk. Maybe something like:

"The error message has been saved into the error log file specified by the maintainer."

As a developer, I would think "ah yeah... cool, let's have a look" instead of "f***, how can find what's wrong if the info not available?".



>+            $further_information = "<p>This is the error reported by perl:\n" .
>+                                   "<code>$msg</code></p>\n";

Any chance to execute arbitrary code either in error.log or from the web browser? Shouldn't the error message be filtered?
(Reporter)

Updated

12 years ago
Status: NEW → ASSIGNED

Comment 5

12 years ago
Not a security bug
Group: webtools-security

Comment 6

12 years ago
(In reply to comment #5)
> Not a security bug
> 

No? http://www.osvdb.org/searchdb.php?action=search_title&vuln_title=path+disclosure&Search=Search
(Reporter)

Updated

12 years ago
Blocks: 300978
(Reporter)

Comment 7

12 years ago
Created attachment 205840 [details] [diff] [review]
Patch 1.2

Addressing comments.
Attachment #205027 - Attachment is obsolete: true
Attachment #205840 - Flags: review?(LpSolit)
Attachment #205027 - Flags: review?

Comment 8

12 years ago
Comment on attachment 205840 [details] [diff] [review]
Patch 1.2

html_quote() is defined in Util.pm. If you don't add 'use Bugzilla::Util;' this will probably not work.
(Reporter)

Comment 9

12 years ago
This works for all scripts doing "use Bugzilla" because "use Bugzilla" pulls in Bugzilla::Util. This is why it worked during my testing. But I agree, we should "use" all modules we intend to call functions from -- please deny review, and I'll put up a corrected patch.

Comment 10

12 years ago
Comment on attachment 205840 [details] [diff] [review]
Patch 1.2

Waiting for an updated patch. Note that a second review would be welcome.
Attachment #205840 - Flags: review?(LpSolit)
(Reporter)

Comment 11

12 years ago
Created attachment 206453 [details] [diff] [review]
Patch 1.3

It turns out we get the error message passed pre-filtered for &, >, < and ".
Attachment #205840 - Attachment is obsolete: true
Attachment #206453 - Flags: review?(LpSolit)
(Reporter)

Updated

12 years ago
Attachment #205840 - Attachment description: Patch → Patch 1.2

Comment 12

12 years ago
Comment on attachment 206453 [details] [diff] [review]
Patch 1.3

The way you have that BEGIN block, it won't work in mod_perl. (BEGIN blocks only run once in the history of the httpd process.)
Attachment #206453 - Flags: review?(LpSolit) → review-

Updated

12 years ago
Priority: -- → P5

Comment 13

12 years ago
We (in #bugzilla-meeting) have decided that this isn't significant enough to merit being backported to 2.18 and 2.20.
Target Milestone: Bugzilla 2.18 → Bugzilla 2.22
(Reporter)

Comment 14

12 years ago
Comment on attachment 206453 [details] [diff] [review]
Patch 1.3

(In reply to comment #12)
> The way you have that BEGIN block, it won't work in mod_perl. (BEGIN blocks
> only run once in the history of the httpd process.)

Once is sufficient -- the BEGIN block registers the sub to handle the message. The reference to the sub is valid accross page views.
Attachment #206453 - Flags: review- → review?(mkanat)

Comment 15

12 years ago
Comment on attachment 206453 [details] [diff] [review]
Patch 1.3

+<html>
+<head>
+<title>Bugzilla error</title>

The title should be 'Bugzilla Error'.

The HTML should probably use 2-space identation.

This should probably be a template --we will soon want errors reported in the XML format. But since it's pretty tricky, it's ok for now I guess.

+                flock($fd, LOCK_EX);
+                $fd->print(sprintf("%s: %s: %s\n", scalar gmtime(),
+                                                   $ENV{'REQUEST_URI'},
+                                                   $msg));
+                undef $fd;

I've tested and this appears to work. I wanted an explicit release of the lock first time I've read this.
Attachment #206453 - Flags: review?(mkanat) → review+

Updated

12 years ago
Flags: approval?
Flags: approval2.22?
Seems strange to give the admin an option to display errors to users without explaining the implications of doing that, especially given that there are security implications.  Also, I imagine some admins will never want to display errors to users but will also not want to log errors all the time (f.e. because they have limited disk space and don't want to risk repeated errors filling it up) but rather turn on the log only when users start hitting a reproducible problem.
Yeah, I agree with Myk, let's address his comments.
Flags: approval?
Flags: approval2.22?

Comment 18

12 years ago
Comment on attachment 206453 [details] [diff] [review]
Patch 1.3

Yeah, thanks Myk for the comments --disk space was something that crossed my mind, but I couldn't put my finger on it.
Attachment #206453 - Flags: review+ → review-
(Reporter)

Comment 19

12 years ago
Ok, so what we want is a third option "neither display nor logging"?

Updated

11 years ago
No longer blocks: 300978
(Reporter)

Comment 20

11 years ago
Created attachment 235768 [details] [diff] [review]
Patch 1.4

Let's assume so.
Attachment #206453 - Attachment is obsolete: true
Attachment #235768 - Flags: review?(LpSolit)

Comment 21

11 years ago
Comment on attachment 235768 [details] [diff] [review]
Patch 1.4

>Index: Bugzilla/CGI.pm

>+    elsif ($error_log_type eq 'screen') {
>+        $further_information = "<p>This is the error reported by perl:\n" .
>+                               "<code>$msg</code></p>\n";

Nit: I would like a newline before $msg. "\n" should be "</p>\n<p>" or </p><p>". I see no newline from my web browser.



>Index: Bugzilla/Config/Core.pm

>+   name => 'error_log',
>+   type => 's',
>+   choices => ['screen', 'logfile', 'none'],
>+   default => 'none'
>+  },

I think this parameter should go into the "Administrative Policies" section (section=admin). We should avoid populating the Core section too much.

Nit: also, wouldn't it make sense to get errors on both the screen and the logfile?


Anyway, your patch is working fine (we just have an annoying "Content-Type: text/html" displayed at the top of the page if the error occurs after "print $cgi->header()"). I still want mkanat's second-review though. I'm not sure we should take it for 2.22. Maybe should we let it bake on the trunk for a while and backport it on 2.22 in a few weeks if we want to. So r=LpSolit for tip only.
Attachment #235768 - Flags: review?(mkanat)
Attachment #235768 - Flags: review?(LpSolit)
Attachment #235768 - Flags: review+

Comment 22

11 years ago
Comment on attachment 235768 [details] [diff] [review]
Patch 1.4

>Index: Bugzilla/CGI.pm

>+        use Bugzilla::Constants qw(bz_locations);

Also, you should *import* Bugzilla::Constants, not use it IMO.
(Reporter)

Comment 23

11 years ago
(In reply to comment #21)
> Nit: I would like a newline before $msg. "\n" should be "</p>\n<p>" or
> </p><p>". I see no newline from my web browser.

How about <pre>$msg</pre>?

Comment 24

11 years ago
(In reply to comment #23)
> How about <pre>$msg</pre>?

If <pre></pre> forces a newline, sure, why not.

Comment 25

11 years ago
My honest opinion about this bug is that it adds complexity without adding any real benefit to the user.

On top of all this, we already know that fatalsToBrowser doesn't work in mod_perl, so we're likely to run into problems there.

My opinion is now, and basically always has been, that this should be WONTFIX'ed. I seem to have forgotten to mention it earlier in this bug, but I'm fairly sure I expressed my opinion in a meeting, one week.

Comment 26

11 years ago
Comment on attachment 235768 [details] [diff] [review]
Patch 1.4

If you do go ahead with this patch (which I don't think we should do), you should use the error-log facility that already exists in _throw_error in Bugzilla::Error. Just abstract out that code into a separate function. Actually, there's already a bug somewhere where karl did that.
Attachment #235768 - Flags: review?(mkanat) → review-
(Reporter)

Comment 27

11 years ago
Ok, bailing at least until we know that we want to go on (referring to comment 26).
Assignee: wurblzap → general
Status: ASSIGNED → NEW

Updated

10 years ago
Target Milestone: Bugzilla 2.22 → Bugzilla 3.0

Comment 28

9 years ago
The Bugzilla 3.0 branch is now locked to security bugs and dataloss fixes only. This bug doesn't fit into one of these two categories and is retargetted to 3.2 as part of a mass-change. To catch bugmails related to this mass-change, use lts081207 in your email client filter.
Target Milestone: Bugzilla 3.0 → Bugzilla 3.2

Comment 29

8 years ago
WONTFIX as per comment 25.
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → WONTFIX

Updated

8 years ago
Target Milestone: Bugzilla 3.2 → ---

Updated

7 years ago
Duplicate of this bug: 657256
I'm not sure why this is a controversy. We shouldn't be displaying file paths at all, and it would be best to not display traceback information in the first place.
Severity: minor → normal
Status: RESOLVED → REOPENED
Priority: P5 → --
Resolution: WONTFIX → ---
Whiteboard: [infrasec:errorhandle][ws:moderate]
Target Milestone: --- → Bugzilla 4.2

Comment 32

7 years ago
Completely disagree. Show me a serious security issue here that's worth losing a lot of the value in error messages. 

Also, if one of the drivers closes a bug as WONTFIX, as I'm sure you know, it's usually more polite to comment in the bug first before just reopening it.
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago7 years ago
Resolution: --- → WONTFIX
How about if I reopen it then.  I'm not a real big fan of security by obscurity either, but there's a huge portion of the security community that are pretty adamant about this stuff, and it could cost us deployments in companies that audit first.

I'm not sure what all this "adding complexity with little benefit to the user" is, either... from a user experience standpoint, the less garbage we throw at the end-user the better.  The traceback belongs in a logfile for the sysadmin, just because it's ugly and complicated.  The end user only needs to know that something went wrong.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---

Comment 34

7 years ago
(In reply to comment #33)
> throw at the end-user the better.  The traceback belongs in a logfile for
> the sysadmin, just because it's ugly and complicated.

The traceback being displayed into the web browser itself is a real benefit to me to debug Bugzilla and track bugs. I wouldn't want to see it gone. wurblzap's patch offers the ability to let the admin decide.
(In reply to comment #34)
> wurblzap's patch offers the ability to let the admin decide.

That works for me.  Having a debug mode available is a nice option when you're troubleshooting something.

Comment 36

7 years ago
(In reply to comment #33)
> How about if I reopen it then.

  You're certainly welcome to--I was just pointing out to reed that usually we discuss things in comments or on IRC before reopening WONTFIX bugs. I was actually available on IRC at the time the bug was reopened, even.

>  I'm not a real big fan of security by
> obscurity either, but there's a huge portion of the security community that
> are pretty adamant about this stuff, and it could cost us deployments in
> companies that audit first.

  You know, I haven't seen any evidence of this costing us a single deployment, and Bugzilla has been deployed by hundreds of security-conscious companies, many of which audit first. Also, I don't think there's really good evidence that there's a "huge portion "of the security community that is pretty adamant about file paths being a security issue.

> I'm not sure what all this "adding complexity with little benefit to the
> user" is, either... from a user experience standpoint, the less garbage we
> throw at the end-user the better.  The traceback belongs in a logfile for
> the sysadmin, just because it's ugly and complicated.  The end user only
> needs to know that something went wrong.

  This bug isn't about tracebacks, this bug is about *absolute paths*. It was spawned from a report that somebody sent us by email saying "absolute paths are a security issue", with which we generally disagreed.

  With that said, it is certainly possible for improperly-formatted tracebacks to give away security information in their *parameters*. Carp has the ability to control whether or not parameters are shown, and for ThrowCodeError tracebacks, they are not. That was why the bug report that got *this* bug report reopened (incorrectly, as this bug has to do with file paths, originally, even though the description may not be so clear) wasn't something we needed to fix.

  fatalsToBrowser does not give tracebacks by default at all, it only gives error messages. So there's no security issue here.

Comment 37

7 years ago
If you are referring to bug #657256, the problem is about showing detailed information during application exceptions while bugzilla is in production.
Absolute paths were just an example.

It's perfectly reasonable to have a 'debug' option. However, it should be disable in production (e.g. https://bugzilla.mozilla.org/page.cgi?id=bbb). I'm pretty sure that a large number of security enthusiasts agree with it.

Having said that, I don't see the point of discussing further on this. If the 'debug' option does not exist, it should be implemented. Otherwise, just disable that option in production environment such as this site.

Comment 38

7 years ago
(In reply to comment #37)
> If you are referring to bug #657256, the problem is about showing detailed
> information during application exceptions while bugzilla is in production.
> Absolute paths were just an example.

  Absolute paths are what this bug is about.

  bug 657256 is about tracebacks. ThrowCodeError prevents any argument parameters from being shown in those tracebacks. The tracebacks themselves do not pose a security threat. 

  Once we start going down that road of assuming that things like function names and stacks are security issues, it's a slippery slope to assuming that *any* disclosure of information about *anything* about the system is security-related. That's a slippery slope that would dilute our security process, add unnecessary code complexity, and put an additional burden on our volunteer workforce.

> I'm pretty sure that a large number of security enthusiasts agree with it.

  I've seen at least five people who claim to have security experience state that they *feel* that there's some value to be had here, but I haven't really heard any strong arguments other than security through obscurity, which I think most (if not all) security experts would agree is a poor form of security theater.

> If the 'debug' option does not exist, it should be implemented.

  For this, actually, I wouldn't want a general "debug mode" option--that gets too general and I think it would be hard to know when to have it on or off. I'd prefer show_tracebacks_to_users, and have it default to on, to make life easier for people who are newly installing Bugzilla. (We already get enough support email from confused people who say "I got Internal Server Error and I have no idea what to do!")

  If we do hide tracebacks from users, we need to show them a unique error id that they can report to the admin, so that the admin knows what the heck they're talking about and can help them debug any problem the user is experiencing. The admin could search for this error id in the logs.

  You also have to remember that various Bugzilla modules (TT, for example) use a Perl-style exception mechanism. You will have to account for that in any error handler you write.

  Also, some parts of Bugzilla (or some modules we use) may override __DIE__, so you may not be able to do this as a pure __DIE__ override.

  Now, of course, all of this would probably be more relevant in another bug, unless we want to change the summary of this bug and make it be about tracebacks instead of file paths.
(In reply to comment #38)
> > I'm pretty sure that a large number of security enthusiasts agree with it.
> 
>   I've seen at least five people who claim to have security experience state
> that they *feel* that there's some value to be had here, but I haven't
> really heard any strong arguments other than security through obscurity,
> which I think most (if not all) security experts would agree is a poor form
> of security theater.
> 

Good error handling in an application is not "security through obscurity". Properly restricting access to system and configuration data in error handling is part of defense in depth. Data leaked through error messages can be used with other vulnerabilities and will generally decrease the difficulty of the attack, increase the number of people capable of exploiting your system, and raise the overall likelihood of compromise.  Specific examples include file traversal vulnerabilities (much easier when you know the path), SQL injection (easy if the error message discloses the SQL string or the syntax error).  While these may or may not be specific examples for this scenario, the fact remains the exposing system data unnecessarily to the user increases risk.


It is regularly accepted standard security process to not display unnecessary data to users.  A debug mode/option seems like a fair tradeoff to achieve both goals.

Regardless of whether we are aware of anyone passing on bugzilla because of this issue, this is still a security issue that will be brought up by any team performing a review of bugzilla.  Compared to issues like sql injection or xss, it may be lower risk. But its still a red flag against the product.

Everything in security comes down to a risk decision. My opinion on this system is to follow the established route that most other applications have done.

1. Hide all system data from users in error messages
2. If error messages need to be recorded, save them on a log on the server side
3. If the user needs to be able to report an error to someone, then provide an error code in the error message. This code can be easily mapped by an admin back to the corresponding details on the system log.
(Assignee)

Comment 40

7 years ago
Created attachment 533075 [details] [diff] [review]
Patch to suppress traceback from browser based on param (v1)

Simple patch to turn off traceback on ThrowCodeError if param 'show_tracebacks' is set to off.

dkl
I think because the defaults are often what people use while testing a system for deployment, it's a fine idea to have this all enabled by default.  The documentation in the "securing your system" section should recommend turning this off once you go to production.

Updated

7 years ago
Assignee: general → dkl
Severity: normal → minor
Summary: Bugzilla shouldn't disclose file paths on fatal errors → Bugzilla shouldn't disclose tracebacks on fatal errors

Comment 42

7 years ago
Comment on attachment 533075 [details] [diff] [review]
Patch to suppress traceback from browser based on param (v1)

This is a good start, though we also need to deal with die, which is what this bug is actually about.

Also, I don't think you need to change Error.pm; I think changing just the template is enough.

Comment 43

7 years ago
You should also deal with manual calls to Carp::confess/etc. and make sure that you implement that unique-error-id for the end user that they can give to the admin, with some clear way for the admin to recover the actual error that occurred.

Of course, also make sure not to interfere with anything that's legitimately using die as an exception-throwing mechanism, and don't kill other __DIE__ overrides.
Are there any other places besides tracebacks where file paths may be disclosed?
Status: REOPENED → ASSIGNED

Comment 45

7 years ago
(In reply to comment #44)
> Are there any other places besides tracebacks where file paths may be
> disclosed?

  I'm not sure, but if there are, separate bugs should be filed for them and they should each be evaluated as a complexity tradeoff individually.

Updated

7 years ago
Target Milestone: Bugzilla 4.2 → Bugzilla 5.0
(Assignee)

Updated

7 years ago
Duplicate of this bug: 661602

Updated

6 years ago
Duplicate of this bug: 676825
Duplicate of this bug: 687235
Duplicate of this bug: 700102
Duplicate of this bug: 702607
Whiteboard: [infrasec:errorhandle][ws:moderate] → [infrasec:errorhandle][ws:moderate][wh-7183290]
Whiteboard: [infrasec:errorhandle][ws:moderate][wh-7183290] → [infrasec:errorhandle][ws:moderate][wh-7183290][wh-7679357]
Duplicate of this bug: 711038
is there an update to this bug?
(In reply to Raymond Forbes[:rforbes] from comment #52)
> is there an update to this bug?

for bugzilla.mozilla.org i removed public facing tracebacks when i integrated arecibo reporting (bug 698345).  the work there may be of assistance here.
Keywords: sec-moderate
Blocks: 785112

Updated

5 years ago
Flags: blocking4.4+

Updated

5 years ago
Depends on: 676844

Comment 54

5 years ago
(In reply to David Lawrence [:dkl] from comment #40)
> Simple patch to turn off traceback on ThrowCodeError if param
> 'show_tracebacks' is set to off.

dkl, could you attach your patch to a separate bug, as this bug is about fatal errors? Your patch is only about ThrowCodeError().

Comment 55

5 years ago
Not a hard-blocker, but would still be good to have.
Flags: blocking4.4+ → blocking4.4-

Updated

4 years ago
Duplicate of this bug: 876105

Comment 57

4 years ago
Comment on attachment 533075 [details] [diff] [review]
Patch to suppress traceback from browser based on param (v1)

>+    if (Bugzilla->params->{'show_tracebacks'}) {

Instead of a new parameter, couldn't we reuse the existing debug_group parameter?

Comment 58

2 years ago
Bugzilla 4.4 is now restricted to security fixes only.
Target Milestone: Bugzilla 4.4 → ---
Max,

There's a lot of history to this bug and it's not immediately clear to me what work remains to be done and I didn't see POC for how the issue was triggered so I'm not sure what to test for myself. 

Could you please summarize those details into this post? I've been triaging older bugs, it would be a big help.
Flags: needinfo?(mkanat)
This flag is not appropriate, see followup email.
Flags: needinfo?(mkanat)
Almost nothing in this bug is correct any more, so it's being closed as invalid.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago2 months ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.