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

RESOLVED FIXED in Bugzilla 6.0

Status

()

--
minor
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: laurens.bal, Assigned: selsky)

Tracking

({relnote})

4.5.1
Bugzilla 6.0
relnote
Dependency tree / graph
Bug Flags:
approval +
sec-bounty -

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

5 years ago
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.
(Reporter)

Updated

5 years ago
Severity: normal → critical
(Reporter)

Comment 1

5 years ago
Finally after two years of testing the software :)

Comment 2

5 years ago
Admins are trusted users. If you cannot trust them, you are in trouble.
Group: bugzilla-security
Severity: critical → minor
(Reporter)

Comment 3

5 years ago
Okay
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

5 years ago
(In reply to Dave Miller [:justdave] (justdave@bugzilla.org) 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...
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: relnote
Summary: Information leakage + command/file execution in adminstration interface → Move the webdotbase and font_file parameters from data/params into localconfig (and kill the Graphs panel)
Target Milestone: --- → Bugzilla 5.0

Updated

5 years ago
Blocks: 208709
(Reporter)

Comment 6

5 years ago
Okay, that's a good solution.
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
(Assignee)

Comment 8

5 years ago
Created attachment 8384272 [details] [diff] [review]
Move webdotbase and font_file to localconfig

I added a function Localconfig() to Bugzilla::Template that allows accessing localconfig variables just like Param()
Assignee: administration → selsky
Status: NEW → ASSIGNED
Attachment #8384272 - Flags: review?(dkl)
(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);
...
(Assignee)

Comment 10

5 years ago
Created attachment 8384332 [details] [diff] [review]
bug-950486-v2.patch

Updated to use Bugzilla object in templates
Attachment #8384272 - Attachment is obsolete: true
Attachment #8384272 - Flags: review?(dkl)
Attachment #8384332 - Flags: review?(dkl)

Updated

5 years ago
Attachment #8384332 - Flags: review?(dkl) → review?(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
Oh, and thanks for the patch, and sorry for the delay :-)

Gerv
(Assignee)

Comment 13

5 years ago
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?
a) Great. b) Yes :-)

Gerv
(Assignee)

Updated

5 years ago
See Also: → bug 978615
Comment on attachment 8384332 [details] [diff] [review]
bug-950486-v2.patch

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

Gerv
Attachment #8384332 - Flags: review?(gerv) → review+
Flags: approval?
(Assignee)

Comment 16

5 years ago
Bug 978615 was filed for the follow-up.
I proposed in bug 978615 that we remove the remote capability and just ask admins to install a local dot.

Gerv
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.
Flags: approval? → approval-

Updated

4 years ago
Target Milestone: Bugzilla 5.0 → ---

Comment 20

4 years ago
a- pretty much means WONTFIX. You should set r- on the patch if you disagree with the current proposed fix.
Flags: approval-
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
Flags: needinfo?(selsky)
(Assignee)

Comment 22

4 years ago
Created attachment 8530685 [details] [diff] [review]
v3

Now handles converting from params.
Flags: needinfo?(selsky)
Attachment #8530685 - Flags: review?(gerv)
(Assignee)

Updated

4 years ago
Attachment #8384332 - Attachment is obsolete: true
(Assignee)

Comment 23

4 years ago
http://bugzilla.readthedocs.org/en/latest/administering/parameters.html will need to be updated as well.  Should that be done in this bug?
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?
Attachment #8530685 - Flags: review?(gerv) → review-
(Assignee)

Comment 26

4 years ago
> 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.
(Assignee)

Comment 27

4 years ago
Created attachment 8534183 [details] [diff] [review]
v4
Attachment #8530685 - Attachment is obsolete: true
Attachment #8534183 - Flags: review?(gerv)
Comment on attachment 8534183 [details] [diff] [review]
v4

r- - missing docs changes.

Gerv
Attachment #8534183 - Flags: review?(gerv) → review-
(Assignee)

Comment 29

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

Thanks!
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
Attachment #8534183 - Flags: review- → review+
Flags: approval?
Flags: approval? → approval+
Target Milestone: --- → Bugzilla 6.0
To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   f264250..adc3c7a  master -> master

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

Gerv
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Failed to remove two files:

To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   adc3c7a..f1d4771  master -> master

Gerv

Updated

3 years ago
Blocks: 1196858
You need to log in before you can comment on or make changes to this bug.