950486 - Move the webdotbase and font_file parameters from data/params into localconfig (and kill the Graphs panel)

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[laurens.bal]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20100101 Firefox/25.0 (Beta/Release)
Build ID: 20131112160018

Steps to reproduce:

1. go to http://127.0.0.1/bugzilla/editparams.cgi?section=dependencygraph
2. Fill in the following webdotbase: /var/www/bugzilla/admin.cgi
3. Click on the button "save changes"
4. go to http://127.0.0.1/bugzilla/showdependencygraph.cgi?id=65845 (the id doesn't matter)
5. The script specified in step 2 will be executed.


Actual results:

The command/file execution:

In step 2, it is possible to execute every script that exist on the local machine with 
execute rights. Every .sh, .cgi, .pl script. As long as they don't expect external parameters.
Including linux commands. Here are some examples that worked:

/bin/echo (doesn't see -Tpng as an external parameter)
/var/www/bugzilla/admin.cgi
/var/www/bugzilla/whineatnews.pl
/usr/bin/anyshellscript.sh


The information leakage:

Because the webdotbase parameter is checked with -x.
It is possible to guess the local file system structure including
executable scripts and folders. With the following examples I was able to determine the file structure of my local bugzilla installation (there are more possibilities):

/var/ - returned no error
/var/www/ - returned no error
/var/www/bugzilla - returned no error
/usr/bin/ - returned no error
/bin/ls - returned no error


Expected results:

/Bugzilla/Config/Common.pm - line 228: if(! -x $value) {
Shouldn't allow leakage of information and should provide better checks
for the executed script.

Comment 1

[laurens.bal]

Finally after two years of testing the software :)

Comment 2

[Frédéric Buclin]

Admins are trusted users. If you cannot trust them, you are in trouble.

Comment 3

[laurens.bal]

Okay

Comment 4

[Dave Miller [:justdave]]

I agree there's no security implication here due to admins being trusted users, but in the grand scheme of things, it'd probably be best if config options that referred to pathnames on the filesystem were in localconfig instead of params.  It's plausible (not likely, but plausible) that if you had more than one bugzilla server talking to the same database that they might not have the binary in the same place on both servers, and localconfig is machine-specific, too.

Comment 5

[Frédéric Buclin]

(In reply to Dave Miller [:justdave] ([e-mail]) from comment #4)
> It's plausible (not likely, but plausible) that if you
> had more than one bugzilla server talking to the same database that they
> might not have the binary in the same place on both servers, and localconfig
> is machine-specific, too.

Parameters are not stored in the DB, but in data/params. But I agree that webdotbase (and font_file, while we are on it) should not be settable from the web UI.

Let's reword this bug summary a bit...

Comment 6

[laurens.bal]

Okay, that's a good solution.

Comment 7

[Gervase Markham [:gerv]]

I agree admins are trusted users, but I wonder whether we should still have a policy that an admin shouldn't be able to execute arbitrary code on the local box. Like, for example, we don't give admins (intentionally, at least!) the right to execute arbitrary SQL. 

Gerv

Comment 8

[Matt Selsky [:selsky]]

Attachment: Move webdotbase and font_file to localconfig

I added a function Localconfig() to Bugzilla::Template that allows accessing localconfig variables just like Param()

Comment 9

[Dave Miller [:justdave]]

(In reply to Matt Selsky [:selsky] from comment #8)
> I added a function Localconfig() to Bugzilla::Template that allows accessing
> localconfig variables just like Param()

This should already be available to the templates via the Bugzilla object.

[% USE Bugzilla %]
[% IF Bugzilla.localconfig.font_file %]
  [% graph.set_x_axis_font(Bugzilla.localconfig.font_file, 9);
...

Comment 10

[Matt Selsky [:selsky]]

Attachment: bug-950486-v2.patch

Updated to use Bugzilla object in templates

Comment 11

[Gervase Markham [:gerv]]

Comment on attachment 8384332 [details] [diff] [review]
bug-950486-v2.patch

Review of attachment 8384332 [details] [diff] [review]:
-----------------------------------------------------------------

::: Bugzilla/Install/Requirements.pm
@@ +666,4 @@
>      if (-e "$webdotdir/.htaccess") {
>          my $htaccess = new IO::File("$webdotdir/.htaccess", 'r') 
>              || die "$webdotdir/.htaccess: " . $!;
> +        if (!grep(/ \\\.png\$/, $htaccess->getlines)) {

If you can explain this change to me, then the rest looks good, and r=gerv.

Gerv

Comment 12

[Gervase Markham [:gerv]]

Oh, and thanks for the patch, and sorry for the delay :-)

Gerv

Comment 13

[Matt Selsky [:selsky]]

That change fixes the htaccess check for .png files being viewable remotely to match the code in Bugzilla/Config/Common.pm's check_webdotbase(), where it checks for "! grep(/ \\\.png\$/,<HTACCESS>)".

Also, http://www.research.att.com/~north/cgi-bin/webdot.cgi doesn't seem to exist anymore (Stephen North left AT&T Research last year).  Should I make a new bug to remove that URL as a default value?

Comment 14

[Gervase Markham [:gerv]]

a) Great. b) Yes :-)

Gerv

Comment 15

[Gervase Markham [:gerv]]

Comment on attachment 8384332 [details] [diff] [review]
bug-950486-v2.patch

r=gerv. Don't forget to file that follow-up bug.

Gerv

Comment 16

[Matt Selsky [:selsky]]

Bug 978615 was filed for the follow-up.

Comment 17

[Gervase Markham [:gerv]]

I proposed in bug 978615 that we remove the remote capability and just ask admins to install a local dot.

Gerv

Comment 18

[:glob ✱]

when upgrading an existing site, existing values for webdotbase and font_file should be automatically migrated to localconfig, instead of reverting to the default value.

Comment 20

[Frédéric Buclin]

a- pretty much means WONTFIX. You should set r- on the patch if you disagree with the current proposed fix.

Comment 21

[Gervase Markham [:gerv]]

Matt Selsky: are you able to enhance your patch to add the feature requested in comment 18? It shouldn't be more than a couple of lines...

Gerv

Comment 22

[Matt Selsky [:selsky]]

Attachment: v3

Now handles converting from params.

Comment 23

[Matt Selsky [:selsky]]

http://bugzilla.readthedocs.org/en/latest/administering/parameters.html will need to be updated as well.  Should that be done in this bug?

Comment 24

[Gervase Markham [:gerv]]

Yes, please.

Gerv

Comment 25

[Gervase Markham [:gerv]]

Comment on attachment 8530685 [details] [diff] [review]
v3

Review of attachment 8530685 [details] [diff] [review]:
-----------------------------------------------------------------

r- due to comments and docs changes needed.

Gerv

::: Bugzilla/Install/Filesystem.pm
@@ +348,5 @@
>          },
>  
>          "$webdotdir/.htaccess" => { perms => WS_SERVE, contents => <<EOT
> +# Restrict access to .dot files to the webdot server, if used
> +# You will need to edit this and uncomment it

This comment should be something like:

# If you run a local webdot server, you will need to allow it access to generated .dot files. Uncomment this section and replace the IP address with the IP address of your webdot server.

::: Bugzilla/Install/Localconfig.pm
@@ +234,5 @@
>                  $localconfig->{$name} = $answer->{$name};
>              }
> +            elsif (exists Bugzilla->params->{$name}) {
> +                $localconfig->{$name} = Bugzilla->params->{$name};
> +            }

This means we can never have parameters and localconfig values with the same name. I guess that's sensible, although perhaps we should document that somewhere?

::: Bugzilla/Install/Requirements.pm
@@ +678,4 @@
>      if (-e "$webdotdir/.htaccess") {
>          my $htaccess = new IO::File("$webdotdir/.htaccess", 'r') 
>              || die "$webdotdir/.htaccess: " . $!;
> +        if (!grep(/ \\\.png\$/, $htaccess->getlines)) {

Why this change?

::: editparams.cgi
@@ +89,4 @@
>              }
>              # Stop complaining if the URL has no trailing slash.
>              # XXX - This hack can go away once bug 303662 is implemented.
> +            if ($name =~ /base$/) {

Why this change?

Comment 26

[Matt Selsky [:selsky]]

> This means we can never have parameters and localconfig values with the same
> name. I guess that's sensible, although perhaps we should document that
> somewhere?

Where is a good place?

> ::: Bugzilla/Install/Requirements.pm
> @@ +678,4 @@
> >      if (-e "$webdotdir/.htaccess") {
> >          my $htaccess = new IO::File("$webdotdir/.htaccess", 'r') 
> >              || die "$webdotdir/.htaccess: " . $!;
> > +        if (!grep(/ \\\.png\$/, $htaccess->getlines)) {
> 
> Why this change?

As per comment 13, that change fixes the htaccess check for .png files being viewable remotely to match the code in Bugzilla/Config/Common.pm's check_webdotbase(), where it checks for "! grep(/ \\\.png\$/,<HTACCESS>)".


> ::: editparams.cgi
> @@ +89,4 @@
> >              }
> >              # Stop complaining if the URL has no trailing slash.
> >              # XXX - This hack can go away once bug 303662 is implemented.
> > +            if ($name =~ /base$/) {
> 
> Why this change?

The patch was truncated.  It should be:

@@ -89,7 +89,7 @@ if ($action eq 'save' && $current_module) {
             }
             # Stop complaining if the URL has no trailing slash.
             # XXX - This hack can go away once bug 303662 is implemented.
-            if ($name =~ /(?<!webdot)base$/) {
+            if ($name =~ /base$/) {
                 $value = "$value/" if ($value && $value !~ m#/$#);
             }
         }

We no longer need to look for webdotbase, but only the other *base variables.

I'll work on the doc changes needed.

Comment 27

[Matt Selsky [:selsky]]

Attachment: v4

Comment 28

[Gervase Markham [:gerv]]

Comment on attachment 8534183 [details] [diff] [review]
v4

r- - missing docs changes.

Gerv

Comment 29

[Matt Selsky [:selsky]]

docs/en/rst/administering/parameters.rst was updated.  Which docs did I miss?

Thanks!

Comment 30

[Gervase Markham [:gerv]]

Comment on attachment 8534183 [details] [diff] [review]
v4

Sorry; I was looking for added documentation, but thinking about it, we don't have docs for localconfig params.

Gerv

Comment 31

[Gervase Markham [:gerv]]

To ssh://[e-mail]/bugzilla/bugzilla.git
   f264250..adc3c7a  master -> master

Thanks, Matt :-) This is definitely an improvement.

Gerv

Comment 32

[Gervase Markham [:gerv]]

Failed to remove two files:

To ssh://[e-mail]/bugzilla/bugzilla.git
   adc3c7a..f1d4771  master -> master

Gerv