Closed
Bug 134575
Opened 23 years ago
Closed 23 years ago
defparams.pl::WriteParams makes data directory world writable
Categories
(Bugzilla :: Bugzilla-General, defect)
Tracking
()
RESOLVED
FIXED
Bugzilla 2.16
People
(Reporter: bbaetz, Assigned: bbaetz)
Details
(Whiteboard: applied to 2.14.2)
Attachments
(2 files)
3.55 KB,
patch
|
justdave
:
review+
gerv
:
review+
|
Details | Diff | Splinter Review |
3.51 KB,
patch
|
justdave
:
review+
tara
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•23 years ago
|
||
-> 2.16, although if you have a better suggestion, feel free to take this.
Target Milestone: --- → Bugzilla 2.16
Comment 2•23 years ago
|
||
CCing the usual suspects.
Comment 3•23 years ago
|
||
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
Assignee | ||
Comment 4•23 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...
Assignee | ||
Comment 5•23 years ago
|
||
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•23 years ago
|
||
Comment on attachment 77567 [details] [diff] [review]
v1
r= justdave
Attachment #77567 -
Flags: review+
Updated•23 years ago
|
Comment 7•23 years ago
|
||
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
Attachment #77567 -
Flags: review+
Assignee | ||
Comment 8•23 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.
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Comment 9•23 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?
Assignee | ||
Comment 10•23 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.
Comment 11•23 years ago
|
||
bbaetz: excellent, exactly what I wanted.
preed: there's no preference; but, in the future, that sort of thing should
ThrowCodeError().
Gerv
Assignee | ||
Comment 12•23 years ago
|
||
gerv: ThrowCodeError is not appropriate for a script which is run from procmail
(ie importxml.pl)
Comment 13•23 years ago
|
||
munging ccs
Comment 14•23 years ago
|
||
Assignee | ||
Comment 15•23 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?
Assignee | ||
Comment 16•23 years ago
|
||
... ie skip the ChmodDataFile stuff, and just apply the rest.
Comment 17•23 years ago
|
||
Adding representatives of the packagers to bugs that are going into the
Bugzilla 2.14.2 security update
Comment 18•23 years ago
|
||
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+
Assignee | ||
Comment 19•23 years ago
|
||
moving secure bugzilla/webtools bugs from mozilla security group to the new
bugzilla security group.
Group: security? → webtools-security?
Updated•23 years ago
|
Attachment #83022 -
Flags: review+
Comment 20•23 years ago
|
||
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•23 years ago
|
||
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
Updated•23 years ago
|
Whiteboard: wanted for 2.14.2
Comment 22•23 years ago
|
||
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
Updated•23 years ago
|
Whiteboard: wanted for 2.14.2 → applied to 2.14.2
Updated•12 years ago
|
QA Contact: matty_is_a_geek → default-qa
You need to log in
before you can comment on or make changes to this bug.
Description
•