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 User image 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 User image 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 User image Dave Miller [:justdave] (justdave@bugzilla.org) 2002-04-01 20:54:29 PST
CCing the usual suspects.
Comment 3 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image Dave Miller [:justdave] (justdave@bugzilla.org) 2002-05-08 21:53:04 PDT
munging ccs
Comment 14 User image 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 User image 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 User image Bradley Baetz (:bbaetz) 2002-05-10 04:57:07 PDT
... ie skip the ChmodDataFile stuff, and just apply the rest.
Comment 17 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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.