Last Comment Bug 231083 - wrong file permissions after installation
: wrong file permissions after installation
Status: RESOLVED FIXED
[sg:local exploit] respin mozilla1.6-...
: fixed-aviary1.0, fixed1.7.3
Product: SeaMonkey
Classification: Client Software
Component: Installer (show other bugs)
: Trunk
: x86 Linux
: -- critical (vote)
: ---
Assigned To: David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
: Grace Bush
Mentors:
: 232636 243662 249651 258883 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2004-01-16 03:25 PST by Daniel Koukola
Modified: 2011-08-05 22:35 PDT (History)
16 users (show)
dbaron: blocking1.7a+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
possible patch (5.35 KB, patch)
2004-02-25 16:40 PST, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details | Diff | Splinter Review
stopgap to ensure that the permissions in the .xpi files are sane pre-extraction (682 bytes, patch)
2004-02-25 17:40 PST, Daniel (Leaf) Nunes
dbaron: review+
Details | Diff | Splinter Review
accepting good suggestion (chmod og-w instead of 755) in installer script (770 bytes, patch)
2004-02-26 14:17 PST, Daniel (Leaf) Nunes
dbaron: review+
Details | Diff | Splinter Review
this ought to fix it (5.77 KB, patch)
2004-08-02 17:02 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details | Diff | Splinter Review
this ought to fix it (7.84 KB, patch)
2004-08-02 17:46 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details | Diff | Splinter Review
patch (7.85 KB, patch)
2004-08-03 02:23 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details | Diff | Splinter Review
patch (12.82 KB, patch)
2004-08-03 12:19 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
dveditz: review+
dveditz: superreview+
asa: approval‑aviary+
mozilla: approval1.7.5+
asa: approval1.8a3+
Details | Diff | Splinter Review

Description Daniel Koukola 2004-01-16 03:25:10 PST
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.6) Gecko/20040113
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.6) Gecko/20040113

After installation, many installed files are group and world writable, even
though the installer was run with the umask value 0022.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.
Comment 1 Met - Martin Hassman 2004-02-03 01:55:36 PST
The problem is with the Mozilla1.6 *.xpi files for Linux - e.g. in
http://ftp.mozilla.org/pub/mozilla.org/mozilla/releases/mozilla1.6/linux-xpi/
the files were packed with wrong permissions (e.g. world writable 666).

The
http://ftp.mozilla.org/pub/mozilla.org/mozilla/releases/mozilla1.5/linux-xpi/ is
OK. But The same problem is in some nightly builds - in the
http://ftp.mozilla.org/pub/mozilla.org/mozilla/nightly/ seems be in all nightly
builds even in the oldest available build -
http://ftp.mozilla.org/pub/mozilla.org/mozilla/nightly/2003-12-01-12-trunk/linux-xpi/

Problem in the some packaging script?
Comment 2 Boris Zbarsky [:bz] 2004-02-03 08:29:27 PST
Confirming.  I just unzipped psm.xpi from the Mozilla 1.6 dir and got a nice
world-writable libnss and libssl.

We need to fix this ASAP; this is a trojan waiting to happen.
Comment 3 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2004-02-03 21:51:25 PST
*** Bug 232636 has been marked as a duplicate of this bug. ***
Comment 4 timeless 2004-02-03 21:51:43 PST
*** Bug 232636 has been marked as a duplicate of this bug. ***
Comment 5 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2004-02-03 21:58:54 PST
This may well be because the default cltbld .cshrc that's on many of the
tinderbox machines (which are now being used for nightlies) has umask 0 in it.

