If I have the wrong component or owner, feel free to change it. There are a few issues in the way nsOSHelperAppService::GetFileTokenForPath in uriloader/exthandler/unix/nsOSHelperAppService.cpp handles PATH searching. In no particular order, 1) PR_GetEnv("PATH") should be tested for null. There's no guarantee PATH exists for the process. It's an interesting question what to do in this case. 2) While the code tests for an absolute path ("/") it doesn't test for "./" or ../". There is a big difference between executing "./foo" and "foo". While this would almost certainly be some sort of user error, executing some other random program is certainly not a good thing. I think that fixing this would mean using nsLocalFile::Normalize but it's at least partially broken now on unix (bug 110769). 3) I am not sure but I don't think the code correctly implements the Linux PATH semantics. A PATH of ":/bin::/usr/bin:" is treated as ".:/bin:.:/usr/bin:.". Note that this brings us back to ./foo and Normalize. The really big issue is whether these rules apply to any other unix system. The correct solution would be to use the platform execvp but that may be quite hard to do. In the absence of a execvp wrapper I would think the only safe approach would be to invoke the user's shell to find and run the program.

Will look into it... Using the shell is not an option unless lots of other things get restructured.. There is XP code upstream that expects an nsIFile pointing to the correct helper and checks that the helper exists; this means that the nsIFile at that point must hold the absolute path to the helper. See bug 78919 for more on that.

The proper solution, of course, is a wrapper around execvp but mozilla doesn't understand unix basics so that's a big problem. In the interim, I know I've said this somewhere else but I can't see anything wrong with argv[0] = "/bin/sh"; argv[1] = "-c"; argv[2] = "concatenated stuff"; It gets the job done.

That fails in the current architecture due to lack of a way to pass around argv[1] and argv[2]. Working on that, but that's a separate bug.

I asked and NSPR has no equivalent for execvp. That's dumb of it. I guess some platform may support the ".../" convention so that needs to be considered. I noticed that glibc's execvp source is in the posix directory which hints that the parsing rules I gave may be posix. If true, that's good. Of course, there's no reason to assume that every platform using this code is posix. That's bad. Unless there's some other way to get to execvp, the safest solution is still to invoke a shell. There might be some issues with shell metacharacters but I think that's a minor issue.

It's not that minor.... it means we have to handle escaping the filename of the file we run the helper on in a way that leaves it as unmangled as possible but still makes sure that the shell will not do anything funky with it. That's not exactly a simple problem. :) Let's keep this bug focused on the PATH parsing (which _can_ be fixed by 1.0) and not on architectural issues outside the scope of it (which will not be fixed by then, at least not by me).

I think I was unclear. If mozilla mis-parses a PATH on any platform and tries to run the wrong executable I consider that a critical security flaw. So having mozilla parse the PATH means that the code has to be carefully tested and verified for each and every platform. That seems a lot harder to me than spawning a shell but what do I know. :-) [ time passes ] A brilliant idea (or just random electrical disturbances). We could always write a wrapper program that just took the real program name and the argument and used the platform execvp.

The problem is that this code is _not_ being called when the helper is executed. It's called _way_ before that and the caller expects an absolute path the the helper out. So things suck and will continue doing so until someone does some arch work. I'll try do it once I graduate and have some time. Attaching a patch that fixes the three issues you list at the beginning: 1) Test for null and bail if there is no PATH (essentially "helper not found") 2) ./foo or ../foo for the helper app command -- bail out, since I can't think of a good place to resolve these relative to. 3) ":/bin::/usr/bin:" now treated as "/bin:/usr/bin" tenthumbs, would you review?

I suggest you add a comment mentioning the relevant bugs. It may keep someone from messing with your code in your absence. #1 is fine with me. #2 is acceptable if the error message mentions the helper. #3 is a problem. You are basically altering the user's PATH without the user's consent and that's very risky. I can't go along with it. Sorry about that. Just for the record, here's a more complete version of my proposal. Compile a simple program like #include <errno.h> #include <unistd.h> int main (int argc, char *argv[]) { errno = ENOENT; if (argc > 1) execvp (argv[1], &argv[1]); exit (errno); /* _exit or return maybe ? */ } Call it something like mailcap_helper and install it in the same directory as mozilla-bin. You now have the equivalent of the absolute path $MOZILLA_FIVE_HOME/mailcap_helper which should be knowable at any time. If not, mozilla-bin is dead in the water anyway. Add an extra slot to the argv array with the name of the command you want to execute and call the helper using the normal mozilla conventions. For example, if you want to execute "foo -v bar" then you call $MOZILLA_FIVE_HOME/mailcap_helper with an argv array that looks something like argv[1] = "foo" argv[2] = "-v" argv[3] = "bar" The helper does all the dirty work for you. With this scheme there are no platform dependencies in the main mozilla code. Anything that needs to be done can be done in the helper which is running in the platform's native environment, not mozilla's. Basically, it's a minimal shell and this is a very old trick.

