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

[Jiri Vanek]

-------- Original Message --------
Subject: Re: [RFC][icedtea-web] -Xtrustall opinion added
Date: Tue, 28 Jun 2011 11:39:41 +0200
From: Jiri Vanek <jvanek at redhat.com>
To: Omair Majid <omajid at redhat.com>
CC: IcedTea Distro List <distro-pkg-dev at openjdk.java.net>

On 06/27/2011 04:46 PM, Omair Majid wrote:
> 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

Yes - it trust certificate, when there is no certificate or nothing to trust, then it have no affect to code.
>
>> 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.
true
>
>>
>>
>> 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 @@
>> }
>>
ˇvˇvˇvˇvˇvˇvˇvˇvˇvˇvˇvˇvˇvˇvˇvˇvˇvˇvˇvˇvˇvˇvˇvˇvˇvˇ
>> 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"
thanx. fixed
>
>> + 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?
Sure. Tahnx.

>
>> }
>> 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?
Yes, you are right, but You probably have missed part in claslaoder - part between V and ^  - When user should be asked (shouldPromptUser - used only in SecurityDialogs class) , then it acts as KEY_SECURITY_PROMPT_USER and user is not asked.
But certificates verification (trustall:) ) is entirely skipped with trustAll flag true. I believe (but don't KNOW!) that untrusted application can not be loaded (so it can not set trustall to true) .

On the other way, when signed jar is loaded, then it can set this option to true (even by reflection) and then any  unsigned jar will pass inside.... (????????????????? XXX ???????????)

There is rising an question how to deal with diferent styles of signing (eg doubled jnlp by Saad).. Even handling this signatures in "my amazing" framework;)

>
>> + }else{
>> return Boolean.valueOf(JNLPRuntime.getConfiguration()
>> .getProperty(DeploymentConfiguration.KEY_SECURITY_PROMPT_USER));
>> + }
>
> Please fix the indentation.
>
>> }
>> });
>> }
>
> Cheers,
> Omair

ping