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

RESOLVED FIXED in Bugzilla 2.16

Status

()

--
blocker
RESOLVED FIXED
17 years ago
6 years ago

People

(Reporter: bbaetz, Assigned: zach)

Tracking

2.15
Bugzilla 2.16
x86
Linux

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

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

Updated

17 years ago
Target Milestone: --- → Bugzilla 2.16
Created attachment 80893 [details] [diff] [review]
patch v1: rectifies situation
(Assignee)

Comment 2

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

r1=zach
Attachment #80893 - Flags: review+
(Reporter)

Comment 3

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

r=bbaetz, obvious
Attachment #80893 - Flags: review+
(Reporter)

Comment 4

17 years ago
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

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

Marking needs-work per comment #4.
Attachment #80893 - Flags: review-

Comment 6

17 years ago
Created attachment 80981 [details] [diff] [review]
patch v2: rectifies situation + change group

This patch includes patch 80893 and fixes additionally the permissions for the
group of data/params.
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.
Attachment #80981 - Flags: review-
Created attachment 81050 [details] [diff] [review]
patch v3: fixes permissions problem in v2
Attachment #80893 - Attachment is obsolete: true
Attachment #80981 - Attachment is obsolete: true
Created attachment 81058 [details] [diff] [review]
patch v4: puts permissions fixing in the right place
Attachment #81050 - Attachment is obsolete: true
(Reporter)

Comment 10

17 years ago
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.
Attachment #81058 - Flags: review+
Keywords: patch, review

Comment 11

17 years ago
> >+      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

17 years ago
> >+      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!!
(Reporter)

Comment 13

17 years ago
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

17 years ago
> 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.)
Warnings are good, filed as bug 140355.  Now can someone give this bug a second
review?
Comment on attachment 81058 [details] [diff] [review]
patch v4: puts permissions fixing in the right place

r=gerv.

Gerv
Attachment #81058 - Flags: review+
Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.138; previous revision: 1.137
done
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.