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.

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

CCing the usual suspects.

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

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...

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 on attachment 77567 [details] [diff] [review] v1 r= justdave

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

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.

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?

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

bbaetz: excellent, exactly what I wanted. preed: there's no preference; but, in the future, that sort of thing should ThrowCodeError(). Gerv

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

munging ccs

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?

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

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

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.

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

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.

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

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

2.14.2 is out, removing security group.