Closed Bug 134575 Opened 23 years ago Closed 22 years ago makes data directory world writable


(Bugzilla :: Bugzilla-General, defect)

Not set



Bugzilla 2.16


(Reporter: bbaetz, Assigned: bbaetz)


(Whiteboard: applied to 2.14.2)


(2 files)

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

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...
Attached patch v1Splinter 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.
Comment on attachment 77567 [details] [diff] [review]

r= justdave
Attachment #77567 - Flags: review+
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+
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.
Closed: 22 years ago
Resolution: --- → FIXED
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

gerv: ThrowCodeError is not appropriate for a script which is run from procmail
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.
Attachment #83022 - Flags: review+
moving secure bugzilla/webtools bugs from mozilla security group to the new
bugzilla security group.
Group: security? → webtools-security?
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.