Closed Bug 231083 Opened 16 years ago Closed 16 years ago
wrong file permissions after installation
770 bytes, patch
|Details | Diff | Splinter Review|
12.82 KB, patch
|Details | Diff | Splinter Review|
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.
16 years ago
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+
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.
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.
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?
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. ***
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
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
This is a working version of the original patch.
Comment on attachment 155103 [details] [diff] [review] patch r/sr=dveditz
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: 16 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.
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+
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.
*** Bug 258883 has been marked as a duplicate of this bug. ***
We've got public dupes of this, why is it still marked confidential?
Whiteboard: [sg:local exploit] respin mozilla1.6-linux? fixed1.7.2+ → [sg:local exploit] respin mozilla1.6-linux? fixed1.7.3
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.
You need to log in before you can comment on or make changes to this bug.