> #2 is acceptable if the error message mentions the helper. There is no error message. The code just ignores the mailcap entry in question and says it doesn't know what to do with the file type > You are basically altering the user's PATH So what is the correct interpretation of ":/bin::/usr/bin:"?

> There is no error message. The code just ignores the mailcap entry in > question and says it doesn't know what to do with the file type That's rather unfriendly. > So what is the correct interpretation of ":/bin::/usr/bin:"? Look very carefully at my original report here, item 3. The rules may well be POSIX so mozilla should not ignore them.

Ah. I see. I though you meant that the current impl treats "::" as ":.:". You mean the _target_ impl should. This code has no way to be friendly. It just gets asked "is there a helper for foo in mailcap"? The caller also queries other sources (plugins, helper apps, etc). The mailcap code should not be interacting with the UI in any way. So given a "." in the path, does mozilla use its current working directory for that? Or what? I feel that we should ignore all non-absolute PATH components, since the working directory of Mozilla is not very well defined...

Oh, and the helper program thing looks good and I'll likely do that once I have a way to pass an argv around....

> The mailcap code should not be interacting with the UI in any way. Users tend to know what they have directly or indirectly added to their mailcap files so they tend to notice if something's not right. They may well blame mozilla even if it's not mozilla's fault and mozilla acquires a bad reputation. I would prefer that didn't happen. > So given a "." in the path, does mozilla use its current working > directory for that? Or what? Mozilla may not understand what a current directory is but the system does. In fact, the system always has a working directory for each process. (Yes, I know you can do "rmdir ." on Linux but look at /proc/<pid>/pwd and the system still remembers it. The installer people keep having difficulty with this concept.) Any system call implicitly uses the current directory. You're calling out to the system so you're stuck with it. Of course, mozilla could do a chdir but that's a bad idea and would instantly define the current directory anyway. As for ignoring the parts of the PATH you don't like, it's a standards compliance issue. The rules are the rules. You are not required to like them just obey them. So hold your nose and do the right thing. Let me repeat that the Right Thing is to pass the whole stinking mess off to the system as quickly as possible. Actually the helper is a little off. It should go something like #include <errno.h> #include <unistd.h> int main (int argc, char *argv[]) { if (argc > 1) { execvp (argv[1], &argv[1]); exit (errno); /* _exit, return. ??? */ } else exit (ENOENT); }

I know what the Right Thing is. Unless _you_ do it, it's not happening until after I turn in my thesis and have time to rearchitect this stinking mess. This bug, as it stands, is here only to fix issues that are critical for 1.0. An empty PATH is one. A user who puts "../../foo" in PATH is not.

No time to work on this for 1.0. Adding helpwanted keyword. If you want to work on this, please let me know and I can give some pointers...

I've been inspired a little. See bug 147659 for a first try at a mime helper. Will it fit into the current uriloader scheme?

It could... we would just return a path to the MIME helper in the MIMEInfo (and need to rewrite the launching code to get the real app to launch).

1.1alpha is frozen. Unsetting milestone and will retriage in a few days when I can make a realistic assessment of the situation.

So is this still on anyone's plate? I have a new version of the helper app available.

This is still on my plate. And I still need to change the way all this works... with the current redesign of nsIMIMEInfo we may be getting there....

Here's my latest idea for an external helper. It closes all unnecessary file descriptors and passes the arguments to wordexp with some paranoid settings. This should catch more weird shell constructs. Wordexp parses the args itself which may also be helpful. The basic idea is still that mozilla launches this and this execs the real helper.

So.. do repeated calls to wordexp() clobber the wordexp_t? Also, how do I use this to launch: xterm -e more filename without having to quote the filename in some way?

> So.. do repeated calls to wordexp() clobber the wordexp_t? No, that's the beauty of it, wordexp grows its own array as needed. See the do while loop in main in my code. > Also, how do I use this to launch: xterm -e more filename without > having to quote the filename in some way? As we discussed before, because mozilla only knows how to spawn a process by absolute pathname, the launcher lives where mozilla-bin does. Mozilla knows where that is so, using my scheme and my syntax, mozilla has to create an argv array that looks like this: argv[0]: "$MOZILLA_FIVE_HOME/launcher" argv[1]: "--" argv[2]: "first part of command" argv[3]: "next part" argv[4]: etc It's not necessary to quote anything because wordexp doesn't use a shell. It's not necessary to requote any internal quotes that might exist. You don't have to concatenate strings either which makes handling any sort of %s substitution you want to do easier. I use execvp so it's not necessary to search the PATH yourself. You can test out the launcher from the command line. Assuming you're running bash do gcc -g external-launcher.c alias try='./a.out --moz-debug --' and then you can try out stuff like try 'xterm -e more filename' try 'xterm -e more' 'filename' try 'xterm' '-e' 'more' 'filename' try xterm -e more foo.txt The quotes here are only because you're talking to a shell. From a program they're unnecessary. Wordexp also catches some of the weirder problems. try '$((2*3))' try '$((2*3) )' If you want to use wordexp inside mozilla you would need to call wordfree when you're through, as well as keeping track of all the memory allocations. I don't care in a separate process because execvp will blow everything away if it succeeds. Of course this does require a resonably POSIX-compliant system. I have no problem with special launcher versions for lesser systems.

