[RFC][icedtea-web] -Xtrustall opinion added

[Omair Majid]

On 06/26/2011 02:04 PM, Jiri Vanek wrote:
> Hi!
> Attached patch is adding (especially for testing purposes) -Xtrustall
> parameter for javaws. When javaws is lunched with this parameter, then
> user is not asked for any security issues, and any necessary steps he
> had to accept are now trusted.
> I believe it is implemented correctly, because running code which need
> to be signed on signed, but without permissions or unsigned will still
> fail.

Sorry, I dont understand this bit. Xtrustall will just trust all 
certificates right? If there are none, there is nothing to trust and so 
things will

> It is worthy to say, that there already existed
> DeploymentConfiguration.KEY_SECURITY_PROMPT_USER key for deploy
> configuration, but this one just hiding all security dialogs to user,
> but thy were then evaluated as untrusted.

That's working as designed. If the user says he doesn't want to see any 
prompts, then we shouldn't be accepting the security prompts. That would 
lead to malicious programs doing whatever they wanted.

>
>
> diff -r 86abbf8be0b1 netx/net/sourceforge/jnlp/runtime/Boot.java
> --- a/netx/net/sourceforge/jnlp/runtime/Boot.java	Thu Jun 23 15:29:45 2011 +0200
> +++ b/netx/net/sourceforge/jnlp/runtime/Boot.java	Sun Jun 26 18:47:30 2011 +0200
> @@ -156,6 +156,9 @@
>           if (null != getOption("-Xnofork")) {
>               JNLPRuntime.setForksAllowed(false);
>           }
> +        if (null != getOption("-Xtrustall")) {
> +            JNLPRuntime.setTrustAll(true);
> +        }
>
>           JNLPRuntime.setInitialArgments(Arrays.asList(argsIn));
>
> diff -r 86abbf8be0b1 netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
> --- a/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java	Thu Jun 23 15:29:45 2011 +0200
> +++ b/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java	Sun Jun 26 18:47:30 2011 +0200
> @@ -519,6 +519,9 @@
>       }
>
>       private void checkTrustWithUser(JarSigner js) throws LaunchException {
> +        if (JNLPRuntime.isTrustAll()){
> +            return;
> +        }
>           if (!js.getRootInCacerts()) { //root cert is not in cacerts
>               boolean b = SecurityDialogs.showCertWarningDialog(
>                       AccessType.UNVERIFIED, file, js);
> diff -r 86abbf8be0b1 netx/net/sourceforge/jnlp/runtime/JNLPRuntime.java
> --- a/netx/net/sourceforge/jnlp/runtime/JNLPRuntime.java	Thu Jun 23 15:29:45 2011 +0200
> +++ b/netx/net/sourceforge/jnlp/runtime/JNLPRuntime.java	Sun Jun 26 18:47:30 2011 +0200
> @@ -121,6 +121,9 @@
>       /** set to false to indicate another JVM should not be spawned, even if necessary */
>       private static boolean forksAllowed = true;
>
> +    /** all security dialogs will be consumed and pretented as beeing verified by user and allowed.*/

typo here: "being"

> +    private static boolean trustAll=false;
> +
>       /** contains the arguments passed to the jnlp runtime */
>       private static List<String>  initialArguments;
>
> @@ -130,6 +133,7 @@
>       public static final String STDERR_FILE = "java.stderr";
>       public static final String STDOUT_FILE = "java.stdout";
>
> +
>       /**
>        * Returns whether the JNLP runtime environment has been
>        * initialized.  Once initialized, some properties such as the
> @@ -728,4 +732,12 @@
>           }
>       }
>
> +    static void setTrustAll(boolean b) {
> +        trustAll=b;
> +    }
> +
> +    public static boolean isTrustAll() {
> +        return trustAll;
> +    }
> +

Could you please add a javadoc comment to these two methods noting that 
they are for testing only?

>   }
> diff -r 86abbf8be0b1 netx/net/sourceforge/jnlp/security/SecurityDialogs.java
> --- a/netx/net/sourceforge/jnlp/security/SecurityDialogs.java	Thu Jun 23 15:29:45 2011 +0200
> +++ b/netx/net/sourceforge/jnlp/security/SecurityDialogs.java	Sun Jun 26 18:47:30 2011 +0200
> @@ -358,8 +358,12 @@
>           return AccessController.doPrivileged(new PrivilegedAction<Boolean>() {
>               @Override
>               public Boolean run() {
> +                if (JNLPRuntime.isTrustAll()) {
> +                    return false;

I am a little confused here. Every place that is calling this method 
(shouldPromptUser) will assume the security check failed and will not 
continue an action - with a name like trustAll, dont we want the 
security check to succeed? Would you mind explaining why this change is 
needed?

> +                }else{
>                   return Boolean.valueOf(JNLPRuntime.getConfiguration()
>                           .getProperty(DeploymentConfiguration.KEY_SECURITY_PROMPT_USER));
> +                }

Please fix the indentation.

>               }
>           });
>       }

Cheers,
Omair