Bug 228608 - [hovering] executing javascript in javadoc potentially malicious
Summary: [hovering] executing javascript in javadoc potentially malicious
Status: RESOLVED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Text (show other bugs)
Version: 3.2.2   Edit
Hardware: All All
: P3 normal with 1 vote (vote)
Target Milestone: 3.5 M6   Edit
Assignee: Dani Megert CLA
QA Contact:
URL:
Whiteboard:
Keywords:
: 238499 300974 (view as bug list)
Depends on: 161864
Blocks:
  Show dependency tree
 
Reported: 2008-04-24 03:28 EDT by Jacek Pospychala CLA
Modified: 2011-04-08 14:46 EDT (History)
10 users (show)

See Also:


Attachments
proposed fix (1.70 KB, patch)
2008-04-28 10:04 EDT, Jacek Pospychala CLA
no flags Details | Diff
workaround (3.92 KB, patch)
2008-05-13 05:41 EDT, Jacek Pospychala CLA
no flags Details | Diff
Plug-in to disable Browser widget (3.89 KB, application/x-jar)
2008-05-29 04:47 EDT, Dani Megert CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jacek Pospychala CLA 2008-04-24 03:28:11 EDT
I have following class:

/**
 * <script>
 * window.open("http://google.com");
 * </script>
 */
public class Test {

}


Now, when javadoc popup is displayed, or javadoc view is visible any javascript is executed, which is potentially malicious.

Would it be possible to strip <script> contents before rendering javadoc?
Comment 1 Dani Megert CLA 2008-04-24 03:33:17 EDT
You shouldn't even load and then look at malicious code at all ;-)
Comment 2 Dani Megert CLA 2008-04-24 03:42:27 EDT
Same in Javadoc view and when using Navigate > Open External Javadoc (Shift+F2).
Comment 3 Jacek Pospychala CLA 2008-04-28 10:04:05 EDT
Created attachment 97769 [details]
proposed fix

Dani,
how about stripping scripts like this way?
Comment 4 Dani Megert CLA 2008-04-28 10:07:55 EDT
Will look during 3.5. Playing with the HTML format isn't something I want to do at this stage.
Comment 5 Feldhacker CLA 2008-04-28 10:54:25 EDT
Proposed fix should be case-insensitive.

How about:
Matcher m = Pattern.compile("(?is)<(script)[^>]*>[^<]*</(script)[^>]*>").matcher(input);
String result = m.replaceAll("");
return result;
Comment 6 Jacek Pospychala CLA 2008-05-05 05:12:28 EDT
Looking closer at this, playing with HTML doesn't seem good idea. Our potential hacker could possibly try injecting javascript using iframe or in milllions other ways!

Other option is to disable javascript in browser (bug 161864) - in org.eclipse.jface.internal.text.html.BrowserInformationControl.
Comment 7 Feldhacker CLA 2008-05-05 09:30:35 EDT
Is the intent of the Javadoc popup box to render/execute ALL possible HTML?  There are several items that don't make sense in a hover popup: scripts, iframes, applets, objects (flash/activex), forms, etc.

I think the intent of the Javadoc popup box is to utilize the HTML formatting instructions in the Javadoc to present the documentation in the most "friendly" format possible -- nobody wants to see text all run together with "<p>", "<br>", or "<li>" tags scattered throughout.  Utilizing the HTML formatting tags (if present) makes sense and makes the comments more readable.  

On the other hand, there are several HTML tags that have nothing to do with formatting -- such as thosed listed above -- and are not relevant in terms of displaying documentation to a user.  Allowing such HTML tags can (in the worse-case) allow malicious code to be executed on the user's machine or at least direct the user to a malicious site.

This is not a new problem.

Limiting the HTML that's rendered happens in many different programs -- the most common example being Outlook.  Because the HTML in a message could be dangerous, only "formatting" tags are rendered by default -- scripts, embedded objects, and external links are blocked -- I have to explicitly click a button to download images.  In this context, the Javadoc popup box is very similar to Outlook -- HTML tags must be separated into "safe" or potentially "unsafe" categories and handled appropriately...

