231083 - wrong file permissions after installation

Status: RESOLVED FIXED

(SeaMonkey :: Installer, defect)

Description

[Daniel Koukola]

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]

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 [:bzbarsky]]

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-4, no longer working on Mozilla)]

*** Bug 232636 has been marked as a duplicate of this bug. ***

Comment 4

[timeless]

*** Bug 232636 has been marked as a duplicate of this bug. ***

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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]

Accepting.

Comment 7

[Daniel (Leaf) Nunes]

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 [:bzbarsky]]

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]

(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]

The problem seems to be fixed in 1.7a.

Comment 11

[Daniel (Leaf) Nunes]

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-4, no longer working on Mozilla)]

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-4, no longer working on Mozilla)]

Attachment: 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]

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-4, no longer working on Mozilla)]

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]

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]

Attachment: 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]

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]

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

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

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]

Attachment: accepting good suggestion (chmod og-w instead of 755) in installer script

obsoleting previous stopgap

Comment 23

[Daniel (Leaf) Nunes]

stopgap checked in. should a new bug get filed for our zip implementation not
respecting the user's umask?

Comment 24

[Andrew Schultz]

(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-4, no longer working on Mozilla)]

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]

> What do you mean by appropriate?  tar uses the umask when writing files,

ah, right you are.

Comment 27

[Daniel Veditz [:dveditz]]

*** Bug 243662 has been marked as a duplicate of this bug. ***

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: this ought to fix it

This isn't the normal way of doing things, but it ought to work.

Comment 29

[Brendan Eich [:brendan]]

*** Bug 249651 has been marked as a duplicate of this bug. ***

Comment 30

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: 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-4, no longer working on Mozilla)]

Attachment: 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-4, no longer working on Mozilla)]

Attachment: patch

This is a working version of the original patch.

Comment 33

[Daniel Veditz [:dveditz]]

Comment on attachment 155103 [details] [diff] [review]
patch

r/sr=dveditz

Comment 34

[Asa Dotzler [:asa]]

Comment on attachment 155103 [details] [diff] [review]
patch

a=asa for 1.8a3

Comment 35

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fix checked in to trunk, 2004-08-16 17:12 -0700.

Comment 36

[Mike Kaply [:mkaply]]

Comment on attachment 155103 [details] [diff] [review]
patch

a=mkaply for 1.7

Comment 37

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fix checked in to MOZILLA_1_7_BRANCH, 2004-08-17 13:24 -0700.

Comment 38

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fix checked in to MOZILLA_1_7_2_BRANCH, 2004-08-27 13:35 -0700.

Comment 39

[Asa Dotzler [:asa]]

Comment on attachment 155103 [details] [diff] [review]
patch

a=asa for aviary checkin

Comment 40

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fix checked in to AVIARY_1_0_20040515_BRANCH, 2004-08-27 14:39 -0700.

Comment 41

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

*** Bug 258883 has been marked as a duplicate of this bug. ***

Comment 42

[Asa Dotzler [:asa]]

We've got public dupes of this, why is it still marked confidential?

Comment 43

[Thomas Hedden]

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 emeritus)]

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.