392149 - -osint protection can be subverted via remote options

Status: RESOLVED FIXED

(Toolkit :: Startup and Profile System, defect)

Description

[Andrew Schultz]

The -osint option is supposed to cause app startup to fail if certain options are passed.  However, if -osint and remote options -a or -u are used, RemoteCommandLine returns failure, but XRE_main interprets that as meaning that remoting didn't work and continues startup.  CheckArg gobbled the -osint argument, so any further options that should also cause failure do not and "work".

Comment 1

[Andrew Schultz]

Attachment: return stronger failure

Comment 2

[Robert Strong (they/them - no direct email)]

Forgive my ignorance but are you using Windows shell integration to make this happen?

Comment 3

[Andrew Schultz]

Robert, OS=linux

Comment 4

[Robert Strong (they/them - no direct email)]

That's what I thought, the bug that -osint was added for had to do with windows shell integration. This is the right thing to do but not having this shouldn't affect security since we don't have the issue as on Windows where using the c runtime can create multiple arguments. We haven't added the -osint arg to the Linux integration in part because of this though we can if someone is up for it. We haven't added it to Mac OS X integration since it uses Apple Events.

Comment 5

[Andrew Schultz]

I'm not sure how the C runtime enters into it.  Any linux app could preface parameters with -osint thinking that firefox/seamonkey/thunderbird would protect itself from any scary parameters that made it through its own sanity checking.

Comment 6

[Robert Strong (they/them - no direct email)]

The C runtime on Windows enters into it because we use it on Windows and it will split the command line into multiple arguments which doesn't affect Linux. The -osint argument is meant specifically for OS Integration of our registered handlers and the command line handler code expects it to be followed by a specific flag that will contain specifically formatted argument. To use it outside of the app provided OS Integration an extension can provide a handler registration with -osint -myFlag somearg and then have a command line handler that will validate that what is coming from the OS doesn't contain additional arguments hidden by using a single argument that is specially crafted with single / double quotes, spaces, etc. Using it outside of our registration of default handler with the OS will be hit and miss though it will act as you have seen in nsAppRunner.cpp and with the -url flag.

Comment 7

[Robert Strong (they/them - no direct email)]

Comment on attachment 276604 [details] [diff] [review]
return stronger failure

Keep in mind that this will only really be valid when the arguments are -osint -url http://etc. If this isn't the argument passed then you will get the following in the error console since it is meant to be used along with the url falg for os integration only.

Error: Warning: unrecognized command line flag -osint

Comment 8

[Andrew Schultz]

OK, I landed on trunk

Comment 9

[Andrew Schultz]

Attachment: merged to branch

This has been on trunk for a couple weeks with no adverse effects.

Comment 10

[Daniel Veditz [:dveditz]]

landed on trunk -> FIXED

Comment 11

[Daniel Veditz [:dveditz]]

Comment on attachment 279406 [details] [diff] [review]
merged to branch

approved for 1.8.1.7, a=dveditz for release-drivers

Comment 12

[juan becerra [:juanb]]

Andrew, could you help us verifying this on FF 20087rc2? Step by step instructions for a wide audience would also be great.

Comment 13

[Andrew Schultz]

juan: with firefox not already running, invoke on the commandline:

firefox -a blah -osint

This should not start the app.  Without the patch, firefox would start.

Comment 14

[Daniel Veditz [:dveditz]]

Assigning sg:low because so far the -osint fix on Linux is extra layers of protection, we don't actually know of a remote way to call the default handler with extra options.

Comment 15

[Stephen Donner [:stephend] Not actively reading bugmail]

mozilla@mozilla-qa:~/Desktop/firefox$ ./firefox -a blah -osint
Error: argument -osint is invalid
Error: argument -a requires an application name

My Linux-fu is weak; what should I pass -a to get the warnings to subside (and verify this bug?)

Comment 16

[Andrew Schultz]

> My Linux-fu is weak; what should I pass -a to get the warnings to subside (and
> verify this bug?)

What you got is the correct behavior (firefox did not start -- you verified this bug).  Firefox should reject the -a argument because of -osint.  If you want to verify that -a would work without -osint, you can do

./firefox -a firefox

If firefox is running, that should open a new window from the existing instance.  If it's not running, that should start firefox (a new instance).

Comment 17

[Stephen Donner [:stephend] Not actively reading bugmail]

Andrew, thanks; because this is a stock Ubuntu Fiesty install (which includes 2.0.0.6,), I had to set my PATH to the Firefox 2.0.0.8 rc2 candidate--installed on the Desktop--as such:

mozilla@mozilla-qa:~$ export PATH=/home/mozilla/Desktop/firefox/:$PATH

By issuing:

mozilla@mozilla-qa:~/Desktop/firefox$ firefox -a blah -osintError: argument -osint is invalid
Error: argument -a requires an application name

Firefox doesn't launch, where it DOES, by doing this:

mozilla@mozilla-qa:~/Desktop/firefox$ ./firefox -a firefox

Verified FIXED using Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.8) Gecko/20071008 Firefox/2.0.0.8

Replacing fixed 1.8.1.8 keyword with verified1.8.1.8.

Comment 18

[Alexander Sack]

Comment on attachment 279406 [details] [diff] [review]
merged to branch

we have this in distro patches. requesting approval for 1.8.0.15.

Comment 19

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 279406 [details] [diff] [review]
merged to branch

a=caillon for 1.8.0.15

Comment 20

[Christopher Aillon (sabbatical, not receiving bugmail)]

Fix committed to the 1.8.0 branch