574462 - "FireFTP", vulnerability of injection script to chrome

Status: RESOLVED INVALID

(addons.mozilla.org :: Security, defect, P2)

Description

[Alice0775 White]

Japanese blog reported.
http://d.hatena.ne.jp/teramako/20100621/p1 (in japanese)

Install add on "FireFTP" ( hhttps://addons.mozilla.org/en-US/firefox/addon/684/ ),
the FTP "welcome message" can inject script to chrome.

gFxp.welcomeMessage is not sanitized.

Actual
the FTP "welcome message" can inject script to chrome.

Expected 
gFxp.welcomeMessage should be sanitized.

It is difficult to prepare testcase. However, this vulnerability is as same as WizzRSS'vulnerability.

Comment 1

[Jorge Villalobos [:jorgev] (he/him)]

Adding author to CC list.

Comment 2

[Mime Čuvalo]

Just returned from an extended vacation - I will look into it as soon as I can.

Comment 3

[Mime Čuvalo]

Apologies for the delay in responding - I've been away for 2 months building a house in the middle of nowhere with internet connection!  Finally got my computer up and running again...

Anyway, unless I'm mistaken this seems to be a duplicate of bug 476851 which was already resolved last year.

And then furthermore, the only other place welcomeMessage is used is in welcome.xul which inserts the message into a textbox, not an HTML capable widget.
see: http://www.mozdev.org/source/browse/fireftp/src/content/welcome.xul?rev=1.8

Please check my logic and review bug 476851.

Comment 4

[Jorge Villalobos [:jorgev] (he/him)]

Adding Wladimir, who analyzed the previous fixes. Could you please have a look?

According to the blog (as far as I understand), they didn't verify the exploit themselves, so it could be a false alarm.

Comment 5

[Wladimir Palant]

Yes, seems to be a false alarm to me. Bug 476851 was about a different issue, handling of the welcome message was safe even then. What I can see:

* gFtp.welcomeMessage is being set by connection/controlSocket.js. No sanitizing performed but it isn't necessary either.

* gFtp.welcomeMessage is used in a few locations, always as a parameter to displayWelcomeMessage(). This function opens chrome://fireftp/content/welcome.xul and passes the message as a window argument.

* Further processing in welcome.xul is very simple:

> $('message').value = window.arguments[0];

Here $('message') is a multi-line textbox. So it will interpret the message as text, absolutely no issue here.

I also checked for uses of innerHTML, just in case. Function appendLog() is still the only problematic case. Sanitizing is performed only if the parameter "untrusted" is set. There are a few direct calls to appendLog() without that parameter but these are safe. It's more problematic when appendLog() is called indirectly through error() function - quite a few of these calls don't have the "untrusted" parameter set despite dealing with data from uncertain sources. Typical example:

> error(gStrbundle.getFormattedString("failedDir", [remotePath]));

I'm not sure how much the remote server can manipulate the contents of remotePath variable. Nor am I really inclined to investigate this in detail because we are talking about unnecessary risk here - failedDir string doesn't even contain any HTML code. The approach was apparently to set "untrusted" parameter only for strings that are definitely not safe. I would recommend doing it exactly the other way around: have a "trusted" parameter in appendLog() that needs to be set if a string is definitely safe and contains HTML code. And then only set it in the few cases where it is absolutely necessary.

The same tendency continues with debugging functions (which shouldn't affect most users because fireftp.debugmode preference needs to be changed). detailedError() function never passes "untrusted" parameter - but seems to be unused anyway. debug() function however is used a lot and usually without "untrusted" parameter, often with potentially unsafe parameters. From what I can tell, none of the debug calls are intended to write HTML code so this risk is again unnecessary.

Comment 6

[Jorge Villalobos [:jorgev] (he/him)]

Resolving this as Invalid.

Thank you all!

Comment 7

[Mime Čuvalo]

Thanks for the analysis and confirmation, Wladimir.  I will take your suggestion to make trusted the default.