1173228 - [Linter: SetJavaScriptEnabled] Audit WebView.setJavaScriptEnabled call

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

via lint:

 ../../src/main/java/org/mozilla/search/PostSearchFragment.java:59: Using setJavaScriptEnabled can introduce XSS vulnerabilities into you application, review carefully.

  56         webview.setWebViewClient(new ResultsWebViewClient());
  57 
  58         // This is required for our greasemonkey terror script.
  59         webview.getSettings().setJavaScriptEnabled(true);
  60 
  61         return mainView;

Priority: 6 / 10
Category: Security
Severity: Warning
Explanation: Using setJavaScriptEnabled
Your code should not invoke setJavaScriptEnabled if you are not sure that your app really requires JavaScript support.

More info: http://developer.android.com/guide/practices/security.html 

---

Margaret, my assumption is that this call is necessary and we should suppress the linter warning.

However, have you had this audited? Are we certain there are no vulnerabilities here?

I'll post a patch if so.

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1173228 - Suppress SetJavaScriptEnabled linter warning. r?margaret

Bug 1173228 - Suppress SetJavaScriptEnabled linter warning. r?margaret

The function is required for a greasemonkey script and the code is expected to
be secure.

Comment 2

[:Margaret Leibovic]

Comment on attachment 8617707 [details]
MozReview Request: Bug 1173228 - Suppress SetJavaScriptEnabled linter warning. r?margaret

https://reviewboard.mozilla.org/r/10689/#review9667

Ship It!

Comment 3

[:Margaret Leibovic]

(In reply to Michael Comella (:mcomella) from comment #0)
> via lint:
> 
>  ../../src/main/java/org/mozilla/search/PostSearchFragment.java:59: Using
> setJavaScriptEnabled can introduce XSS vulnerabilities into you application,
> review carefully.
> 
>   56         webview.setWebViewClient(new ResultsWebViewClient());
>   57 
>   58         // This is required for our greasemonkey terror script.
>   59         webview.getSettings().setJavaScriptEnabled(true);
>   60 
>   61         return mainView;
> 
> Priority: 6 / 10
> Category: Security
> Severity: Warning
> Explanation: Using setJavaScriptEnabled
> Your code should not invoke setJavaScriptEnabled if you are not sure that
> your app really requires JavaScript support.
> 
> More info: http://developer.android.com/guide/practices/security.html 
> 
> ---
> 
> Margaret, my assumption is that this call is necessary and we should
> suppress the linter warning.
> 
> However, have you had this audited? Are we certain there are no
> vulnerabilities here?
> 
> I'll post a patch if so.

For the record, we use this to load a style script to hide certain contents on the search results pages:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/search/java/org/mozilla/search/providers/SearchEngine.java#49

That "greasemonkey terror script" comment is a bit snarky, we should probably update it with something more useful :)

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

That injection seems very fragile – were any alternatives considered (e.g. talking to the providers directly to get a version of the page with those tags)?

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://hg.mozilla.org/integration/fx-team/rev/12ab70b36685

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/12ab70b36685

Comment 7

[:Margaret Leibovic]

(In reply to Michael Comella (:mcomella) from comment #4)
> That injection seems very fragile – were any alternatives considered (e.g.
> talking to the providers directly to get a version of the page with those
> tags)?

Yes, it's terrible. See bug 1059984.