defparams.pl::WriteParams makes data directory world writable

RESOLVED FIXED in Bugzilla 2.16

Status

()

Bugzilla
Bugzilla-General
--
critical
RESOLVED FIXED
16 years ago
5 years ago

People

(Reporter: bbaetz, Assigned: bbaetz)

Tracking

2.15
Bugzilla 2.16
x86
Linux

Details

(Whiteboard: applied to 2.14.2)

Attachments

(2 attachments)

(Assignee)

Description

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

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

Gerv
(Assignee)

Comment 4

15 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

15 years ago
Created attachment 77567 [details] [diff] [review]
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 on attachment 77567 [details] [diff] [review]
v1

r= justdave
Attachment #77567 - Flags: review+
Keywords: patch, review
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

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

Comment 9

15 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

15 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
ThrowCodeError(). 

Gerv
(Assignee)

Comment 12

15 years ago
gerv: ThrowCodeError is not appropriate for a script which is run from procmail
(ie importxml.pl)
munging ccs
Created attachment 83022 [details] [diff] [review]
Backported patch for BUGZILLA-2_14_1-BRANCH
(Assignee)

Comment 15

15 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

15 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+
(Assignee)

Comment 19

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

Updated

15 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 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
Whiteboard: wanted for 2.14.2
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
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.