[icedtea-web] RFC: check for nulls in JNLPPolicy.isSystemJar

[Deepak Bhole]

* Omair Majid <omajid at redhat.com> [2011-03-01 17:58]:
> On 03/01/2011 05:41 PM, Deepak Bhole wrote:
> >* Deepak Bhole<dbhole at redhat.com>  [2011-03-01 17:23]:
> >>* Omair Majid<omajid at redhat.com>  [2011-03-01 17:06]:
> >>>On 03/01/2011 04:48 PM, Deepak Bhole wrote:
> >>>>* Omair Majid<omajid at redhat.com>   [2011-03-01 16:41]:
> >>>>>Hi,
> >>>>>
> >>>>>The attached patch adds a null check in JNLPPolicy.isSystemJar. It's
> >>>>>needed as PluginAppletSecurityContext.getAccessControlContext
> >>>>>creates CodeSources with null locations.
> >>>>>
> >>>>
> >>>>Why is this needed? The function is only called from getPermissions()
> >>>>which does a check on source prior to calling:
> >>>>
> >>>>         if (source.equals(systemSource) || source.equals(shellSource))
> >>>>             return getAllPermissions();
> >>>>
> >>>
> >>>Actually, the code is more like this:
> >>>
> >>>     public PermissionCollection getPermissions(CodeSource source) {
> >>>         if (source.equals(systemSource) || source.equals(shellSource))
> >>>             return getAllPermissions();
> >>>
> >>>         if (isSystemJar(source)) {
> >>>             return getAllPermissions();
> >>>         }
> >>>
> >>
> >>Yep, I know. Which means source is (in some way) looked at before
> >>calling isSystemJar.
> >>
> >>>If source is null (or, more importantly, if source.location is null)
> >>>isSystemJar may still be called. Without this patch, isSystemJar
> >>>will throw a NullPointerException instead of returning false.
> >>>
> >>
> >>Well source can't be null else .equals will throw an NPE. As for
> >>source.getLocation.. systemSource is derived as:
> >>
> >>systemSource = Policy.class.getProtectionDomain().getCodeSource()
> >>
> >>which will have location == null
> >>
> >>So in theory there should never be a case where isSystemJar is called
> >>will null source or where source.getLocation is null.
> >>
> >
> >Sorry, scratch that. Just came to mind that location won't be null for
> >systemSource, source will be null. So what is an example where location
> >is null?
> >
> 
> One reproducer is at http://www.javatester.org/version.html. Wait
> for the applet to load and then reload the page. A debugger gives me
> a stack trace like this:
> 
> Thread [Thread-22] (Suspended (exception NullPointerException))	
> 	JNLPPolicy.isSystemJar(CodeSource) line: 143	
> 	JNLPPolicy.getPermissions(CodeSource) line: 78	
> 	AppletPanel$10.run() line: 1071	
> 	AccessController.doPrivileged(PrivilegedAction<T>) line: not
> available [native method] [local variables unavailable]	
> 	NetxPanel(AppletPanel).getAccessControlContext(URL) line: 1066	
> 	NetxPanel(AppletPanel).getClassLoader(URL, String) line: 1021	
> 	NetxPanel(AppletPanel).sendEvent(int) line: 302	
> 	PluginAppletViewer.appletShutdown(AppletPanel) line: 1628	
> 	PluginAppletViewer.access$200(PluginAppletViewer, AppletPanel) line: 301	
> 	PluginAppletViewer$5.run() line: 1651	
> 	Thread.run() line: 679	
> 
> The culprit line is:
> return p.getPermissions(new CodeSource(null,
> (java.security.cert.Certificate[]) null));
> 
> 
> I also see something similar in
> plugin/icedteanp/java/sun/applet/PluginAppletSecurityContext.java
> CodeSource cs = new CodeSource((URL) null,
> (java.security.cert.Certificate[]) null);
> But I am not sure how to trigger it. I just wanted to point out that
> there are cases where null location is possible. If the policy is
> ever consulted on this CodeSource, the code blows up.
> 
> Hope that makes things clear.
> 

Ah I see. Thanks for the clarification. Okay for HEAD.

Cheers,
Deepak

> Cheers,
> Omair
> 
> >>>>>ChangeLog:
> >>>>>2011-03-01  Omair Majid<omajid at redhat.com>
> >>>>>
> >>>>>     * netx/net/sourceforge/jnlp/runtime/JNLPPolicy.java (isSystemJar):
> >>>>>     Check for nulls.
> >>>>>
> >>>>>Okay to commit?
> >>>>>
> >>>>>Cheers,
> >>>>>Omair
> >>>>
> >>>>>diff -r c4b91b61f88e netx/net/sourceforge/jnlp/runtime/JNLPPolicy.java
> >>>>>--- a/netx/net/sourceforge/jnlp/runtime/JNLPPolicy.java	Mon Feb 28 17:29:31 2011 -0500
> >>>>>+++ b/netx/net/sourceforge/jnlp/runtime/JNLPPolicy.java	Tue Mar 01 16:31:13 2011 -0500
> >>>>>@@ -137,6 +137,10 @@
> >>>>>       * it's part of the JRE.
> >>>>>       */
> >>>>>      private boolean isSystemJar(CodeSource source) {
> >>>>>+        if (source == null || source.getLocation() == null) {
> >>>>>+            return false;
> >>>>>+        }
> >>>>>+
> >>>>>          // anything in JRE/lib/ext is a system jar and has full permissions
> >>>>>          String sourceProtocol = source.getLocation().getProtocol();
> >>>>>          String sourcePath = source.getLocation().getPath();
> >>>>
> >>>
>