Closed Bug 229636 Opened 21 years ago Closed 21 years ago

search for helper apps in mozilla directory before $PATH

Categories

(Core Graveyard :: File Handling, defect)

Product:
Core Graveyard
Component:
File Handling
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla1.7beta

People

(Reporter: Biesinger, Assigned: Biesinger)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

final patch
14.31 KB, patch
Details | Diff | Splinter Review
mozilla should search for helper apps in the program directory, if the path is not absolute. reason: people who ship a mozilla(-based) browser on cd (as viewer for some html on that cd) sometimes want to specify a specific helper app (also on the cd) for some file types. currently, they can't really, because helper apps are searched for only in $PATH.
On unix it's generally considered a security risk to alter the user's PATH withouth her consent. It's most definitely a security risk to alter a privileged account's PATH or even have the current directory in the PATH. Please don't even think of doing this on unix. Even my Windows XP home edition can have admisistrative and normal accounts so I tend to think the unix issues would also apply. This seems like an idea that will cause more trouble than it's worth.
If someone can write to the mozilla execution path, you're already in trouble, since you're running a binary they could write to. Please explain to me exactly how doing this will lead to any trouble that's not already present? Alternately, please explain how else the goal (launching an app that comes with Mozilla when you don't know where Mozilla was installed) can be accomplished, if not by using a relative path and trying it relative to the install dir.
> If someone can write to the mozilla execution path, you're already in > trouble, since you're running a binary they could write to. It's not about writing to a specific object, it's about finding objects using specific search rules. > Please explain to me exactly how doing this will lead to any trouble > that's not already present? Are you saying that knowing an insecurity exists allows you to exploit it in new code? I hope not. Now consider this: application Z is found to contain a security hole. An update is issues and the user dutifully installs it. Unfortunately, the mozilla cd contains an older version. Under the proposal here mozilla would use the insecure version. A very bad idea. > Alternately, please explain how else the goal ... I see a distinction between mozilla components (using a broad definition) and bundled third-party apps. I would treat bundled apps as toxic waste and not run them without explicit user permission. I can see mozilla trying the system search rules and, if and only if they fail, asking the user if it may use its own version. Something like: "I've found a helper app squirreled away in my attic. May I use it.? You just can't presume an app can be used just because you happen to have a copy. There can be very good reasons why a particular app does not exist.
> Are you saying that knowing an insecurity exists allows you to exploit it > in new code? I hope not. No, I am saying that I don't see the insecurity in the new code. You're drawing a line between trusting bundled third party apps and trusting the third-party Mozilla itself (since this is clearly not a mozilla.org build or a build installed with the OS by the OS distributor). There is no such line. If you're trusting the crap on that CD you put into the machine, you are trusting it. The version of Mozilla on the CD can be old and have security holes just like apps on the CD could be. There is no difference. There are probably good reasons Mozilla is not on the machine either, forcing you to run it off the CD. Sorry, but your arguments so far are specious. If you can explain how running apps from the mozilla dir is different from running XPCOM components from there or Mozilla itself, I'll listen. So far you haven't deigned to do so, and I see no difference.
> Now consider this: application Z is found to contain a security hole. err, how many applications (executables) do you expect exist in a mozilla program directory? I have exactly 4 here: mozilla.exe PalmSyncInstall.exe regxpcom.exe xpicleanup.exe that makes 3 on linux (which you are mostly worried about), one of them being mozilla itself. Are you aware that the only way to make use of this feature is to edit mimeTypes.rdf and make it readonly (because otherwise mozilla makes the path absolute)?
> err, how many applications (executables) do you expect exist in a mozilla > program directory? There's nothing preventing the author of the supposed cdrom from putting anything he want in that directory. It's actually a logical place given the cdrom. My point is that, if mozilla is going to look in its own bin directory, it should have knowledge of what its own components are and just load them. > Are you aware that the only way to make use of this feature is to edit > mimeTypes.rdf and make it readonly So what. This is something that users can do, either intentionally or not. It's also something that other programs can do. You are proposing changing a search rule based on what many would consider a normal operation. That is just risky as far as I'm concerned. bz: of course running a cdrom is risky but so is running anything. It about minimizing risk not eliminating it. I see no reason to make it easy to bypass the normal system search rules, particularly just to fix mozilla's problems with read-only files. Let me try this again. There are XPCOM components, there are GRE components, there are executables, there are plugins, there are any number of other things. They can be scattered across cdroms, disks, whatever. There is probably no way to plan for all possible combinations but it is possible to plan for the common, and some of the uncommon, cases. I am arguing that items on cdroms, or any removable media, are less trustworthy than those on the local system just because they're removable. This is particularly true for a non-essential package like mozilla. Sure, removable media are very useful for restoring systems but that's not the issue here. Oh yes, the bug summary specifically mentions helper apps not mozilla components. I think they are even less trustworthy than mozilla components. I am arguing that it's far better to trust the system than a cdrom. BTW, wouldn't it be better in the long run to fix mozilla's problem with read-only files? I really don't see why runtime-behavior depends on the ability to write to a file.
Problem: People creating CD ROMs using Mozilla who need a proprietary plug-in for some of the content currently can't do it without writing to the disk, because there is no way to tell Mozilla to look on the CD for plugins. Proposed solution: Let Mozilla check it's own directory for plugins. In response to this, tenthumbs wrote: > There's nothing preventing the author of the supposed cdrom from putting > anything he want in that directory. It's actually a logical place given the > cdrom. My point is that, if mozilla is going to look in its own bin > directory, it should have knowledge of what its own components are and just > load them. We're not talking about components, we're talking about plugins. >> the only way to make use of this feature is to edit mimeTypes.rdf and >> make it readonly > > This is something that users can do, either intentionally or not. Not if the file is on the CD-ROM. > It's also something that other programs can do. Again, not if the file is on the CD-ROM. > of course running a cdrom is risky but so is running anything. It about > minimizing risk not eliminating it. If you trust the CD, why would trust one of the executables on there any more or less than any other? There is no reduction in risk here. Once you are running programs from the CD, you are at the mercy of the CD. > I see no reason to make it easy to bypass the normal system search rules People creating CD ROMs using Mozilla who need a proprietary plug-in for some of the content currently can't do it without writing to the disk, because there is no way to tell Mozilla to look on the CD for plugins. > particularly just to fix mozilla's problems with read-only files. I'm not sure what you are referring to here. > I am arguing that items on cdroms, or any removable media, are less > trustworthy than those on the local system just because they're removable. We're not arguing that point. We're arguing that once you've agreed to run this code, running more code from the same source makes no difference to the trust level. > I am arguing that it's far better to trust the system than a cdrom. The system may not have the plug-in at _all_. That's the problem. > BTW, wouldn't it be better in the long run to fix mozilla's problem with > read-only files? I really don't see why runtime-behavior depends on the > ability to write to a file. Again, I'm not sure how this is relevant here. Maybe I am missing something.
Comment 6 pretty clearly shows that we are talking about totally different things here (as comment 7 points out). Read-onlyness has nothing whatsoever to do with this.
OK, let's assume a developer need to find a helper app from his plugin. He can 1) search for it himself without mozilla's help, 2) search for it using the system's search rules, or 3) expect mozilla to help him. Case 1 isn't important here. In case 2 having mozilla alter the system's search rules is unexpected and possible a big problem. This is one of the places I consider the proposal here a security issue. So let's now assume that mozilla is helping case 3. Is it possible that this mozilla-on-a-disk might find a pre-existing user profile with possibly plugins of its own and will this mozilla handle this profile the same way as a normal mozilla will. If so then there is the possibility that the profile plugins will load. If they are _not_ expecting to operate in the case 3 environment then I claim this is another security issue. I have a lot of trouble with the idea that mozilla should trust itself just because it's mozilla. Obviously anyone can hack mozilla if they want to, given the source code. I see no reason why mozilla should allow even the most remote security hole. Make these people do it themselves. At the very least you could probably force these people to not use the mozilla name. Now, adding code will make mozilla larger and this proposal is about a very limited issue so I think any code should be wrapped in a configure option. If you're compiling a plugin you can build your own mozilla too. On a unix-like system you don't need to change any code at all. You just need to change the startup scripts. If you start with an executable then you should do it there not in a shared library. After all a shared library is expected to be used by more than one application. I think you're doing this in the wrong component. Personally, I think all this sort of stuff should be done before mozilla itself starts. As for read-only files, I just think that any instance of an application should be able to run if it can read its config files and any state files from a previous instance. Whether an instance can write should only affect subsequent instances not the current one.
When did plugins enter the picture? THIS BUG IS NOT ABOUT PLUGINS. Further, the read-only issue is completely unrelated to this bug. Note that this is filed as a Windows bug, not a Unix one; I agree that on Unix adjusting the startup scripts to set $PATH would be a completely equivalent solution; in that case, how is it preferable to a solution that works XP?
One common way to start a helper app is with a plugin so I say plugins are very relevant to this bug. I claim it's important that _all_ methods that can execute helper apps behave as expected. To be honest I couldn't care less what mozilla does on Windows but any xp method affects Linux/unix. Unix history has many stories about unexpected behavior if a search path is changed. There's no need for mozilla to repeat past mistakes by inserting code into mozilla. The very small percentage of mozilla users who think they need this can hack the code or scripts or whatever. Even on Windows I see no reason why these poeple couldn't have a mozilla.bat instead of mozilla.exe.
on windows mozilla does not search $PATH for helper apps. does that answer your question?
You didn't answer the salient question from comment 10, tenthumbs.
> on windows mozilla does not search $PATH for helper apps. does that answer > your question? It raises more. This bug is supposedly about Windows so why is $PATH mentioned in the summary? If this bug is cross-platform then why have people claimed it's only about Windows? bz: I think I've addressed all the issues. You'll have to give me some idea as to what I was unclear on.
What you were unclear on is why it's better from a security point of view if someone shipping Mozilla has to put a binary in the program dir _and_ change the startup script to add the program dir to $PATH (as opposed to the current proposal, where the would just put a binary in the program dir). The end result for the user is completely identical; all it is is a slight hassle for the distributor; not enough hassle to not do it, but enough to be annoyed. Also note that no one has claimed that this bug is "about Windows". Just that it's not "only about Unix", and that "Windows needs a solution, and there is absolutely no reason for Unix not to use that solution". Biesi, I think you should just do this.
OS: Windows 2000 → All
Hardware: PC → All
Attached patch patch (obsolete) — — Splinter Review
I didn't change mac and beos... the "nice" solution doesn't seem to fit in their structure, and I didn't feel it was worth the effort to implement this in another way. can do that though, if you want me to. there was some reindenting in unix/nsOSHelperAppService.cpp, but it should be readable in this patch. tell me if you want a -w version though.
Attachment #141429 - Flags: review?(bzbarsky)
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.7beta
I think you both are presuming that distributors/embedders will always consider mozilla's interests. My experience is that they don't really care so I wouldn't let them do anything that could damage my product's reputation. If you want to allow this I can't stop you but I think it's a terrible mistake.
why would embeddors damage mozilla's reputation? it's their own product, not mozilla's.
And frankly, they can already do it in so many ways (disabling security settings, adding spyware addons, etc, etc)...
Is NS_OS_CURRENT_PROCESS_DIR really the right thing? That's the install dir, not the dir we got started from? I'm not sure when I'll be able to get to the rest of the review.
bz: huh? from nsDirectoryServiceDefs.h: 61 /* Property returns the directory in which the procces was started from. 62 * On Unix this will be the path in the MOZILLA_FIVE_HOME env var and if 63 * unset will be the current working directory. 64 */ 65 #define NS_OS_CURRENT_PROCESS_DIR "CurProcD" so this is indeed the dir in which the process was started. it's also what http://lxr.mozilla.org/seamonkey/source/uriloader/exthandler/unix/nsOSHelperAppService.cpp#1242 uses. bah. that code should be removed, now that GetFileTokenForPath does this. I'll make a new patch sometime in the next few days.
Um.. we do NOT want to be getting helpers from the dir we started from, do we? That has all the issues that tenthumbs brought up, indeed. We want to be using the install dir.
oh, that's what you meant yeah, correct. but I believe the documentation is wrong :) especially if you consider that it uses MOZILLA_FIVE_HOME on linux. also, the implementation always gets the path of the executable, i.e. the directory that we really want.
I would rather not rely on the fact that no one will fix the code to follow the documentation.... Is there no way to get the actual program dir from the directory service?
bsmedberg: any idea on comment 24?
The value NS_OS_CURRENT_PROCESS_DIR is the OS "current directory". The value NS_XPCOM_CURRENT_PROCESS_DIR is the directory of the mozilla executable. This is almost always what people actually want.
filed Bug 234515 to improve the comment about NS_OS_CURRENT_PROCESS_DIR, and will fix the patch.
Comment on attachment 141429 [details] [diff] [review] patch >Index: nsExternalHelperAppService.cpp >+nsresult nsExternalHelperAppService::GetFileTokenForPath(const PRUnichar * platformAppPath, >+ nsresult rv = NS_GetSpecialDirectory(NS_OS_CURRENT_PROCESS_DIR, aFile); >+ rv = localFile->InitWithPath(nsDependentString(platformAppPath)); You want to do these in the opposite order (in case the path is a full path). >Index: unix/nsOSHelperAppService.cpp >+ // XXX provided the string code has all it's bugs fixed, we should be able to >+ // remove this PromiseFlatCString call. >+ localFile->InitWithNativePath(PromiseFlatCString(Substring(start_iter, colon_iter))); Since darin's landing, should be safe to remove the comment and the PromiseFlatCString call. With those and the directory id issue I pointed out earlier, r=bzbarsky.
Attachment #141429 - Flags: review?(bzbarsky) → review+
> why would embeddors damage mozilla's reputation? it's their own product, > not mozilla's. Customers are fickle and have their own agendas. They may like you today but may jettison you in a picosecond tomorrow if it suits their purposes, particularly if they can blame you for their mistakes. I hope you all know what you're doing. The last patch is cross-platform at all even though there are arguments here that this idea is. Why?
> The last patch is cross-platform at all You meant "isn't"? That patch makes identical changes to OS/2, Windows, and Unix. I too would be interested in an explanation of why MacOS and BeOS don't need this change, though.
Yes, "isn't." Brain cramp. I still think this is a useless Windows-only hack that should be forgotten but if you're going to do it at all you should do it everywhere.
Ah, right... platform-specific api crap.... Yeah, fixing those to do this would be rather hard.
(In reply to comment #29) > Since darin's landing, should be safe to remove the comment and the > PromiseFlatCString call. ok it took me some while until I realized that I had done this already as part of bug 78919. which now forced me to apply this patch manually in parts. oh well.
Attached patch patch v2 (obsolete) — — Splinter Review
Attachment #141429 - Attachment is obsolete: true
Attachment #142381 - Flags: review?(bzbarsky)
Comment on attachment 142381 [details] [diff] [review] patch v2 r=bzbarsky, but please file a followup bug for beos and mac.
Attachment #142381 - Flags: review?(bzbarsky) → review+
Comment on attachment 142381 [details] [diff] [review] patch v2 bz: ok, will do that once I check this in
Attachment #142381 - Flags: superreview?(darin)
Comment on attachment 142381 [details] [diff] [review] patch v2 >Index: nsExternalHelperAppService.cpp >+nsresult nsExternalHelperAppService::GetFileTokenForPath(const PRUnichar * platformAppPath, >+ nsIFile ** aFile) this function looks like its trying hard to avoid nsCOMPtr. i understand why. how about this instead: { nsDependentString platformAppPath(aPlatformAppPath); // First, check if we have an absolute path nsIFile* file; nsresult rv = NS_NewLocalFile(platformAppPath, PR_TRUE, (nsILocalFile **) &file); if (NS_SUCCEEDED(rv)) { PRBool exists; if (NS_FAILED(file->Exists(&exists)) || !exists) { NS_RELEASE(file); return NS_ERROR_FILE_NOT_FOUND; } *aFile = file; return NS_OK; } // Second, check if file exists in mozilla program directory rv = NS_GetSpecialDirectory(NS_XPCOM_CURRENT_PROCESS_DIR, &file); if (NS_SUCCEEDED(rv)) { rv = file->Append(platformAppPath); if (NS_SUCCEEDED(rv)) { PRBool exists; rv = file->Exists(&exists); if (NS_SUCCEEDED(rv) && exists) { *aFile = file; return NS_OK; } } NS_RELEASE(file); } return NS_ERROR_FILE_NOT_FOUND; } >Index: nsExternalHelperAppService.h >+ * The base class implementation will first try to interpret platformAppPath >+ * as an absolute path, and if that fails look for a file next to the mozilla ... and if that fails [it will] look for a file next to ... >Index: os2/nsOSHelperAppService.cpp hmm... #ifndef XP_OS2 code in OS/2 only code. that's odd. i assume that's why you are okay removing it. want to make sure there isn't a reason for its existance? or maybe you've already checked with one of the OS/2 guys? >Index: unix/nsOSHelperAppService.cpp > nsCOMPtr<nsILocalFile> localFile (do_CreateInstance(NS_LOCAL_FILE_CONTRACTID)); > > if (!localFile) return NS_ERROR_NOT_INITIALIZED; >+ > PRBool exists = PR_FALSE; >+ // ugly hack. Walk the PATH variable... >+ char* unixpath = PR_GetEnv("PATH"); >+ nsCAutoString path(unixpath); >+ nsACString::const_iterator start_iter, end_iter, colon_iter; > >+ path.BeginReading(start_iter); >+ colon_iter = start_iter; >+ path.EndReading(end_iter); while you're at it, you might as well just use |const char *| for the iterator instead of nsReadingIterator. like this: nsCAutoString path(unixpath); const char *start_iter, *end_iter, *colon_iter; start_iter = path.BeginReading(); colon_iter = start_iter; end_iter = path.EndReading(); that's slightly less code in the end. sr=darin with these changes.
Attachment #142381 - Flags: superreview?(darin) → superreview+
(In reply to comment #39) > nsIFile* file; > nsresult rv = NS_NewLocalFile(platformAppPath, PR_TRUE, (nsILocalFile **) > &file); I have to say that I dislike this cast. (well, casts in general, really ;) ) I much prefer the way I did this... I did use your suggestion of moving the nsDependentString to the beginning of the function to not create it twice. > hmm... #ifndef XP_OS2 code in OS/2 only code. that's odd. i assume that's why > you are okay removing it. want to make sure there isn't a reason for its > existance? or maybe you've already checked with one of the OS/2 guys? I suspect this is just left over from the unix nsOSHelperAppService from which the os/2 file was forked... mkaply? > start_iter = path.BeginReading(); oh cool, the BeginReading signature is finally sane?
(In reply to comment #40) > oh cool, the BeginReading signature is finally sane? Doesn't seem to be, looks like I still need: const char* start_iter = path.BeginReading(start_iter);
Attached patch final patch — — Splinter Review
Attachment #142381 - Attachment is obsolete: true
biesi is correct - it was part of my unix port - it can be removed.
thanks, "final patch" checked in. filed bug 236844 and bug 236845 for beos and mac
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
this likely caused bug 237947
Blocks: 237947
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: