Last Comment Bug 134575 - defparams.pl::WriteParams makes data directory world writable
: defparams.pl::WriteParams makes data directory world writable
Status: RESOLVED FIXED
applied to 2.14.2
:
Product: Bugzilla
Classification: Server Software
Component: Bugzilla-General (show other bugs)
: 2.15
: x86 Linux
: -- critical (vote)
: Bugzilla 2.16
Assigned To: Bradley Baetz (:bbaetz)
: default-qa
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2002-03-31 19:21 PST by Bradley Baetz (:bbaetz)
Modified: 2012-12-18 20:46 PST (History)
13 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
v1 (3.55 KB, patch)
2002-04-03 17:46 PST, Bradley Baetz (:bbaetz)
justdave: review+
gerv: review+
Details | Diff | Splinter Review
Backported patch for BUGZILLA-2_14_1-BRANCH (3.51 KB, patch)
2002-05-10 03:02 PDT, J. Paul Reed [:preed]
justdave: review+
tara: review+
Details | Diff | Splinter Review

Description Bradley Baetz (:bbaetz) 2002-03-31 19:21:21 PST
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.
Comment 1 Bradley Baetz (:bbaetz) 2002-03-31 19:26:26 PST
-> 2.16, although if you have a better suggestion, feel free to take this.
Comment 2 Dave Miller [:justdave] (justdave@bugzilla.org) 2002-04-01 20:54:29 PST
CCing the usual suspects.
Comment 3 Gervase Markham [:gerv] 2002-04-02 11:03:12 PST
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
Comment 4 Bradley Baetz (:bbaetz) 2002-04-02 19:30:23 PST
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 Bradley Baetz (:bbaetz) 2002-04-03 17:46:39 PST
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 6 Dave Miller [:justdave] (justdave@bugzilla.org) 2002-04-03 18:24:15 PST
Comment on attachment 77567 [details] [diff] [review]
v1

r= justdave
Comment 7 Gervase Markham [:gerv] 2002-04-06 07:10:57 PST
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
Comment 8 Bradley Baetz (:bbaetz) 2002-04-06 20:02:56 PST
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.
Comment 9 J. Paul Reed [:preed] 2002-04-06 22:43:41 PST
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 Bradley Baetz (:bbaetz) 2002-04-06 23:09:44 PST
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 Gervase Markham [:gerv] 2002-04-06 23:30:45 PST
bbaetz: excellent, exactly what I wanted.

preed: there's no preference; but, in the future, that sort of thing should
ThrowCodeError(). 

Gerv
Comment 12 Bradley Baetz (:bbaetz) 2002-04-06 23:40:22 PST
gerv: ThrowCodeError is not appropriate for a script which is run from procmail
(ie importxml.pl)
Comment 13 Dave Miller [:justdave] (justdave@bugzilla.org) 2002-05-08 21:53:04 PDT
munging ccs
Comment 14 J. Paul Reed [:preed] 2002-05-10 03:02:12 PDT
Created attachment 83022 [details] [diff] [review]
Backported patch for BUGZILLA-2_14_1-BRANCH
Comment 15 Bradley Baetz (:bbaetz) 2002-05-10 04:54:15 PDT
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 Bradley Baetz (:bbaetz) 2002-05-10 04:57:07 PDT
... ie skip the ChmodDataFile stuff, and just apply the rest.
Comment 17 Dave Miller [:justdave] (justdave@bugzilla.org) 2002-05-12 09:12:13 PDT
Adding representatives of the packagers to bugs that are going into the
Bugzilla 2.14.2 security update
Comment 18 Dave Miller [:justdave] (justdave@bugzilla.org) 2002-05-13 15:13:44 PDT
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.
Comment 19 Bradley Baetz (:bbaetz) 2002-05-15 22:30:13 PDT
moving secure bugzilla/webtools bugs from mozilla security group to the new
bugzilla security group.
Comment 20 Myk Melez [:myk] [@mykmelez] 2002-05-23 11:44:38 PDT
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 Dave Miller [:justdave] (justdave@bugzilla.org) 2002-05-25 07:53:29 PDT
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
Comment 22 J. Paul Reed [:preed] 2002-05-25 13:53:17 PDT
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
Comment 23 Dave Miller [:justdave] (justdave@bugzilla.org) 2002-06-08 00:01:31 PDT
2.14.2 is out, removing security group.

Note You need to log in before you can comment on or make changes to this bug.