714241 - collectstats.pl complains a lot if run as non-root

Status: ASSIGNED

(Bugzilla :: Reporting/Charting, defect)

Description

[Marc Schumann [:Wurblzap]]

This is a regression of bug 368250, affecting 4.2 and trunk.

When run as non-root, collectstats.pl complains loudly because of newly introduced chown/chmod calls:

./data/mining: Failed to change ownership: Operation not permitted
./data/mining: Failed to change permissions: Operation not permitted
./data/mining: Failed to change ownership: Operation not permitted
./data/mining: Failed to change permissions: Operation not permitted
./data/mining/ProductA: Failed to change ownership: Operation not permitted
./data/mining/ProductA: Failed to change permissions: Operation not permitted
./data/mining/ProductB: Failed to change ownership: Operation not permitted
./data/mining/ProductB: Failed to change permissions: Operation not permitted
...

These messages clobber cronmail, which is a nuisance. The stats still get collected, though.

Comment 1

[A. Shimono [:himorin]]

Note.
This occurs when installed as root, set web server account in localconfig.
Some directory in data which should be in 660 have permission in 600.

# occurs from 4.1.2 or so.

Comment 2

[Marc Schumann [:Wurblzap]]

Attachment: Patch

This fixes things.

Tested scenarios:
 - no webservergroup, collectstats.pl run by root
 - webservergroup defined, collectstats.pl run by root anyway
 - webservergroup defined, collectstats.pl run by a user of the webservergroup

Comment 3

[Frédéric Buclin]

One question from mkanat on IRC: how does this code behave when suexec is enabled?

Comment 4

[Marc Schumann [:Wurblzap]]

I can't test with suexec, but from my understanding this should be exactly equal to the third mentioned scenario.

Comment 5

[Frédéric Buclin]

Comment on attachment 628622 [details] [diff] [review]
Patch

>=== modified file 'collectstats.pl'

>+umask 0022;

To be sure, if a file is created with this umask, the file won't be writable by the webserver group, right? In that case, how do you plan to update the file the next time collectstats.pl is executed?


> if (!-d $dir) {
>+    die 'Run checksetup.pl as ' . ROOT_USER .
>+        " before running collectstats.pl for the first time.\n";
> }

This bothers me. Imagine that you don't have root access at all, and there is no way to get an admin to execute checksetup.pl for you (e.g. on a remote server shared between many users, all with restricted privileges); in that case, you would be unable to run this script at all.

Comment 6

[Marc Schumann [:Wurblzap]]

(In reply to Frédéric Buclin from comment #5)
> >+umask 0022;
> 
> To be sure, if a file is created with this umask, the file won't be writable
> by the webserver group, right? In that case, how do you plan to update the
> file the next time collectstats.pl is executed?

The data/mining/ directory has its flags set to 02770, so it'll be created as belonging to the webserver group.

> > if (!-d $dir) {
> >+    die 'Run checksetup.pl as ' . ROOT_USER .
> >+        " before running collectstats.pl for the first time.\n";
> > }
> 
> This bothers me. Imagine that you don't have root access at all, and there
> is no way to get an admin to execute checksetup.pl for you (e.g. on a remote
> server shared between many users, all with restricted privileges); in that
> case, you would be unable to run this script at all.

If you want, I can drop the ROOT_USER part of this sentence. Or, I can write "... or as a member of the webservergroup...", but I think this is confusing. With this patch, checksetup.pl doesn't need more root access than before. You can run checksetup.pl as the same regular user who extracted Bugzilla from its .tar.gz, and the data/mining/ will be created, so you'll be fine.

Comment 7

[Frédéric Buclin]

Comment on attachment 628622 [details] [diff] [review]
Patch

The existing code, including Bugzilla 3.x, sets permissions to 640:

-rw-r----- 1 root apache  290 oct 25  2010 TestProduct

With your patch applied, checksetup.pl sets permissions to 660:

-rw-rw---- 1 root apache 150K jun 26 15:58 TestProduct

But with your patch applied, collectstats.pl sets permissions to 644 for newly created products, which is inconsistent with what checksetup.pl does:

-rw-r--r-- 1 root apache  318 jun 26 16:19 Pulpo

Also, with permissions set to 644, a user being in the webserver group cannot write to the files. So it looks like the umask should be set to 0017 instead of 0022. Do you agree with that?


While you are on it, do not forget to fix the confusing comment about ROOT_USER.

Comment 8

[Frédéric Buclin]

Same root cause as bug 511340?

Comment 9

[Marc Schumann [:Wurblzap]]

It's a very related issue, I'd say. In both cases, a non-root user tries to chmod/chown other users' files.

Comment 10

[Frédéric Buclin]

Bugzilla 4.4 is now restricted to security fixes only.