makes data directory world writable

RESOLVED FIXED in Bugzilla 2.16



17 years ago
6 years ago


(Reporter: bbaetz, Assigned: bbaetz)


Bugzilla 2.16


(Whiteboard: applied to 2.14.2)


(2 attachments)



17 years ago
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 check if a webservergroup was specified?

How about we move the WriteParams stuff into 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

17 years ago
-> 2.16, although if you have a better suggestion, feel free to take this.
Target Milestone: --- → Bugzilla 2.16
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?


Comment 4

17 years ago
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

17 years ago
Created attachment 77567 [details] [diff] [review]

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 and are untested - is there a case where
the user running those scripts (mainly 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.
Keywords: patch, review
Comment on attachment 77567 [details] [diff] [review]

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

Attachment #77567 - Flags: review+

Comment 8

17 years ago
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.
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 9

17 years ago
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

17 years ago
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


Comment 12

17 years ago
gerv: ThrowCodeError is not appropriate for a script which is run from procmail
Created attachment 83022 [details] [diff] [review]
Backported patch for BUGZILLA-2_14_1-BRANCH

Comment 15

17 years ago
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

17 years ago
... 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.
Attachment #83022 - Flags: review+

Comment 19

17 years ago
moving secure bugzilla/webtools bugs from mozilla security group to the new
bugzilla security group.
Group: security? → webtools-security?


17 years ago
Attachment #83022 - Flags: review+
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
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 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
Whiteboard: wanted for 2.14.2
Yes, oh Captain, my Captain:

Checking in;
/cvsroot/mozilla/webtools/bugzilla/,v  <--
new revision:; previous revision: 1.56
Checking in;
/cvsroot/mozilla/webtools/bugzilla/,v  <--
new revision:; previous revision:
Checking in;
/cvsroot/mozilla/webtools/bugzilla/,v  <--
new revision:; previous revision: 1.18
Checking in;
/cvsroot/mozilla/webtools/bugzilla/,v  <--
new revision:; previous revision: 1.6
Whiteboard: wanted for 2.14.2 → applied to 2.14.2
2.14.2 is out, removing security group.
Group: webtools-security?
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.