99024 - checksetup doesn't give template proper permissions.

Status: RESOLVED FIXED

(Bugzilla :: Installation & Upgrading, defect, P1)

Description

[Matthew Tuck [:CodeMachine]]

checksetup.pl should make the template directory and its subdirectories have
correct permissions.

Currently it doesn't, and I spent quite a while trying to work out why I
couldn't emit templates.

Comment 1

[Zach Lipton [:zach]]

Can someone tell me what the correct permissions are. I would be happy 
to fix this if this is known.

Comment 2

[Jacob Steenhagen]

I think it'd be 750 w/$webservgroup and 755 w/out.

Comment 3

[Zach Lipton [:zach]]

Index: checksetup.pl
===================================================================
RCS file: /cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v
retrieving revision 1.104
diff -u -2 -r1.104 checksetup.pl
--- checksetup.pl       2001/09/23 17:07:55     1.104
+++ checksetup.pl       2001/10/07 19:56:55
@@ -80,5 +80,5 @@
 #     change table definitions                         --TABLE--
 #     add more groups                                  --GROUPS--
-#     create initial administrator account            --ADMIN--
+#     create initial administrator account             --ADMIN--
 #
 # Note: sometimes those special comments occur more then once. For
@@ -740,4 +740,5 @@
     chmod 0771, 'data';
     chmod 0770, 'graphs';
+    system("chmod -R 755 template");
 } else {
     # get current gid from $( list

Comment 4

[Zach Lipton [:zach]]

Try this:

Index: checksetup.pl
===================================================================
RCS file: /cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v
retrieving revision 1.104
diff -u -2 -r1.104 checksetup.pl
--- checksetup.pl       2001/09/23 17:07:55     1.104
+++ checksetup.pl       2001/10/07 20:04:25
@@ -80,5 +80,5 @@
 #     change table definitions                         --TABLE--
 #     add more groups                                  --GROUPS--
-#     create initial administrator account            --ADMIN--
+#     create initial administrator account             --ADMIN--
 #
 # Note: sometimes those special comments occur more then once. For
@@ -740,4 +740,5 @@
     chmod 0771, 'data';
     chmod 0770, 'graphs';
+    system("chmod -R 755 template");
 } else {
     # get current gid from $( list
@@ -746,4 +747,5 @@
     fixPerms('*',022);
     chmod 01777, 'data', 'graphs';
+    system("chmod -R 755 template");
 }
 

Comment 5

[Christian Reis]

This first patch didn't cover the else clause for $webservergroup users - I see
this is fixed, thanks. The second one ignores the security problem of an invalid
path: if the user running checksetup.pl has path set insecurely and a trojanized
chmod, we might cause trouble.

IMHO we should check for /usr/bin and /bin and run chmod. But this makes bbaetz
ask: what about windowsNT. So I look up:

http://aspn.activestate.com/ASPN/Reference/Products/ActivePerl/faq/Windows/ActivePerl-Winfaq4.html#What_s_the_equivalent_of_chmod_f

And realize that chmod() does work on NTFS, so we should use perl chmod to keep
portability. Do the recursion, zach. Sorry. :-)

Comment 6

[Dave Miller [:justdave]]

I was going to question that anyway...  are the template files supposed to be
executable?  I would think you'd only want 644 for the files, and 755 for the
folders.  (and you probably don't want the CVS folders to be 755, those should
be 700)

Comment 7

[Dave Miller [:justdave]]

This is going to be a blocker for the 2.16 release

Comment 8

[Gervase Markham [:gerv]]

Someone needs to establish definitively what the permissions should be.

Gerv

Comment 9

[Zach Lipton [:zach]]

This has reached an extremely critical stage as this has to be in 2.16. I 
don't know what the permissions need to be, can someone who knows 
for sure please experiment and post results here so I can write a patch?

Comment 10

[Christian Reis]

And what about the recursion issue?

Comment 11

[Dave Miller [:justdave]]

