319140 - Bugzilla shouldn't disclose tracebacks on fatal errors

Status: RESOLVED INVALID

(Bugzilla :: Bugzilla-General, defect)

Description

[Marc Schumann [:Wurblzap]]

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.

Comment 1

[Marc Schumann [:Wurblzap]]

Attachment: 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.

Comment 2

[Frédéric Buclin]

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.

Comment 3

[Marc Schumann [:Wurblzap]]

Attachment: 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.

Comment 4

[Frédéric Buclin]

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?

Comment 5

[Frédéric Buclin]

Not a security bug

Comment 6

[Tim Brown]

(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

Comment 7

[Marc Schumann [:Wurblzap]]

Attachment: Patch 1.2

Addressing comments.

Comment 8

[Frédéric Buclin]

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.

Comment 9

[Marc Schumann [:Wurblzap]]

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

[Frédéric Buclin]

Comment on attachment 205840 [details] [diff] [review]
Patch 1.2

Waiting for an updated patch. Note that a second review would be welcome.

Comment 11

[Marc Schumann [:Wurblzap]]

Attachment: Patch 1.3

It turns out we get the error message passed pre-filtered for &, >, < and ".

Comment 12

[Max Kanat-Alexander]

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.)

Comment 13

[Max Kanat-Alexander]

We (in #bugzilla-meeting) have decided that this isn't significant enough to merit being backported to 2.18 and 2.20.

Comment 14

[Marc Schumann [:Wurblzap]]

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.

Comment 15

[Vlad Dascalu]

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.

Comment 16

[Myk Melez [:myk] [@mykmelez]]

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.

Comment 17

[Dave Miller [:justdave]]

Yeah, I agree with Myk, let's address his comments.

Comment 18

[Vlad Dascalu]

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.

Comment 19

[Marc Schumann [:Wurblzap]]

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

Comment 20

[Marc Schumann [:Wurblzap]]

Attachment: Patch 1.4

Let's assume so.

Comment 21

[Frédéric Buclin]

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.

Comment 22

[Frédéric Buclin]

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.

Comment 23

[Marc Schumann [:Wurblzap]]

(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

[Frédéric Buclin]

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

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

Comment 25

[Max Kanat-Alexander]

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

[Max Kanat-Alexander]

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.

Comment 27

[Marc Schumann [:Wurblzap]]

Ok, bailing at least until we know that we want to go on (referring to comment 26).

Comment 28

[Frédéric Buclin]

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.

Comment 29

[Max Kanat-Alexander]

WONTFIX as per comment 25.

Comment 31

[Reed Loden [:reed]]

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.

Comment 32

[Max Kanat-Alexander]

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.

Comment 33

[Dave Miller [:justdave]]

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.

Comment 34

[Frédéric Buclin]

(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.

Comment 35

[Dave Miller [:justdave]]

(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

[Max Kanat-Alexander]

(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

[Luca Carettoni]

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

[Max Kanat-Alexander]

(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.

Comment 39

[Michael Coates [:mcoates] (acct no longer active)]

(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.

Comment 40

[David Lawrence [:dkl]]

Attachment: 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

Comment 41

[Dave Miller [:justdave]]

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.

Comment 42

[Max Kanat-Alexander]

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

[Max Kanat-Alexander]

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.

Comment 44

[Reed Loden [:reed]]

Are there any other places besides tracebacks where file paths may be disclosed?

Comment 45

[Max Kanat-Alexander]

(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.

Comment 52

[Raymond Forbes[:rforbes]]

is there an update to this bug?

Comment 53

[:glob ✱]

(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.

Comment 54

[Frédéric Buclin]

(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

[Frédéric Buclin]

Not a hard-blocker, but would still be good to have.

Comment 57

[Frédéric Buclin]

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

[Frédéric Buclin]

Bugzilla 4.4 is now restricted to security fixes only.

Comment 59

[Adam Muntner [:adamm] (use NEEDINFO)]

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.

Comment 60

[Dylan Hardison [:dylan] (he/him)]

This flag is not appropriate, see followup email.

Comment 61

[Dylan Hardison [:dylan] (he/him)]

Almost nothing in this bug is correct any more, so it's being closed as invalid.