Open Bug 125505 Opened 22 years ago Updated 1 year ago

PATH handling in uriloader/exthandler/unix/nsOSHelperAppService.cpp

Categories

(Firefox :: File Handling, defect)

All
Linux
defect

Tracking

()

People

(Reporter: tenthumbs, Unassigned)

Details

(Keywords: helpwanted)

Attachments

(2 files, 2 obsolete files)

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.
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → mozilla1.0
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...
Keywords: helpwanted
Target Milestone: mozilla1.0 → mozilla1.1
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.
Target Milestone: mozilla1.1alpha → ---
QA Contact: sairuh → petersen
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....
Attached file launcher.tar.gz (obsolete) —
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.
Attached file launcher.tar.gz 2 (obsolete) —
I added a --moz-literal option whichi can occur after the "--" separator and
says that the following arg is a literal string.
Attachment #104536 - Attachment is obsolete: true
Attached file launcher.tar.gz 3
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.
Attachment #104771 - Attachment is obsolete: true
Hardware: PC → All
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?
Priority: P2 → P4
Target Milestone: --- → Future
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.
QA Contact: chrispetersen → file-handling
Assignee: bzbarsky → nobody
Status: ASSIGNED → NEW
Priority: P4 → --
Target Milestone: Future → ---
Product: Core → Firefox
Version: Trunk → unspecified
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.