(We probably need to get a common .cshrc set up in cvs somewhere for tinderobxes.)
Comment 6 Daniel (Leaf) Nunes 2004-02-08 20:08:51 PST
Accepting.
Comment 7 Daniel (Leaf) Nunes 2004-02-15 18:49:03 PST
1.7a builds will produced exactly the same way the current trunk builds are
produced, so if current trunk builds install with correct permissions, 1.7a will
also have correct permissions. Can a reporter test a trunk build and report back
here?
Comment 8 Boris Zbarsky [:bz] 2004-02-15 19:11:28 PST
I just downloaded
http://ftp.mozilla.org/pub/mozilla.org/mozilla/nightly/latest/mozilla-i686-pc-linux-gnu-full-installer.tar.gz
(which has today's timestamp).  I untarred it, then unzipped the resulting
browser.xpi.  Here're the permissions on some randomly chosen files that resulted:

-rwxrwxrwx  1 bzbarsky bzbarsky     3797 Feb 15 11:24 mozilla
-rwxrwxrwx  1 bzbarsky bzbarsky   276072 Feb 15 11:24 mozilla-bin
-rwxrwxrwx  1 bzbarsky bzbarsky  4936564 Feb 15 11:24 components/libgklayout.so

So this is definitely still a problem.
Comment 9 Met - Martin Hassman 2004-02-15 23:13:14 PST
(In reply to comment #7)
> Can a reporter test a trunk build and report back
> here?

If it can help - here is a simple Python script, which shows you Linux
permission in the ZIP file (so you can test the Linux permissions even under
Windows - I am testing the *.xpi packages in the czech localized add-ons in this
way).

http://biomikro.vscht.cz/maldiman/hassmanm/projects/testzip.py.txt

Rename to testzip.py and run e.g.: python testzip.py your.xpi

Comment 10 Daniel Koukola 2004-02-24 14:57:11 PST
The problem seems to be fixed in 1.7a.
Comment 11 Daniel (Leaf) Nunes 2004-02-24 17:11:51 PST
I fixed the 1.7a release by hand, still need to fix the umask on the build
machine. Working on that today.

(or, better yet, i'll write a patch for the installer to bulletproof permissions)
Comment 12 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2004-02-25 16:16:08 PST
I think the problem lies here:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/modules/libjar/nsZipArchive.cpp&rev=1.76&mark=644,664#641

If we pass the right permissions to PR_Open instead of doing a chmod, things
ought to work.
Comment 13 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2004-02-25 16:40:03 PST
Created attachment 142288 [details] [diff] [review]
possible patch

I think this patch should do what we want (pass the right mode to PR_Open so
the umask is respected).  I need to figure out (a) how to test that it works
and (b) what else needs to be tested.
Comment 14 Daniel (Leaf) Nunes 2004-02-25 16:45:17 PST
I can make a test build with this patch (and intentionally make the local
permissions on the files in the zip archives be "bad") and create installers to
test.
Comment 15 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2004-02-25 17:25:23 PST
One potential problem with this patch is that if the file being created already
exists, this won't change the permissions.  That seems like it could be a problem.
Comment 16 Daniel (Leaf) Nunes 2004-02-25 17:35:21 PST
Comment on attachment 142288 [details] [diff] [review]
possible patch


Didn't seem to work; made clean and rebuilt modules/libjar and xpinstall, then
rebuilt installers.

Dave, can i check in my stupid hack in the while we track down the right way to
make zip work? (like tar).

I'll post it in a separate attachment.
Comment 17 Daniel (Leaf) Nunes 2004-02-25 17:40:14 PST
Created attachment 142291 [details] [diff] [review]
stopgap to ensure that the permissions in the .xpi files are sane pre-extraction

I know this isn't the "right" solution, but it'd be nice if trunk builds didn't
have this vulnerability anymore.
Comment 18 Met - Martin Hassman 2004-02-25 22:51:04 PST
Sorry for off-topic, but if you are making changes in the code of permission of
extracted files, maybe you should also consider bug #189905 (problem with Unix
permission when the XPI file was packed under DOS/Win) - this does not really
affects Mozilla installation itself, but it is problem of many addons from
mozdev.org.
Comment 19 Andrew Schultz 2004-02-26 01:46:18 PST
Comment on attachment 142288 [details] [diff] [review]
possible patch

>+  PRFileDesc* fOut = PR_Open(aOutname, ZFILE_CREATE, item->mode);

the installer only calls this via standalone libjar, where PR_Open is defined
as fopen in zipstub.h, so the perms get ignored.  This might be why the chmod
was there instead of passing the mode to PR_Open (in addition to the case of
overwriting files).

>-  rv = localFile->OpenNSPRFileDesc(PR_RDWR | PR_CREATE_FILE, 0664, &fd);
>+  rv = localFile->OpenNSPRFileDesc(PR_WRONLY | PR_CREATE_FILE, item->mode, &fd);

item->mode here does get used, but it didn't help because mode is 0777. 
(item->mode & 0755) would work.

there's also a chmod later in this function that got left.

but perhaps 
chmod(file, item->mode & 0755);
would work better considering the case of overwriting files.
Comment 20 Brendan Eich [:brendan] 2004-02-26 10:14:59 PST
> the installer only calls this via standalone libjar, where PR_Open is defined
> as fopen in zipstub.h, so the perms get ignored.

D'oh -- faulty emulation is always a hazard.  Can someone patch zipstub.h to do
the right thing?

/be
Comment 21 Andrew Schultz 2004-02-26 10:44:22 PST
Comment on attachment 142291 [details] [diff] [review]
stopgap to ensure that the permissions in the .xpi files are sane pre-extraction

>+system("chmod -R 755 $STAGE");

or chmod og-w -R $STAGE
755 makes all files (like bookmarks.html and all.js) executable.
Comment 22 Daniel (Leaf) Nunes 2004-02-26 14:17:08 PST
Created attachment 142369 [details] [diff] [review]
accepting good suggestion (chmod og-w instead of 755) in installer script

obsoleting previous stopgap
Comment 23 Daniel (Leaf) Nunes 2004-02-26 15:43:54 PST
stopgap checked in. should a new bug get filed for our zip implementation not
respecting the user's umask?
Comment 24 Andrew Schultz 2004-02-26 19:58:07 PST
(In reply to comment #23)
> stopgap checked in. should a new bug get filed for our zip implementation not
> respecting the user's umask?

I filed bug 235781 for xpinstall issues.  But the files still need to be
packaged with appropriate permissions for the non-installer tarballs, if nothing
else.
Comment 25 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2004-02-26 22:21:10 PST
What do you mean by appropriate?  tar uses the umask when writing files, so
appropriate permissions for a tarball should be 0777 or 0666, I'd think.
Comment 26 Andrew Schultz 2004-02-26 22:32:53 PST
> What do you mean by appropriate?  tar uses the umask when writing files,

ah, right you are.
Comment 27 Daniel Veditz [:dveditz] 2004-06-07 08:26:45 PDT
*** Bug 243662 has been marked as a duplicate of this bug. ***
Comment 28 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2004-08-02 17:02:29 PDT
Created attachment 155043 [details] [diff] [review]
this ought to fix it

This isn't the normal way of doing things, but it ought to work.
Comment 29 Brendan Eich [:brendan] 2004-08-02 17:21:32 PDT
*** Bug 249651 has been marked as a duplicate of this bug. ***
Comment 30 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2004-08-02 17:46:24 PDT
Created attachment 155047 [details] [diff] [review]
this ought to fix it

Same thing, but with old workaround removed and better comments.  Note that
this also fixes potential permission problems in the middle of installation of
files with g-r or o-r.
Comment 31 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2004-08-03 02:23:08 PDT
Created attachment 155077 [details] [diff] [review]
patch

This actually does fix it, but I've decided I like parts of ajschultz's
approach on bug 235781 better (some of which was originally from my earlier
patch).
Comment 32 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2004-08-03 12:19:51 PDT
Created attachment 155103 [details] [diff] [review]
patch

This is a working version of the original patch.
Comment 33 Daniel Veditz [:dveditz] 2004-08-10 14:47:04 PDT
Comment on attachment 155103 [details] [diff] [review]
patch

r/sr=dveditz
Comment 34 Asa Dotzler [:asa] 2004-08-16 14:37:31 PDT
Comment on attachment 155103 [details] [diff] [review]
patch

a=asa for 1.8a3
Comment 35 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2004-08-16 18:43:52 PDT
Fix checked in to trunk, 2004-08-16 17:12 -0700.
Comment 36 Mike Kaply [:mkaply] 2004-08-17 05:27:06 PDT
Comment on attachment 155103 [details] [diff] [review]
patch

a=mkaply for 1.7
Comment 37 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2004-08-17 13:27:11 PDT
Fix checked in to MOZILLA_1_7_BRANCH, 2004-08-17 13:24 -0700.
Comment 38 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2004-08-27 13:36:32 PDT
Fix checked in to MOZILLA_1_7_2_BRANCH, 2004-08-27 13:35 -0700.
Comment 39 Asa Dotzler [:asa] 2004-08-27 14:31:28 PDT
Comment on attachment 155103 [details] [diff] [review]
patch

a=asa for aviary checkin
Comment 40 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2004-08-27 17:28:04 PDT
Fix checked in to AVIARY_1_0_20040515_BRANCH, 2004-08-27 14:39 -0700.
Comment 41 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2004-09-13 08:58:58 PDT
*** Bug 258883 has been marked as a duplicate of this bug. ***
Comment 42 Asa Dotzler [:asa] 2004-09-13 09:31:09 PDT
We've got public dupes of this, why is it still marked confidential?
Comment 43 Thomas Hedden 2004-12-10 12:49:44 PST
There's been a lot of discussion about this matter in
this bug and also in duplicates. Perhaps this suggestion
is redundant, and if so please excuse me.
I think it would be a good idea for the Firefox installer
script to ask the user at install time whether the
installation is intended for just the user who launched
the install script or for other users as well. It could
also check the value of umask and/or warn the user if
it's not right or possibly reset it for the install.
Although it's true that permission errors can be fixed
by recursive chmod commands, if Firefox is intended for
a broad audience there's going to be a lot of frustration
among non-sysadmin types if it gets installed with the
wrong permissions. This would be very easy to do.
Comment 44 Mike Shaver (:shaver -- probably not reading bugmail closely) 2004-12-11 09:42:19 PST
Thomas: That's an interesting suggestion.  If you were to file another bug and
stick an example patch in it for feedback and testing, it might get more
attention, though.

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