Yes, secure application development is challenging, but that doesn't mean it can be ignored...
Comment 8 Jacek Pospychala CLA 2008-05-05 10:08:57 EDT
(In reply to comment #7)
All correct Chris, unfortunately security is pretty wide topic to discuss it all here.

I believe if someone wanted to compromise your security, they'd rather put malicious code, which is hard/impossible to find, than javadoc.

It's also difficult to compare with Outlook, where user data is at risk. In javadoc example, one can only get cookies, browser history, etc. So for me it sounds a bit like fixing browser security bugs in Eclipse.
Comment 9 Jacek Pospychala CLA 2008-05-12 06:57:23 EDT
Another possible solution (until bug 161864 gets fixed):
If Browser is not available, JDT uses
org.eclipse.jface.text.DefaultInformationControl.DefaultInformationControl (in JavadocHover) and org.eclipse.swt.widgets.StyledText (in JavadocView) to try display simple HTML (DefaultInformationControl) or raw HTML (StyledText).

There could be a preference added to explicitly disable/enable this simple HTML mode for users who mind security.
Comment 10 Jacek Pospychala CLA 2008-05-13 05:38:33 EDT
Problem was found originally in Eclipse 3.2.2 (/RAD7.0) and we need fix for 3.2.2 as well as for anything more decent.
Comment 11 Jacek Pospychala CLA 2008-05-13 05:41:56 EDT
Created attachment 99911 [details]
workaround

workaround as described in comment 9.
It's against Eclipse 3.2.2 = tag r322_v20070124

With patch applied, user can disable insecure browser by adding -DJDT_DISABLE_BROWSER to command line or eclipse.ini.
Comment 12 Dani Megert CLA 2008-05-13 06:24:29 EDT
You would not want to use the text widget if a browser is available. If security is a concern then simply don't attach source or Javadoc from a source you don't trust. Even with your workaround the problem persists as soon as the Javadoc is shown in an external browser, e.g. using Shift+F2.
Comment 13 Jacek Pospychala CLA 2008-05-13 07:36:08 EDT
(In reply to comment #12)
> You would not want to use the text widget if a browser is available. If
> security is a concern then simply don't attach source or Javadoc from a source
> you don't trust. 

Security is concern, but it's better to have any javadoc (text widget) than nothing at all.
Also, if I had paranoia, I'd be affraid not only of external javadocs, but e.g. incorrect Code Templates, like:

/**
 * @author <script>alert("hi ${user}! :)")</script>
 */

Afterall it'd be better to let users throw anything into their editors and make sure it won't blow their screens, instead of denying hundrets of things, like external javadocs, top code templates, etc.etc.
Comment 14 Jacek Pospychala CLA 2008-05-21 11:16:36 EDT
(In reply to comment #12)
> You would not want to use the text widget if a browser is available. If
> security is a concern then simply don't attach source or Javadoc from a source
> you don't trust. Even with your workaround the problem persists as soon as the
> Javadoc is shown in an external browser, e.g. using Shift+F2.


Just to clarify, as of chat today earlier with Dani, in External browser it's possible to disable javascript, so Shift+F2 is not a problem. But even having javascript disabled in IE, doesn't disable it in SWT Browser in JDT (see comment 13 in bug 161864).

For Eclipse 3.2, it still seems the easiest fix would be the opportunity to revert to no-browser functionality. (attachment 2 [details] - workaround).

Comment 15 Philipe Mulet CLA 2008-05-22 04:23:04 EDT
I would share Dani's opinion that Javadocs are essentially part of the original library code. So in term of security, it shouldn't be a problem, i.e. if you trust the library code, then you should trust the library javadoc which comes with it.
In essence, nothing precludes the library code to wipe out your hard drive when you run it... Javadocs may be evil as well...

Jacek - what is exactly the scenario in real life ? Are there cases where arbitrary Javadocs are being referenced, and thus exposing this kind of evil outcome ?
Comment 16 Dani Megert CLA 2008-05-22 04:45:35 EDT
Using the text widget for Javadoc instead of the Browser is not the right thing to do in my opinion:
1) there are many other places where the Browser widget is used to display things, e.g. if you use the internal browser, PDE uses it, intro/Help uses the widget to display help (i.e. if you install a plug-in with malicious help you're also doomed like in the Javadoc case) and refactoring uses it in their previews.

2) the hover will look ugly, as the text version only provides very minimalistic API

I think the best fix would be to have a mode (either a global preference in the UI or a system property) that disables JS in the Browser widget.

Jacek, what do you think?
Comment 17 Dani Megert CLA 2008-05-22 06:01:47 EDT
Also, is the concern only for JavaScript i.e. would the flag to disable JavaScript on the Browser be enough or is there more?
Comment 18 Feldhacker CLA 2008-05-22 09:26:13 EDT
The distinction between "intended behavior" and a vulnerability in part relates to what action is required of the user for the "bad" thing to occur.  Somebody could email me a trojan, but I still have to make the choice to double-click and execute it for it to impact me.  (Which is why, as an added protection, programs like Outlook warn that opening attachments could be dangerous.)  Somebody could email me a link to a malicious web site, but I still have to click on the link to actually navigate to the page.

The actual Java code could delete every file on my harddrive (a trojan), but I still have to explicitly choose to run the code.


In this case, all a developer has to do is rest their mouse over the wrong part of the screen and the scripts in the JavaDoc will be executed...

And the concern isn't just with "destructive" scripts, right?  As a way to make money, maybe I'll release code with Javadocs with scripts that simulate an ad-click -- I could make money every time a developer places their cursor over my class/method name.  Or, as a shady vendor, maybe I'll try to track usage/distribution by embedding scripts that will ping one of my servers with who knows what information.  (Or, maybe this could also be extended to corporate espionage use-cases with scripts that gather and submit data...)


Currently, not much protection is offered -- scripts are executed just by resting the mouse cursor in the wrong/right place.  Something more should be done -- disable script execution, display a warning when scripts are present, strip-out any scripts, something...

The concern is with anything that is automatically executed by hovering, and scripts are the primary example.  But, does the same situation exist if the Javadoc contains embeded "<object>"s?  Are embedded ActiveX controls executed automatically and without warning as well?
Comment 19 Dani Megert CLA 2008-05-22 09:30:00 EDT
>But, does the same situation exist if the
>Javadoc contains embeded "<object>"s?  Are embedded ActiveX controls executed
>automatically and without warning as well?
Right, that's why I suspect that either the suggested API from bug 161864 must be extended or we can't use that approach.
Comment 20 Philipe Mulet CLA 2008-05-23 03:42:02 EDT
Re: comment 18
We all agree that the automatic execution is problematic, and we are trying to come up with a good solution for it. Note that when you download random plug-ins, they may contribute evil builders which are going to perform automatically... so beware there as well.

Anyway, there are 2 sides of the problem. For 3.5, we can introduce necessary APIs in SWT/Text to address these concerns. There may be more problems than just JavaScript indeed; so the required API needs to be discussed with SWT team based on what the widget can deliver etc...

Looking back, for addressing this concern in earlier streams, then we have a compatibility rule which prevent from adding any new API in a service release/update. So our solution there would have to look different, probably revolving around resurrecting some basic html renderer, which isn't super nice any longer, but secure again.
Comment 21 Dani Megert CLA 2008-05-26 11:32:01 EDT
In my opinion the most important part to address asap is to offer users a way to disable HTML in the Javadoc hover, correct? Like other areas, the Javadoc view is is also affected but it's less dangerous than the hover and the view can be closed if deemed dangerous.

I suggest the following approach for the Javadoc hover: we offer a plug-in that adds a preference like "Use simple HTML in Javadoc hover" which is on by default. 

The benefit of this is
1. works for all versions that use the Browser widget in the hover (3.2 - 3.4)
2. clients can not only "patch" original Eclipse downloads but all Eclipse-based 
   products
3. we don't need touch the 3.4 code base at this very late stage (we will add a 
   README entry with a link to the plug-in)


For 3.5 we will find a solution together with SWT that allows to disable malicious code directly in the Browser widget and apply that also to the Javadoc view.

Is that acceptable?
Comment 22 Jacek Pospychala CLA 2008-05-26 11:37:06 EDT
(In reply to comment #21)
> I suggest the following approach for the Javadoc hover: we offer a plug-in that
> adds a preference like "Use simple HTML in Javadoc hover" which is on by
> default. 

it sounds perfect.
btw, how would that new plug-in alter hover behaviour without any changes to jdt.ui plug-in?
Comment 23 Dani Megert CLA 2008-05-26 11:41:05 EDT
>btw, how would that new plug-in alter hover behaviour without any changes to
>jdt.ui plug-in?
That's not something I'd want to reveal in public. Drop me a note if you absolutely want to know this ;-)
Comment 24 Dani Megert CLA 2008-05-29 04:47:09 EDT
Created attachment 102584 [details]
Plug-in to disable Browser widget

Installing this plug-in disables the Browser widget in hovers. Works for R3.2.x up to R3.4.x.
Comment 25 Dani Megert CLA 2008-07-08 02:53:03 EDT
*** Bug 238499 has been marked as a duplicate of this bug. ***
Comment 26 Dani Megert CLA 2009-02-25 06:14:46 EST
Fixed in HEAD.
Available in builds > N20090224-2000.
Comment 27 Markus Keller CLA 2010-01-27 06:50:06 EST
*** Bug 300974 has been marked as a duplicate of this bug. ***
Comment 28 Dani Megert CLA 2010-02-25 06:18:50 EST
.
Comment 29 Dani Megert CLA 2010-02-25 06:24:22 EST
.
Comment 30 John Arthorne CLA 2011-04-08 14:46:59 EDT
Making this bug publicly visible. This is fixed in all current releases and streams.