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 on attachment 80893 [details] [diff] [review] patch v1: rectifies situation r1=zach

Comment on attachment 80893 [details] [diff] [review] patch v1: rectifies situation r=bbaetz, obvious

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 on attachment 80893 [details] [diff] [review] patch v1: rectifies situation Marking needs-work per comment #4.

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.

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.

> >+ 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);

> >+ 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!!

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?

> 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

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