> It's not necessary to quote anything Sorry, wrong. As I suspected: ~/test% echo test > foo\ bar ~/test% echo error > foo ~/test% ./Vla.out -- more foo\ bar :::::::::::::: foo :::::::::::::: error bar: No such file or directory And the same would happen if I were to pass a filename with a space in it from inside Mozilla, no? I'm not even starting on filenames containing dollar signs, tildes, etc. (eg a file named "~foo" in some random directory).

> Sorry, wrong. I thought you were asking if you needed to quote all the arguments. In general you don't know if you need to without parsing everything yourself. Since you have a priori knowledge that a string is a filename, obviously you know what to do. Is this really an issue? Mozilla is launching a helper app, right? Presumably it's using temp file names so it has control over their form. anything really goofy is either a typo or something malicious. In either case I'm happy to see it fail, provided mozilla issues a sensible warning.

Well... I _could_ munge all the filenames with spaces in them to have underscores instead. But that seems utterly unnecessary. As a note, we keep the original filename whenever possible (from url or content-disposition) so users can find the file in /tmp if needed. Finally, once I fix the "local files should be opened in-place" bug we need to be able to deal with arbitrary filenames because we're not going to be renaming those.

> Well... I _could_ munge all the filenames with spaces in them to have > underscores instead. But that seems utterly unnecessary. > As a note, we keep the original filename whenever possible (from url or > content-disposition) so users can find the file in /tmp if needed. Well, I think the users should be able to find something that's similar to the orginal name but I'm not sure absolute fidelity is a good idea. How well does Windows deal with names like "foo+bar" or "foo,bar"? I think it's essential that the user can clean up after any mozilla crash (yes, I know they never happen) with the native tools so I think that mozilla should do some translation on names. But I'm very weird. > Finally, once I fix the "local files should be opened in-place" bug we need > to be able to deal with arbitrary filenames because we're not going to be > renaming those. Are you mutating this bug? I don't mind if you are but I've always thought this one was about helpers.

That _is_ about helpers... if I have a file:// link to a PDF in Mozilla and I click it, I expect it to open in acroread.... and I expect acroread to open the original file on disk, not a copy Mozilla made in /tmp. For now we always copy, but I'd like any proposed architecture for this to handle both cases....

> That _is_ about helpers... if I have a file:// link ... Yeah, I almost never use file: so I forgot about that. > For now we always copy, but I'd like any proposed architecture for > this to handle both cases.... I think real local files are actually distinct from a network file saved locally. Simply because a real file exists, the path name is guaranteed to be correct for the system. Saving a network file to disk involves such things as foreign filesystems (remember saving to a FAT partition). The name you think you're saving to may be silently mangled into something else. Seemingly distinct names may be mapped into the same thing. I think for network files copying may always be necessary, or at least storing them in the final directory with a temp name and later renaming them. It would be nice if objects in the cache were stored separately because you could use them from there but I guess the cache people have other ideas. As for my launcher, I think I know how to tell it to treat certain args as literals and to not process them. I should have a new version soon.

I added a --moz-literal option whichi can occur after the "--" separator and says that the following arg is a literal string.

Fixed a build problem with HAVE_VLA and old gcc versions like egcs. Added a check for some debugging environment variables. This could help with problems in the field. Maybe we should consider adding environment sanitizing.

Bug 33282 is related

Note: we either need to finish/review/etc this, or add functionality equivalent to this to mozilla/nspr/etc itself in order to commit 33282 without security holes.

Let's be careful here and not rush into things. I wrote the launcher so that mozilla would not have to parse the PATH variable so the launcher user execvp not execve. It really needs to do that but that does mean some concern that something, say a rogue plugin, might change PATH on mozilla. This is actually part of a larger problem but it's not this bug. More importantly the launcher runs outside the mozilla environment. That's also by design. It's much easier to find a platform expert that a combined mozilla/platform one. You may well need platform-specific versions. Just a few thoughts.

Speaking of environment variables, the startup shell scripts set or alter a lot of them. Should these be passed to a helper app? Does a helper app really need to know where to find mozilla libraries? Will the helper work correctly if the dynamic linker looks in the mozilla directories before the system ones? Something to think about. It's certainly possible to address this but it means a smarter way of starting mozilla and the powers that be don't seem interested. Maybe they'll wake up and smell the paranoia.

tenthumbs@cybernex.net, would you like to take this bug?

I don't think I'm qualified enough to mess around with some of this stuff. I don't really understand mozilla internals and I only have linux available which limits me. I have been thinking about this, though, and there's more to be done but it would require rethinking how to create, enter, and leave the mozilla environment. For example, shouln't mozilla sanitize the environment variables when it calls a helper? If so, this would mean retaining the starting environment. Be that as it may, you can assign it to me but I don't think I'll do a lot with it any time soon.