Closed Bug 231083 Opened 16 years ago Closed 15 years ago

wrong file permissions after installation

Categories

(SeaMonkey :: Installer, defect, critical)

x86
Linux
defect
Not set
critical

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: dan, Assigned: dbaron)

References

Details

(Keywords: fixed-aviary1.0, fixed1.7.3, Whiteboard: [sg:local exploit] respin mozilla1.6-linux?)

Attachments

(2 files, 5 obsolete files)

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.
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
*** Bug 232636 has been marked as a duplicate of this bug. ***
*** Bug 232636 has been marked as a duplicate of this bug. ***
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
Flags: blocking1.7a? → blocking1.7a+
Whiteboard: respin mozilla1.6-linux?
Accepting.
Status: NEW → ASSIGNED
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.
(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

The problem seems to be fixed in 1.7a.
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)
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.
Attached patch possible patch (obsolete) — Splinter Review
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.
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.
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 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.
I know this isn't the "right" solution, but it'd be nice if trunk builds didn't
have this vulnerability anymore.
Attachment #142291 - Flags: review+
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 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 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.
obsoleting previous stopgap
Attachment #142291 - Attachment is obsolete: true
Attachment #142369 - Flags: review+
stopgap checked in. should a new bug get filed for our zip implementation not
respecting the user's umask?
(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.
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.
> What do you mean by appropriate?  tar uses the umask when writing files,

ah, right you are.
QA Contact: bugzilla → agracebush
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?
Attached patch this ought to fix it (obsolete) — Splinter Review
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. ***
Attached patch this ought to fix it (obsolete) — Splinter Review
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
Attached patch patch (obsolete) — Splinter Review
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).
Attachment #155047 - Attachment is obsolete: true
Attached patch patchSplinter Review
This is a working version of the original patch.
Attachment #142288 - Attachment is obsolete: true
Attachment #155077 - Attachment is obsolete: true
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+
Attachment #155103 - Flags: approval1.8a3?
Comment on attachment 155103 [details] [diff] [review]
patch

a=asa for 1.8a3
Attachment #155103 - Flags: approval1.8a3? → approval1.8a3+
Fix checked in to trunk, 2004-08-16 17:12 -0700.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Attachment #155103 - Flags: approval1.7.3?
Comment on attachment 155103 [details] [diff] [review]
patch

a=mkaply for 1.7
Attachment #155103 - Flags: approval1.7.3? → approval1.7.3+
Fix checked in to MOZILLA_1_7_BRANCH, 2004-08-17 13:24 -0700.
Keywords: fixed1.7
Attachment #155103 - Flags: approval-aviary?
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: leaf → dbaron
Comment on attachment 155103 [details] [diff] [review]
patch

a=asa for aviary checkin
Attachment #155103 - Flags: approval-aviary? → approval-aviary+
Fix checked in to AVIARY_1_0_20040515_BRANCH, 2004-08-27 14:39 -0700.
Keywords: fixed-aviary1.0
*** Bug 258883 has been marked as a duplicate of this bug. ***
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
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.5fixed1.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.