201554 - checksetup.pl gives un-necessary warning to non-root user with webservergroup access

Status: RESOLVED FIXED

(Bugzilla :: Installation & Upgrading, defect)

Description

[Dave Miller [:justdave]]

If you have $webservergroup set, but run checksetup.pl as a non-root user, you
get the following warning:

---------
Warning: you have entered a value for the "webservergroup" parameter
in localconfig, but you are not running this script as root.
This can cause permissions problems and decreased security.  If you
experience problems running Bugzilla scripts, log in as root and re-run
this script, or remove the value of the "webservergroup" parameter.
Note that any warnings about "uninitialized values" that you may
see below are caused by this.
---------

If the user is not root, but *is* a member of the group named in
$webservergroup, then this warning is unnecessary.  We should test for it and
skip the warning if that's the case.

Suggested way to test for it is to attempt to chgrp a file to $webservergroup
and either test for errors and/or go back and see if it actually changed.

Comment 1

[Dave Miller [:justdave]]

*** Bug 246196 has been marked as a duplicate of this bug. ***

Comment 2

[Joel Peshkin]

Wouldn't it be simpler to just compare the groupid of webservergroup with $( ??

Comment 3

[Dave Miller [:justdave]]

Hmm, suppose it would.  $( in list context does indeed give all of your groups
and not just the primary.  $) might be better though (effective group), which
also gives all of them when referenced in list context.

Comment 4

[Rob Siklos [:robzilla]]

Attachment: rsiklos_v1

Patch for Trunk

Comment 5

[Rob Siklos [:robzilla]]

Attachment: rsiklos_v1-218

Patch for 2.18 branch

Comment 6

[Vlad Dascalu]

Comment on attachment 168229 [details] [diff] [review]
rsiklos_v1

+    my $webservergid = (getgrnam($my_webservergroup))[2];

Is there any chance of $webservergid to be undefined? Or is it guaranteed that
getgrnam will always return a result? If there is a chance to be undef, the
grep below will emit an undef warning and strange results. :-)

+localconfig, but you are not either a) running this script as $::root; or b) a 

Not sure about:

"not either" --> "neither"
"or" --> "nor"

but since I'm not a native English speaker, maybe the one evaluating the
approval could put some light on this.

Comment 7

[:glob ✱]

Comment on attachment 168229 [details] [diff] [review]
rsiklos_v1

>+    my $webservergid = (getgrnam($my_webservergroup))[2];
>+    if ($< != 0 && !grep(/^$webservergid$/, split(" ", $)))) {

this will break on win32 (getgrnam is not implemented).

Comment 8

[:glob ✱]

Comment on attachment 168230 [details] [diff] [review]
rsiklos_v1-218

>-    if ($< != 0) { # zach: if not root, yell at them, bug 87398 
>+    my $webservergid = (getgrnam($my_webservergroup))[2];
>+    if ($< != 0 && !grep(/^$webservergid$/, split(" ", $)))) {

same win32 breakage.

Comment 9

[Vlad Dascalu]

Comment on attachment 168229 [details] [diff] [review]
rsiklos_v1

I thought about that as well but somehow my brain thought $my_webservergroup is
undefined in Windows.

Maybe we should do that instead (undef $my_webservergroup somewhere up in the
.cgi); that would save us from checking the platform twice.

Comment 10

[Vlad Dascalu]

Rob, can you upload new patches in which you stick the new code inside a:

if ($^O !~ /MSWin32/i) {
    // new, Unix-only code goes here
}

block? Thanks! :)

Comment 11

[Rob Siklos [:robzilla]]

Attachment: rsiklos_v2-218

Ok, this should be better.

Comment 12

[Rob Siklos [:robzilla]]

Attachment: rsiklos_v2

patch v2 for trunk

Comment 13

[Dave Miller [:justdave]]

Nit: the $::root variable contains "Administrator" on Win32 and "root"
otherwise.  This chunk of text is now inside a block that prevents it from
running on Win32, so the use of the variable for it is unnecessary.

Comment 14

[Vlad Dascalu]

I thought about the $::root stuff as well, but then I thought that maybe
sometimes we will be able to control ACL (access lists) from NTFS from Perl :-),
and a Windows version of getgrnam will be available :) Heh, one can only dream :)

But anyway, in the case where we will be able to port this code on Windows as
well, file permissions security-wise, I won't like to replace "root" with
"$::root" back again. So I guess leaving it as $::root for now doesn't make any
harm.

Comment 15

[Vlad Dascalu]

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

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