wrong file permissions after installation

RESOLVED FIXED

Status

SeaMonkey
Installer
--
critical
RESOLVED FIXED
14 years ago
6 years ago

People

(Reporter: Daniel Koukola, Assigned: dbaron)

Tracking

({fixed-aviary1.0, fixed1.7.3})

Trunk
x86
Linux
fixed-aviary1.0, fixed1.7.3
Bug Flags:
blocking1.7a +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:local exploit] respin mozilla1.6-linux?)

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

14 years ago
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

14 years ago
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?
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.
Flags: blocking1.7a?
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 3

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

Comment 4

14 years ago
*** Bug 232636 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 5

14 years ago
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.)
Assignee: general → leaf
(Assignee)

Updated

14 years ago
Flags: blocking1.7a? → blocking1.7a+

Updated

14 years ago
Whiteboard: respin mozilla1.6-linux?

Comment 6

14 years ago
Accepting.
Status: NEW → ASSIGNED

Comment 7

14 years ago
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?
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

14 years ago
(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

(Reporter)

Comment 10

14 years ago
The problem seems to be fixed in 1.7a.

Comment 11

14 years ago
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)
(Assignee)

Comment 12

14 years ago
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.
(Assignee)

Comment 13

14 years ago
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

14 years ago
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.
(Assignee)

Comment 15

14 years ago
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

14 years ago
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

14 years ago
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.
(Assignee)

Updated

14 years ago
Attachment #142291 - Flags: review+

Comment 18

14 years ago
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

14 years ago
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.
> 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

14 years ago
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

14 years ago
Created attachment 142369 [details] [diff] [review]
accepting good suggestion (chmod og-w instead of 755) in installer script

obsoleting previous stopgap
Attachment #142291 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #142369 - Flags: review+

Comment 23

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

Comment 24

14 years ago
(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.
(Assignee)

Comment 25

14 years ago
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

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

ah, right you are.

Updated

14 years ago
QA Contact: bugzilla → agracebush

Updated

13 years ago
Whiteboard: respin mozilla1.6-linux? → [local exploit] respin mozilla1.6-linux?
*** Bug 243662 has been marked as a duplicate of this bug. ***
Whiteboard: [local exploit] respin mozilla1.6-linux? → [sg:local exploit] respin mozilla1.6-linux?
(Assignee)

Comment 28

13 years ago
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.
*** Bug 249651 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 30

13 years ago
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.
Attachment #155043 - Attachment is obsolete: true
(Assignee)

Comment 31

13 years ago
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).
(Assignee)

Updated

13 years ago
Attachment #155047 - Attachment is obsolete: true
(Assignee)

Comment 32

13 years ago
Created attachment 155103 [details] [diff] [review]
patch

This is a working version of the original patch.
Attachment #142288 - Attachment is obsolete: true
Attachment #155077 - Attachment is obsolete: true
(Assignee)

Updated

13 years ago
Attachment #155103 - Flags: superreview?(dveditz)
Attachment #155103 - Flags: review?(ajschult)
Comment on attachment 155103 [details] [diff] [review]
patch

r/sr=dveditz
Attachment #155103 - Flags: superreview?(dveditz)
Attachment #155103 - Flags: superreview+
Attachment #155103 - Flags: review?(ajschult)
Attachment #155103 - Flags: review+
(Assignee)

Updated

13 years ago
Attachment #155103 - Flags: approval1.8a3?

Comment 34

13 years ago
Comment on attachment 155103 [details] [diff] [review]
patch

a=asa for 1.8a3
Attachment #155103 - Flags: approval1.8a3? → approval1.8a3+
(Assignee)

Comment 35

13 years ago
Fix checked in to trunk, 2004-08-16 17:12 -0700.
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
(Assignee)

Updated

13 years ago
Attachment #155103 - Flags: approval1.7.3?

Comment 36

13 years ago
Comment on attachment 155103 [details] [diff] [review]
patch

a=mkaply for 1.7
Attachment #155103 - Flags: approval1.7.3? → approval1.7.3+
(Assignee)

Comment 37

13 years ago
Fix checked in to MOZILLA_1_7_BRANCH, 2004-08-17 13:24 -0700.
Keywords: fixed1.7
(Assignee)

Updated

13 years ago
Attachment #155103 - Flags: approval-aviary?
(Assignee)

Updated

13 years ago
Keywords: fixed1.7 → fixed1.7.3
(Assignee)

Comment 38

13 years ago
Fix checked in to MOZILLA_1_7_2_BRANCH, 2004-08-27 13:35 -0700.
Whiteboard: [sg:local exploit] respin mozilla1.6-linux? → [sg:local exploit] respin mozilla1.6-linux? fixed1.7.2+
(Assignee)

Updated

13 years ago
Assignee: leaf → dbaron

Comment 39

13 years ago
Comment on attachment 155103 [details] [diff] [review]
patch

a=asa for aviary checkin
Attachment #155103 - Flags: approval-aviary? → approval-aviary+
(Assignee)

Comment 40

13 years ago
Fix checked in to AVIARY_1_0_20040515_BRANCH, 2004-08-27 14:39 -0700.
Keywords: fixed-aviary1.0
(Assignee)

Comment 41

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

Comment 42

13 years ago
We've got public dupes of this, why is it still marked confidential?
Group: security
Whiteboard: [sg:local exploit] respin mozilla1.6-linux? fixed1.7.2+ → [sg:local exploit] respin mozilla1.6-linux? fixed1.7.3
Product: Browser → Seamonkey

Comment 43

13 years ago
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.
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.
Keywords: fixed1.7.5 → fixed1.7.3
Whiteboard: [sg:local exploit] respin mozilla1.6-linux? fixed1.7.3 → [sg:local exploit] respin mozilla1.6-linux?
You need to log in before you can comment on or make changes to this bug.