134575 - defparams.pl::WriteParams makes data directory world writable

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Bradley Baetz (:bbaetz)]

If the dir isn't there, it tires to create it, and make it world writable. This
is a bad idea - if it doesn't exist, we should die.

The params file is also world writable, which is even a worse idea, because even
with a webservergroup, the perms are 771, so if you know the name of the file, a
local user can overwrite it, and add code to do whatever they want:

if ($::userid = 12345) { $::usergroupset = whatever; }

for example. How can defparams.pl check if a webservergroup was specified?

How about we move the WriteParams stuff into checksetup.pl? This would also mean
that params can be used in checksetup in a cleaner way.

Changing params would then just copy the perms on the existing file. This
wouldn't help the non-webservergroup case, though - see bug 107513.

Comment 1

[Bradley Baetz (:bbaetz)]

-> 2.16, although if you have a better suggestion, feel free to take this.

Comment 2

[Dave Miller [:justdave]]

CCing the usual suspects.

Comment 3

[Gervase Markham [:gerv]]

Our permissions story is, in general, a complete mess. We really need to make a
plan of exactly what data we have, what permissions it should have, and where we
are storing it.

Is there a quick 2.16 fix for this, rather than moving WriteParams into
checksetup, which sounds scary?

Gerv

Comment 4

[Bradley Baetz (:bbaetz)]

Yeah, we could probably just see what perms the data dir had to work out if a
webserver group was set in localconfig. Its a bit of a hack, but...

Comment 5

[Bradley Baetz (:bbaetz)]

Attachment: v1

OK, here we go. This fixes all teh places we call chmod, I think. Except for
data/mining, which isn't created by checksetup for some reason. That is one
which may be running as a different user to the webserver, though.

The changes for importxml.pl and move.pl are untested - is there a case where
the user running those scripts (mainly importxml.pl) will not have write access
to the dir, but will be able to change the permissions on the directory? I
don't think that that is possible.

Comment 6

[Dave Miller [:justdave]]

Comment on attachment 77567 [details] [diff] [review]
v1

r= justdave

Comment 7

[Gervase Markham [:gerv]]

Comment on attachment 77567 [details] [diff] [review]
v1

>+        open(LOCKFID, ">>data/maillock") || die "Can't open lockfile.";

I hate error messages like this. While you are there, please give 
the name of the file you can't open, and print $! also. In both files.

Other than that, this seems OK. r=gerv.

Gerv

Comment 8

[Bradley Baetz (:bbaetz)]

One issue per bug, Gerv...

Anyway, checked in as:

+        open(LOCKFID, ">>data/maillock") || die "Can't open data/maillock: $!";

I couldn't test it though - I can't get this error to trigger at all, or at
least I can't get the results to display for me in the browser.

Comment 9

[J. Paul Reed [:preed]]

Is there a preference on using "die" vs. "confess"?

I don't know the difference between the two, but I *thought* it was that die
just caused the script to bomb out and print stuff on STDERR, whereas confess
printed out those nice "Software Error" messages.

Am I wrong, or?

Does BZ have a style preference? If so, should it go in the coding manual?

Comment 10

[Bradley Baetz (:bbaetz)]

No clue. I didn't add the die(), I just moved it. File a separate bug if you
think that there are problems.

Comment 11

[Gervase Markham [:gerv]]

bbaetz: excellent, exactly what I wanted.

preed: there's no preference; but, in the future, that sort of thing should
ThrowCodeError(). 

Gerv

Comment 12

[Bradley Baetz (:bbaetz)]

gerv: ThrowCodeError is not appropriate for a script which is run from procmail
(ie importxml.pl)

Comment 13

[Dave Miller [:justdave]]

munging ccs

Comment 14

[J. Paul Reed [:preed]]

Attachment: Backported patch for BUGZILLA-2_14_1-BRANCH

Comment 15

[Bradley Baetz (:bbaetz)]

2.14 had u+s on data for non webserver group, and I don't think all the fixPerm
stuff was the same. It was certainly less involved. If we have +s, do we need
all of this?

Its definately a bug, though, and since I wasnt'a rround for 2.14 I don't know
whether any of this is a security issue.

Do we want/need this for 2.14.2? Is it necessary, or will it break stuff? Maybe
we should just remove teh mkdir/chmod for 'data', which definately isn't needed,
and leave the rest to 2.16?

Comment 16

[Bradley Baetz (:bbaetz)]

... ie skip the ChmodDataFile stuff, and just apply the rest.

Comment 17

[Dave Miller [:justdave]]

Adding representatives of the packagers to bugs that are going into the
Bugzilla 2.14.2 security update

Comment 18

[Dave Miller [:justdave]]

Comment on attachment 83022 [details] [diff] [review]
Backported patch for BUGZILLA-2_14_1-BRANCH

r= justdave

Yeah, but this will work.  Just because it wasn't done that way for 2.14.x to
begin with doesn't mean we can't fix it for this.

Comment 19

[Bradley Baetz (:bbaetz)]

moving secure bugzilla/webtools bugs from mozilla security group to the new
bugzilla security group.

Comment 20

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

Comment on attachment 83022 [details] [diff] [review]
Backported patch for BUGZILLA-2_14_1-BRANCH

After applying this patch editparams.cgi and doeditparams.cgi 
couldn't find defparams.cgi in @INC until I re-ran checksetup.pl.
This should be documented in the release notes for 2.14.2
if it isn't just a peculiarity of my system.

Comment 21

[Dave Miller [:justdave]]

The release notes already say to run checksetup.pl after applying the upgrade. 
Just like they always do. :-)  I don't see a problem.

JayPee: go ahead and check this in on the 2_14_1 branch

Comment 22

[J. Paul Reed [:preed]]

Yes, oh Captain, my Captain:

Checking in defparams.pl;
/cvsroot/mozilla/webtools/bugzilla/defparams.pl,v  <--  defparams.pl
new revision: 1.56.2.1; previous revision: 1.56
done
Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.110.2.3; previous revision: 1.110.2.2
done
Checking in importxml.pl;
/cvsroot/mozilla/webtools/bugzilla/importxml.pl,v  <--  importxml.pl
new revision: 1.18.2.1; previous revision: 1.18
done
Checking in move.pl;
/cvsroot/mozilla/webtools/bugzilla/move.pl,v  <--  move.pl
new revision: 1.6.10.1; previous revision: 1.6
done

Comment 23

[Dave Miller [:justdave]]

2.14.2 is out, removing security group.