183188 - data/mining is world writable after each run of collectstats.pl

Status: RESOLVED FIXED

(Bugzilla :: Reporting/Charting, defect)

Description

[Christian Franke]

User-Agent:       Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.2) Gecko/20021126
Build Identifier: 

data/mining is world writable after each run of collectstats.pl

Reproducible: Always

Steps to Reproduce:
1. $ cd /somewhere/bugs
2. $ chmod og-w data/mining
3. $ ./collectstats.pl
4. $ ls -ld data/mining
   drwxrwxrwx ............ data/mining

Actual Results:  
data/mining is world writable after each run of collectstats.pl

Expected Results:  
chmod 0777,data/mining only if dir created by collectstats.pl

Comment 1

[Christian Franke]

Attachment: Patch to collectstats.pl which WORKSFORME ;-)

Comment 2

[Gervase Markham [:gerv]]

Marking security-sensitive. I will evaluate this tonight.

Gerv

Comment 3

[Dave Miller [:justdave]]

Christian:  do you have a value set for $webservergroup in localconfig?

If you don't, that's the intended behaviour, because that directory had to be
world-writable for both the cron job and the webserver to get at it, since they
run as different users.

If you are using a webservergroup, then this is not as bad as it sounds, because
the data directory itself should not be world-readable, so the world-whatever
access on anything inside it is not quite so relevant.  You're still only going
to have access to it from people who are also in the webservergroup, which is
going to have write access to it anyway.

My opinion is this is not a security issue because of the above.

Comment 4

[Dave Miller [:justdave]]

OK, I take that back, with a webservergroup, the data directory is going to be
chmod 771, which does make it world-accessible (but not world-readable), which
means anything world-readable inside it will be accessible if they know the
filename, so yes this is an issue, if it's still doing that with $webservergroup
set.

Comment 5

[Dave Miller [:justdave]]

confirmed on landfill.  collectstats.pl is ignoring $webservergroup, and making
it world-writable regardless.

Comment 6

[Bradley Baetz (:bbaetz)]

collectstats does (for each product)

sub check_data_dir {
    my $dir = shift;

    if (! -d) {
        mkdir $dir, 0777;
        chmod 0777, $dir;
    }
}

with an arg of "data/mining".
This shouldn't be needed, because checketup sets this stuff up, but since it is
a directory this code shouldn't be firing anyway. We should jus remove this
check, but why is teh chmod being hit at all? (Theres no other chmod call in
that file)

Comment 7

[Dave Miller [:justdave]]

my $dir = shift

Doesn't that remove the $_ that was the original argument to the sub that -d
will act on if it there's no argument to -d?  That test probably needs to be 
if (! -d $dir) {

Comment 8

[Christian Franke]

Dave: I agree with comment 7. Look at my patch attached in comment 1, this is
exactly the change which works for us ;-)

This bug may not be visible on all platforms: Depending on perl and/or OS
version, the expression (! -d) (same as (! -d $_) always and (! -d "") here) may
be always true, because the empty string may be considered equivalent to ".". I
don't know for sure.

Bradley: I agree with comment 6.
collectstat.pl should simply die if (! -d "data/mining");
Only checksetup.pl should handle directory creation and permissions.

Comment 9

[Bradley Baetz (:bbaetz)]

Yeah, I thought we pulled all the mkdir stuff a while back - must have missed one

Comment 10

[Dave Miller [:justdave]]

From Comment #2 (Gervase Markham  2002-12-03 04:11)

> Marking security-sensitive. I will evaluate this tonight.

ping?

Comment 11

[Gervase Markham [:gerv]]

Well, it looked like you guys have already done the evaluation :-)

Gerv

Comment 12

[Dave Miller [:justdave]]

Comment on attachment 108014 [details] [diff] [review]
Patch to collectstats.pl which WORKSFORME ;-)

while this doesn't remove the mkdir, it does seem to solve the security
issue....

Comment 13

[Bradley Baetz (:bbaetz)]

Comment on attachment 108014 [details] [diff] [review]
Patch to collectstats.pl which WORKSFORME ;-)

yeah, ok, r=bbaetz

Comment 14

[Dave Miller [:justdave]]

-> patch author

Comment 15

[Dave Miller [:justdave]]

checked in on trunk:

Checking in collectstats.pl;
/cvsroot/mozilla/webtools/bugzilla/collectstats.pl,v  <--  collectstats.pl
new revision: 1.24; previous revision: 1.23
done

checked in on BUGZILLA-2_16-BRANCH:

Checking in collectstats.pl;
/cvsroot/mozilla/webtools/bugzilla/collectstats.pl,v  <--  collectstats.pl
new revision: 1.20.12.1; previous revision: 1.20
done

checked in on BUGZILLA-2_14_1-BRANCH:

Checking in collectstats.pl;
/cvsroot/mozilla/webtools/bugzilla/collectstats.pl,v  <--  collectstats.pl
new revision: 1.20.10.1; previous revision: 1.20
done

same patch applied cleanly to all three.

Comment 16

[Dave Miller [:justdave]]

public notice has been made, clearing security flag