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

[Omair Majid]

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.

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();
>>>>
>>>