[RFC][IcedTea-Web]: Change #4 (PersistenceService) of new JNLP specification (v7.0)

[Omair Majid]

Hey,

On 06/08/2011 03:16 PM, Saad Mohammad wrote:
> This is the patch that allows any trusted application to have access to
> any PersistenceService data (including ones from other hosts). It is
> part of the new JNLP specification, and is stated under change #4
> (http://jcp.org/aboutJava/communityprocess/maintenance/jsr056/jnlp-7_0-changes.html).
>
>
> This patch is very simple, it uses a custom method [a modified version
> of ServiceUtil.checkAccess()] to validate whether the current
> application is trusted and has a signature. If the current application
> is  a trusted application, XPersistenceService.checkLocation() makes
> sure that the application has access to all Persistence Service data. If
> the current application is not a trusted application and it want to have
> access to PersistenceService data from an outside host,
> XPersistenceService.checkLocation() will throw MalformedURLException.
>

Thanks for contributing to icedtea-web! It's nice to support this. 
Overall, I am fine with the patch. But I do have a few specific 
concerns, noted below.

>
> diff -r af1ed34483d1 netx/net/sourceforge/jnlp/services/XPersistenceService.java
> --- a/netx/net/sourceforge/jnlp/services/XPersistenceService.java	Tue May 31 12:00:01 2011 -0400
> +++ b/netx/net/sourceforge/jnlp/services/XPersistenceService.java	Wed Jun 08 13:54:41 2011 -0400
> @@ -53,8 +53,11 @@
>
>           URL source = app.getJNLPFile().getCodeBase();
>
> -        if (!source.getHost().equalsIgnoreCase(location.getHost()))
> -            throw new MalformedURLException("Cannot access data from a different host.");
> +        if (!source.getHost().equalsIgnoreCase(location.getHost())){
> +        	if (!checkSigned(app)){ // If the application is not trusted, throw exception. Allow trusted application to access data from a different host
> +        		throw new MalformedURLException("Untrusted application cannot access data from a different host.");
> +        	}
> +        }
>

Please try and keep your lines to within a reasonable limit (we dont 
enforce 80-char wide lines, but something around that is good).

>           // test for above codebase, not perfect but works for now
>
> @@ -69,8 +72,11 @@
>               System.out.println("request path: " + requestPath);
>           }
>
> -        if (!source.getFile().startsWith(requestPath))
> -            throw new MalformedURLException("Cannot access data below source URL path.");
> +        if (!source.getFile().startsWith(requestPath)){
> +        	if (!checkSigned(app)){ // If the application is not trusted, throw exception. Allow trusted application to access data from a different host
> +        		throw new MalformedURLException("Untrusted application cannot access data below source URL path.");
> +        	}
> +        }
>       }
>
>       /**
> @@ -177,5 +183,65 @@
>
>           // todo: actually implement tags
>       }
> +
> +    /**
> +	 * This method is a modification to the method ServiceUtil.checkAccess
> +	 * which checks if the current application has a signature and is a trusted application
> +	 * @author<a href="mailto:smohammad at redhat.com">Saad Mohammad</a>
> +	 *
> +	 * @param app the application which is requesting the check. If null, the
> +	 *            current application is used.
> +	 * @return a boolean is returned after checking if the current application
> +	 *         is trusted

The "@return ..." parts are transformed into "returns ....". And the 
sentence should read properly after that substitution. Please take a 
look at:
http://download.oracle.com/javase/6/docs/api/java/lang/Object.html#equals%28java.lang.Object%29

> +	 */
> +	public static boolean checkSigned(ApplicationInstance app) {
>

By convention, we normally use isFoo or hasFoo for methods that return a 
boolean. Can you please rename this method to isSigned()?

> +		if (app == null)
> +			app = JNLPRuntime.getApplication();
> +		if (app == null)
> +			throw new NullPointerException(
> +					"ApplicationInstance is null and cannot determine the current application.");
> +
> +		boolean codeTrusted = true;
> +
> +		StackTraceElement[] stack = Thread.currentThread().getStackTrace();
> +
> +		for (int i = 0; i<  stack.length; i++) {
> +
> +			Class<?>  c = null;
> +
> +			try {
> +				c = Class.forName(stack[i].getClassName());
> +			} catch (Exception e1) {
> +				try {
> +					c = Class.forName(stack[i].getClassName(), false,
> +							app.getClassLoader());
> +				} catch (Exception e2) {
> +					System.err.println(e2.getMessage());
> +				}
> +			}
> +
> +			// Everything up to the desired class/method must be trusted
> +			if (c == null || // class not found
> +					(c.getProtectionDomain().getCodeSource() != null&&  // class
> +																		// is
> +																		// not
> +																		// in
> +																		// bootclasspath
> +					c.getProtectionDomain().getCodeSource().getCodeSigners() == null) // class
> +																						// is
> +																						// trusted
> +			) {
> +				System.out.println("Code is untrusted " + i);
> +				codeTrusted = false;
> +			}
> +		}
> +
> +		if (!codeTrusted) {
> +
> +			return false;
> +		}
> +
> +		return true; // allow
> +	}
>   }

If this is mostly the same code as ServiceUtil.checkAccess, then I am 
against copying it. Duplicating code makes future maintenance harder. 
Please extract the relevant bits into a new method (and move it to an 
appropriate class). And then have ServiceUtil.checkAccess and 
XPersistenceService call that instead.

Cheers,
Omair