Attachment: add "do directories" optional param to fixPerms() and make it recursive

Comment 12

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

Comment on attachment 65133 [details] [diff] [review]
add "do directories" optional param to fixPerms() and make it recursive

>Index: checksetup.pl
  ...
>@@ -780,7 +790,8 @@
>     if (-e ".htaccess") { chown $<, $webservergid, ".htaccess" } # glob('*') doesn't catch dotfiles
>     if (-e "data/.htaccess") { chown $<, $webservergid, "data/.htaccess" }
>     if (-e "data/webdot/.htaccess") { chown $<, $webservergid, "data/webdot/.htaccess" }

Any reason not to recursively chown all files inside the Bugzilla
install directory like this?  Seems like the template hierarchy
should also be chowned in this way.

Comment 13

[Christian Reis]

Comment on attachment 65133 [details] [diff] [review]
add "do directories" optional param to fixPerms() and make it recursive

What Myk said; however, careful with the data directory since at least params
and versioncache need to be writeable by webservergroup.

That, minus the printfs, grant r=kiko

Comment 14

[Andreas Franke (gone)]

With respect to the printf's: it may make sense to introduce a "--verbose", or
"-v" option that prints debug information like this.

Such an option would have been useful when I had the problem described in bug
123165. 

Comment 15

[Zach Lipton [:zach]]

I like the idea of adding a verbose option to checksetup.pl, but 2.16 crunch 
time isn't the time to do it. It is already coded into the newinstall system 
(2.20?) along with a quiet mode for use with automatic update scripts and 
such. 

Comment 16

[Dave Miller [:justdave]]

Attachment: handles file ownership as well

Comment 17

[Dave Miller [:justdave]]

Just a note for whoever's reviewing this, note that the "do_dirs" flag is NOT
set on the fixPerms('*'), so directories are not recursed.  They are recursed
for the individual call to fixPerms('template').  This means that the contents
of the data directory are treated no differently than they were before this patch.

Comment 18

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

Comment on attachment 67697 [details] [diff] [review]
handles file ownership as well

>Index: checksetup.pl

>+        if (!(-d $file)) {
>+            # check if the file is executable.
>+            if (isExecutableFile($file)) {
>+                #printf ("Changing $file to %o\n", $execperm);
>+                chown $owner, $group, $file;
>+                chmod $execperm, $file;
>+            } else {
>+                #printf ("Changing $file to %o\n", $normperm);
>+                chown $owner, $group, $file;
>+                chmod $normperm, $file;
>+            }
>+        }

"chown $owner, $group, $file;" should be placed before the 
isExecutableFile conditional since it gets run regardless.


>+        elsif ($do_dirs) {
>+            if ($file =~ /CVS$/) {
>+                chown $owner, $group, $file;
>+                chmod 0700, $file;
>+            }
>+            else {
>+                #printf ("Changing $file to %o\n", $execperm);
>+                chown $owner, $group, $file;
>+                chmod $execperm, $file;
>+                fixPerms("$file/*", $owner, $group, $umask, $do_dirs); # do the contents of the directory
>+            }
>         }

Same comment as the previous one.

Otherwise the patch looks good and works fine.	One more issue, however:

I noticed while testing out this patch that template files are loadable
in raw form from the URL line.	Should this be possible?  My instinctive
reaction tells me it shouldn't.

Comment 19

[Dave Miller [:justdave]]

Attachment: adds .htaccess file for template and consolidates chown calls

Comment 20

[Zach Lipton [:zach]]

Comment on attachment 68503 [details] [diff] [review]
adds .htaccess file for template and consolidates chown calls

r=zach x2

Comment 21

[Dave Miller [:justdave]]

-> patch author

Comment 22

[Dave Miller [:justdave]]

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

Comment 23

[Bradley Baetz (:bbaetz)]

You need to create template/.cvsignore, containing only .htaccess.

Comment 24

[Bradley Baetz (:bbaetz)]

Checked in .cvsignore, r=justdave x2 (on IRC)