-osint protection can be subverted via remote options

RESOLVED FIXED

Status

()

RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: ajschult784, Assigned: ajschult784)

Tracking

({fixed1.8.0.15, verified1.8.1.8})

Trunk
x86
Linux
fixed1.8.0.15, verified1.8.1.8
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:low] [need testcase])

Attachments

(2 attachments)

(Assignee)

Description

11 years ago
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".
(Assignee)

Comment 1

11 years ago
Created attachment 276604 [details] [diff] [review]
return stronger failure
Attachment #276604 - Flags: review?(benjamin)
Forgive my ignorance but are you using Windows shell integration to make this happen?
(Assignee)

Comment 3

11 years ago
Robert, OS=linux
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.
(Assignee)

Comment 5

11 years ago
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.
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.

Updated

11 years ago
Attachment #276604 - Flags: review?(benjamin) → review?(robert.bugzilla)
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
Attachment #276604 - Flags: review?(robert.bugzilla) → review+
(Assignee)

Comment 8

11 years ago
OK, I landed on trunk
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Comment 9

11 years ago
Created attachment 279406 [details] [diff] [review]
merged to branch

This has been on trunk for a couple weeks with no adverse effects.
Assignee: nobody → ajschult
Status: RESOLVED → ASSIGNED
Attachment #279406 - Flags: approval1.8.1.7?
Resolution: FIXED → ---
landed on trunk -> FIXED
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED
Comment on attachment 279406 [details] [diff] [review]
merged to branch

approved for 1.8.1.7, a=dveditz for release-drivers
Attachment #279406 - Flags: approval1.8.1.7? → approval1.8.1.7+
(Assignee)

Updated

11 years ago
Keywords: fixed1.8.1.7
Andrew, could you help us verifying this on FF 20087rc2? Step by step instructions for a wide audience would also be great.
Whiteboard: [need testcase]
(Assignee)

Comment 13

11 years ago
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.
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.
Whiteboard: [need testcase] → [sg:low] [need testcase]
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?)
(Assignee)

Comment 16

11 years ago
> 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).
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.
Keywords: fixed1.8.1.8 → verified1.8.1.8

Comment 18

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

we have this in distro patches. requesting approval for 1.8.0.15.
Attachment #279406 - Flags: approval1.8.0.15?
Comment on attachment 279406 [details] [diff] [review]
merged to branch

a=caillon for 1.8.0.15
Attachment #279406 - Flags: approval1.8.0.15? → approval1.8.0.15+
Fix committed to the 1.8.0 branch
Keywords: fixed1.8.0.15
Group: security

Updated

10 years ago
Component: XRE Startup → Startup and Profile System
QA Contact: xre.startup → startup
You need to log in before you can comment on or make changes to this bug.