139930 - checksetup.pl fails if data/params doesn't exist

Status: RESOLVED FIXED

(Bugzilla :: Installation & Upgrading, defect)

Description

[Bradley Baetz (:bbaetz)]

Bug 126571 went in, but that requries data/parmas without checking that it
exists.  that block should be wrapped in an |if (-e 'data/params')| check

Note that if it does exist, the graphviz check will have brought it in earlier.
Thats a separate very minor optimisation issue, though

2.16 blocker, since we do want new installations to work

Comment 1

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

Attachment: patch v1: rectifies situation

Comment 2

[Zach Lipton [:zach]]

Comment on attachment 80893 [details] [diff] [review]
patch v1: rectifies situation

r1=zach

Comment 3

[Bradley Baetz (:bbaetz)]

Comment on attachment 80893 [details] [diff] [review]
patch v1: rectifies situation

r=bbaetz, obvious

Comment 4

[Bradley Baetz (:bbaetz)]

This is necessary, but not sufficient - checksetup.pl now calls WriteParams,
which runs as the user running the script. So the params file is now:

-rw-rw----    1 root     root         7397 Apr 25 21:46 data/params

and since my apache doesn't run as root, we fail.

Comment 5

[David D. Kilzer (ddk)]

Comment on attachment 80893 [details] [diff] [review]
patch v1: rectifies situation

Marking needs-work per comment #4.

Comment 6

[Tobias Burnus]

Attachment: patch v2: rectifies situation + change group

This patch includes patch 80893 and fixes additionally the permissions for the
group of data/params.

Comment 7

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

Comment on attachment 80981 [details] [diff] [review]
patch v2: rectifies situation + change group

>Index: checksetup.pl

>+      my $gid = (split " ", $()[0];
>+      fixPerms("data/params",$<,$gid,017);

This needs to be 011 or the web server user won't be able
to read from or write to the file.

Comment 8

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

Attachment: patch v3: fixes permissions problem in v2

Comment 9

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

Attachment: patch v4: puts permissions fixing in the right place

Comment 10

[Bradley Baetz (:bbaetz)]

Comment on attachment 81058 [details] [diff] [review]
patch v4: puts permissions fixing in the right place

r=bbaetz Looks fine, not in a position to test now, though.

Comment 11

[Tobias Burnus]

> >+      my $gid = (split " ", $()[0];
> >+      fixPerms("data/params",$<,$gid,017);
>
> This needs to be 011 or the web server user won't be able
> to read from or write to the file.

Is than this ok? (attachment 81058 [details] [diff] [review])
| +    fixPerms('data/params', $<, $webservergid, 017);
| @@ -875,6 +888,7 @@
| +    fixPerms('data/params', $<, $gid, 011);

Comment 12

[Tobias Burnus]

> >+      my $gid = (split " ", $()[0];
> >+      fixPerms("data/params",$<,$gid,017);
>
> This needs to be 011 or the web server user won't be able
> to read from or write to the file.
Well I get:
017: -rw-rw----   root www-data
011: -rw-rw-rw-   root www-data

I actually prefer 017 to a world writable file!!

Comment 13

[Bradley Baetz (:bbaetz)]

The entire data directory is world writable if you don't have a webserver group
enabled. So technically, we could get away with the file only being world
readable, since the webserver will be able to overwrite the file anyway. That
doens't affect the security, of course.

This used to be +t, but that broke stuff - see bug 107513.

You really should use a webservergroup on any real installation, though, for
security reasons. Maybe we should document this better, along the lines of "YOU
REALLY REALLY REALLY WANT TO USE THIS OPTION!!!!", and say this in checksetup.

OTOH, if your webserver does run as root, then thats not needed, I suppose.
However, if your webserver does run as root for all cgi scripts, then no matter
what the permissions are, anyone could hack up a quick cgi script to do whatever
they wanted. If it only runs as root for some of them, that implies that you
have acccess to the root account, and can set the webservergroup to www-data and
run checksetup as root.

Or was the ls -l output from you running checksetup as root?

gerv, justdave, myk: comments on the above?

Comment 14

[Tobias Burnus]

> The entire data directory is world writable if you don't have a webserver
> group enabled. So technically, we could get away with the file only being
> world readable, since the webserver will be able to overwrite the file anyway.
> That doens't affect the security, of course.
Ok. I've missed that such permissions are set in general. But I still don't like
the idea of having world writable files. I think a warning should be printed
every time ./checksetup is running. (And of cause I have my webservergroup set
-- I observed this only when testing this patch and didn't know/realise that
this was intentional.)

Comment 15

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

Warnings are good, filed as bug 140355.  Now can someone give this bug a second
review?

Comment 16

[Gervase Markham [:gerv]]

Comment on attachment 81058 [details] [diff] [review]
patch v4: puts permissions fixing in the right place

r=gerv.

Gerv

Comment 17

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

Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.138; previous revision: 1.137
done