Summary: | [hovering] executing javascript in javadoc potentially malicious | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | [Eclipse Project] JDT | Reporter: | Jacek Pospychala <jacek.pospychala> | ||||||||
Component: | Text | Assignee: | Dani Megert <daniel_megert> | ||||||||
Status: | RESOLVED FIXED | QA Contact: | |||||||||
Severity: | normal | ||||||||||
Priority: | P3 | CC: | bpasero, daniel_megert, darin.eclipse, feldhacker.chris, froste, krzysztof.daniel, markus.kell.r, pengchenglee, philippe_mulet, remy.suen | ||||||||
Version: | 3.2.2 | ||||||||||
Target Milestone: | 3.5 M6 | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Whiteboard: | |||||||||||
Bug Depends on: | 161864 | ||||||||||
Bug Blocks: | |||||||||||
Attachments: |
|
Description
Jacek Pospychala
2008-04-24 03:28:11 EDT
You shouldn't even load and then look at malicious code at all ;-) Same in Javadoc view and when using Navigate > Open External Javadoc (Shift+F2). Created attachment 97769 [details]
proposed fix
Dani,
how about stripping scripts like this way?
Will look during 3.5. Playing with the HTML format isn't something I want to do at this stage. Proposed fix should be case-insensitive. How about: Matcher m = Pattern.compile("(?is)<(script)[^>]*>[^<]*</(script)[^>]*>").matcher(input); String result = m.replaceAll(""); return result; 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. 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... (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. 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. 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. 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. 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. (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. (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). 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 ? 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? Also, is the concern only for JavaScript i.e. would the flag to disable JavaScript on the Browser be enough or is there more? 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? >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. 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. 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? (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? >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 ;-)
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.
*** Bug 238499 has been marked as a duplicate of this bug. *** Fixed in HEAD. Available in builds > N20090224-2000. *** Bug 300974 has been marked as a duplicate of this bug. *** . . Making this bug publicly visible. This is fixed in all current releases and streams. |