Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 347224 - [Help][Security] Need to encode URL in livehelp.js
Summary: [Help][Security] Need to encode URL in livehelp.js
Status: RESOLVED WONTFIX
Alias: None
Product: Platform
Classification: Eclipse Project
Component: User Assistance (show other bugs)
Version: 3.7   Edit
Hardware: PC Windows 7
: P3 normal (vote)
Target Milestone: ---   Edit
Assignee: platform-ua-inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-05-25 17:29 EDT by Chris Goldthorpe CLA
Modified: 2012-01-13 12:28 EST (History)
1 user (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Goldthorpe CLA 2011-05-25 17:29:43 EDT
I20110523-0800

/org.eclipse.help/livehelp.js contains these lines

	// construct the proper url for communicating with the server	
	var url= window.location.href;
	
	var i = url.indexOf("?");
	if(i>0)
		url=url.substring(0, i);
	
	i = url.indexOf("/topic/");
	if(i < 0)
		i = url.lastIndexOf("/");

	url=url.substring(0, i+1);
	var encodedArg=encodeURIComponent(argument);
	url=url+"livehelp/?pluginID="+pluginId+"&class="+className+"&arg="+encodedArg+"&nocaching="+Math.random();

	// we need to find the toolbar frame.
	// to do: cleanup this, including the location of the hidden livehelp frame.	
	var toolbarFrame = topHelpWindow.HelpFrame.ContentFrame.ContentToolbarFrame;
	if (!toolbarFrame){
		window.location=url;
		return;
	}

Note that url needs to be encoded before using to set the location. I don't believe that this is an exploitable vulnerability but it should be fixed so that the code is completely clean.
Comment 1 Chris Goldthorpe CLA 2011-05-26 17:27:26 EDT
This problem was originally reported by Rational Appscan version . There is not an exploitable vulnerability but the code should be cleaned up to encode when appropriate.

I've carefully reviewed the code in livehelp.js which AppScan flagged in it's report and concluded that there is not an exploitable vulnerability.

AppScan detected a case where an href was read in JavaScript and used in a DOM operation without being encoded. This indicates a possible vulnerability, however code analysis is required to determine if an exploitable vulnerability exists. If there is an exploitable vulnerability this bug should have a high severity, if not the priority should be lower, we still want to clean up the code to add the encoding but it is not a stop ship issue.

The variable which is not encoded is called "url" which is initialized on line 62 as follows:

    var url= window.location.href;
 		
    var i = url.indexOf("?");
    if(i>0)
        url=url.substring(0, i);


window.location.href starts off containing the URL of the help page being opened with possibly an anchor or parameters at the end. An XSS vulnerability would exist if a malicious string inserted into the parameter or anchor could cause bad things to happen when the page is loaded or a script executed. In this case the parameters are immediately stripped off so the only possible way the variable "url" could still contain malicious characters at this point would be if they were in the anchor.

The next few lines truncate everything in the URL starting from /ntopic/ If /ntopic/ is not in the url the code returns.

    i = url.indexOf("/ntopic/");
    if(i < 0) {
	// not help view
	return;
    }
	
    url=url.substring(0, i+1);


The only way that the variable "url" can still contain a malicious payload at this point is if it occurs after a '#' character. Finally the rest of the path is appended to url.


	var encodedArg=encodeURIComponent(argument);
	url=url+"livehelp/?pluginID="+pluginId+"&class="+className+
            "&arg="+encodedArg+"&nocaching="+Math.random();
        window.location.href = url;

At this point window.location has been set to a string which could at worst have malicious characters after a '#' sign, i.e. in the anchor section. Since the anchor is not even passed to the server there is no possibility of exploitation.
Comment 2 Chris Goldthorpe CLA 2011-10-11 17:11:21 EDT
This is no longer being flagged by Appscan, since there is no underlying vulnerability I am closing as WONTFIX.
Comment 3 Wayne Beaton CLA 2012-01-13 12:10:01 EST
Does this bug need to be restricted to committers only?
Comment 4 Chris Goldthorpe CLA 2012-01-13 12:28:16 EST
This is not an exploitable vulnerability so I have removed the committer-only flag.