[RFC[PATCH]: Updated Patch for validating signedJNLP file at launch

[Omair Majid]

On 08/15/2011 12:04 PM, Saad Mohammad wrote:
>>> + /** True if the application has a signed JNLP File */
>>> + private boolean isSignedJNLP = false;
>>> +
>>> + /** True if the application is signed but is missing a signed JNLP
>>> File (warning should be shown); otherwise false */
>>> + private static boolean signedJNLPWarning= false;
>>> +
>>
>> Why is it static?
>
> This is a static variable because showSignedJNLPWarning() is a static
> method that returns this variable. This is used to determine if a signed
> JNLP warning should be shown in the 'More Information' panel.
>

I dont think this is a good idea. Because the variable is static, we 
cant determine which classloader this corresponds to. If multiple 
classloaders are instantiated (which is the case for applets and 
extension jnlps) then this may give us the wrong answer.

>>
>> Also, is this behaviour (if the jar is signed but does not have a
>> sigend jnlp file, then a warning is shown) documented somewhere? I ask
>> because I find it rather strange. AFAIK, a lot of applications fit
>> this pattern. I am not sure if showing warnings for all of these makes
>> sense.
>
> Unfortunately, I did not find any documentations that state anything
> about displaying a warning, but it's the way Oracle behave. I assume
> IcedTea-web should behave similar?
>
> I have also noticed that Oracle only display the warning if the jar file
> is not verified. If it's a verified jar, it does not show any warning
> regarding a signed JNLP. I have not made that change with IcedTea-Web, I
> was debating whether it should show this warning every time or not. I
> think I will implement that on my updated patch.
>

Hm.. I tested Oracle's javaws with SweetHome3D and I got the warning 
dialog, but I dont see any signed JNLP warnings with JPathReport:

http://www.jgoodies.com/download/jpathreport/jpathreport.jnlp

There might be a more specific condition to showing the warning than 
just a missing signed jnlp file.

>>> + private void checkForMain(List<JARDesc> jars) throws LaunchException {
>>>
>>> - activateJars(initialJars);
>>> + ApplicationDesc ad = (ApplicationDesc) file.getLaunchInfo();
>>> + String mainClass = ad.getMainClass();
>>

Hmm... I just realized that this may not work well at all. What if 
file.getLaunchInfo() returns an AppletDesc?

>>> +
>>> + public static boolean showSignedJNLPWarning()
>>> + {
>>> + return signedJNLPWarning;
>>> }
>>
>> Why is this static? Also, the name can be a bit misleading. Invoking a
>> method named <verb>Foo will normally take the action implied. This
>> method does not show the warning.
>
> This is a static method because MoreInfroPane calls this method without
> having a JNLPClassLoader object to determine if a signed jnlp warning
> should be displayed.
>

Okay, but that wont work properly in the presence of multiple 
JNLPClassLoaders. Please make this non-static. Better yet, see how other 
security problems are handled (so they can be shown in security warnings 
properly) and make this mirror that.

>>
>> Is there a way you can avoid this dependency? This really should not
>> be tied to the classloader.
>
> Perhaps, I can create another class that holds the static variable that
> can be used by MoreInfoPane?
> I will have to look into this one. I will get back to you.
>

Please avoid using static variables here - even if they wrapped in 
another class. They wont work with multiple instances of JNLPClassLoader 
active.

>>> + //Show signed JNLP warning if the signed main jar does not have a
>>> signed JNLP file
>>> + if(JNLPClassLoader.showSignedJNLPWarning())
>>> + details.add(R("SJNLPFileIsNotSigned"));
>>> +
>>
>> This doesn't look right at all. This dialog is only shown if there is
>> a security problem in the first place. I am not sure if the user
>> should be shown this warning, but this will make it so that the user
>> _only_ sees it if there is a bigger security concern.
>>
>
> The way Oracle is showing their warning message is only if the jar does
> not have a verified signature. If the jar is verified and is part of the
> trusted certificates, the warning is not shown (even if a signed JNLP
> file is not found). I have not implemented this part, but I will send it
> in the next patch.
>
> I don't think it should show this warning for any other dialog. I think
> it should only show it when it's with an unverified jar.
> What do you think?
>

As I said above, there is probably a more specific condition to showing 
the warning than a missing the signed jnlp file. Please try and see if 
you can figure it out.

Once you have that, you will need to show the security warning. I would 
try and integrate this into the way the rest of the security information 
is passed from JNLPClassLoader to CertWarningPane and MoreInfoPane (and 
any other appropriate places).

>>> int numLabels = details.size();
>>> JPanel errorPanel = new JPanel(new GridLayout(numLabels, 1));
>>> errorPanel.setBorder(BorderFactory.createEmptyBorder(10, 10, 10, 10));
>>> @@ -88,6 +94,10 @@
>>>
>>> errorPanel.add(new JLabel(htmlWrap(details.get(i)), icon,
>>> SwingConstants.LEFT));
>>> }
>>> +
>>> + //Remove signed JNLP warning
>>> + if(JNLPClassLoader.showSignedJNLPWarning())
>>> + details.remove(details.size()-1);
>>>
>>
>> Are you removing the warning unconditionally?
>
> I am removing this warning after it has been added to the 'More
> Information' panel. When I add to details, it also adds it to certVerifier.
> That's why I am removing it because I don't want it to be part of
> certVerifier permanently.
>

Oh. I see. A comment explaining this would have been useful :) However, 
I think it might be a better idea to integrate this into 
JarSigner/CertVerifier.

Cheers,
Omair