[rfc][icedtea-web] missing jnlpfile and securtydelegate for TemporaryPermissionsButton calls
Jiri Vanek
jvanek at redhat.com
Fri Sep 12 11:30:31 UTC 2014
On 09/04/2014 10:46 PM, Andrew Azores wrote:
> On 03/09/14 08:45 AM, Jiri Vanek wrote:
>> On 08/28/2014 12:33 AM, Andrew Azores wrote:
>>> Hi,
>>>
>>> Still on PTO here and preparing to move back into my parents' place while I go back to finish
>>> school, so I haven't really invested any real time into this bug. But...
>>>
>>> On 08/27/2014 05:25 AM, Jiri Vanek wrote:
>>>> Hi!
>>>>
>>>> During testing of https probing, helpcrypto found and interesting issue
>>>> in head.
>>>>
>>>> Under some circumstances calls to TemporaryPermissionsButton have
>>>> jnlpfile and securitydelegate null.
>>>
>>> The SecurityDelegate can be instantiated much earlier in JNLPClassLoader if need be. It can even be
>>> the first thing done after calling the super constructor AFAIK. Since the JNLPClassLoader is also
>>> what eventually causes the security dialog to appear with the Temporary Permissions button on it,
>>> this will probably resolve the null securityDelegate reference part.
>>>
>>>>
>>>> Two things troubles me:
>>>> - both those values are needed only when "tmeporarypermissions" are
>>>> clicked, so they should not affect startup of ITW
>>>> - how come that they are null? I blame
>>>> http://icedtea.classpath.org/hg/icedtea-web/annotate/d87ee4e6e81a/netx/net/sourceforge/jnlp/security/dialogs/TemporaryPermissionsButton.java#78
>>>>
>>>>
>>>>
>>>> (looking into Looking to this,
>>>> http://icedtea.classpath.org/hg/icedtea-web/file/b4363c984e1b/netx/net/sourceforge/jnlp/security/dialogs/TemporaryPermissionsButton.java#l79
>>>>
>>>>
>>>> )
>>>
>>> For the null jnlpFile - I can't think off the top of my head what is causing that. It could just be
>>> again that the dialog is being shown before a field has been assigned a value, as it is with the
>>> securityDelegate. If it's simply an ordering issue like this then that isn't *too* concerning. If we
>>> truly just do not have a reference to any JNLPFile or PluginBridge for this applet then there's
>>> something seriously wrong.
>>>
>>>>
>>>>
>>>> Atatched is the nasty workarorund and here is snippet of log
>>>>
>>>> Securitymanager=net.sourceforge.jnlp.runtime.JNLPSecurityManager at 19f17b1
>>>> Registering priority for string reference 2
>>>> Registering priority for reference 2
>>>> Returning value:0
>>>> Setting value:0
>>>> Setting value:null
>>>> dummy string null, null, javax.swing.JButton[,0,0,0x0,inva...snip...
>>>> plugin_in_pipe_callback return
>>>> plugin_send_message_to_appletviewer return
>>>> PIPE: plugin wrote(?): plugin PluginProxyInfo reference 1 DIRECT
>>>> plugin_send_message_to_appletviewer
>>>> Proxy info: plugin PluginProxyInfo reference 1 DIRECT
>>>>
>>>>
>>>>
>>>> Without the workarround itw dies:
>>>>
>>>> ITNP_SetWindow return
>>>> ITNP_SetWindow: window already exists.
>>>> ITNP_SetWindow
>>>> at java.lang.Thread.run(Thread.java:745)
>>>> at
>>>> net.sourceforge.jnlp.security.SecurityDialogMessageHandler.run(SecurityDialogMessageHandler.java:81)
>>>>
>>>>
>>>> at
>>>> net.sourceforge.jnlp.security.SecurityDialogMessageHandler.handleMessage(SecurityDialogMessageHandler.java:102)
>>>>
>>>>
>>>>
>>>> at net.sourceforge.jnlp.security.SecurityDialog.
>>>> (SecurityDialog.java:129)
>>>> at
>>>> net.sourceforge.jnlp.security.SecurityDialog.initDialog(SecurityDialog.java:255)
>>>>
>>>> at
>>>> net.sourceforge.jnlp.security.SecurityDialog.installPanel(SecurityDialog.java:307)
>>>>
>>>> at net.sourceforge.jnlp.security.dialogs.CertWarningPane.
>>>> (CertWarningPane.java:116)
>>>> at
>>>> net.sourceforge.jnlp.security.dialogs.CertWarningPane.addComponents(CertWarningPane.java:124)
>>>>
>>>> at
>>>> net.sourceforge.jnlp.security.dialogs.CertWarningPane.addButtons(CertWarningPane.java:242)
>>>>
>>>> at
>>>> net.sourceforge.jnlp.security.dialogs.TemporaryPermissionsButton.
>>>> (TemporaryPermissionsButton.java:79)
>>>> at java.util.Objects.requireNonNull(Objects.java:201)
>>>> Exception in thread "NetxSecurityThread" java.lang.NullPointerException
>>>> plugin_in_pipe_callback return
>>>> plugin_send_message_to_appletviewer return
>>>> PIPE: plugin wrote(?): plugin PluginProxyInfo reference 1 DIRECT
>>>> plugin_send_message_to_appletviewer
>>>> Proxy info: plugin PluginProxyInfo reference 1 DIRECT
>>>>
>>>>
>>>>
>>>> I think the AssertNotNUll are really really invasive here. But at least
>>>> they cought probably more serious issue - to early call to the dialogue
>>>> with tmppermissions button....
>>>>
>>>>
>>>> J.
>>>
>>> Yup. The PolicyEditor can't be sensibly launched if the jnlpFile is null (your nasty workaround
>>> really is nasty - assigning the new permissions to all applets rather than only ones on the current
>>> applet's codebase!), and the securityDelegate is needed if this button is to function at all. So
>>> there's no sensible thing to do if either of those fields come up null. Perhaps rather than outright
>>> failing to launch ITW we could have the button indicate a failure somehow and have the dialog simply
>>> not install the button, however. Just another option for Lukasz or whomever to look into. If this
>>> still hasn't been fixed by the time I come off PTO then I'll gladly take ownership of the bug.
>>>
>>
>> Andrew have made some investigations, and this is moreover conclusion
>> - its a bug - the tmp permissions dialogue is shared among javaws and applets, and in applets
>> lifecycle can happen this NPE
>> - however, the fix will take some time
>
> Eh, not necessarily :) see below.
>
>>
>> Even after it may be some cases, when this NPE will rise. We agreed that disabling the button n
>> this case is probably best solution.
>>
>>
>> The attache dpatch do so,
>>
>> Two nits :
>> - is really the SecurityDelegate suddenly useless?
>
> Yes - this NPE bug only occurs when the button is being installed by a CertWarningPane invoked by
> the VariableX509TrustManager. In other words, it only occurs when the dialog is asking the user
> whether they trust the certificate presented for the current HTTPS connection, not when the dialog
> is asking the user if they trust a certificate used to sign an applet. IMO it doesn't make sense to
> decide to apply sandboxing rules to an applet at this point, based on trusting the HTTPS
> certificate, although maybe that is something to explore. If we do want this to be an option then
> the VariableX509TrustManager will have to have some work done so it can carry the extra state of the
> JNLPFile and SecurityDelegate corresponding to the current applet/application as well. This would
> take some extra time, but if we don't want to add allowing sandboxing based on the HTTPS certificate
> trust, then your patch or mine are basically sufficient.
>
>> - Why all apps reproducing this issue suddenly started to work? ( I mean without this patch)
>
> Do they? I haven't tried to verify this but that's surprising to me. The only validation currently
> done by the CertWarningPane is checking if the JNLPFile is null (which is a pretty fragile check -
> it's been changed to check the type of the CertVerifier instead), and if it is, then the button is
> not installed into the button panel. This decision is made well after the TemporaryPermissionsButton
> has been instantiated, which means that the Objects.requireNonNull checks have been performed. And
> the VariableX509TrustManager is still hard-coded to specifically provide "null" for the "file" and
> "securityDelegate" parameters...
>
>>
>>
>> Still I think it is good thing to get rid of assers and disable button.
>
> I think it's better to just not show the button (buttonS really, it's the Sandbox as well as
> Temporary Permissions) in this context, but it's an easy change to make either way.
>
>>
>>
>> J.
>>
>>
>
> Attached is my take on the patch. I also corrected the buttons on the dialog so that their labels
> and tooltips make more sense in the context of trusting an HTTPS certificate rather than an applet
> certificate. This looks a little messy with all the if statements added, but maybe it's not worth
> abstracting this class and making two subclasses just to change the labels on the buttons (and
> ignore two buttons from the button panel). Up to you. I can refactor it into two classes no problem
> but I know you expressed distaste for this approach on IRC ;)
>
> Thanks,
>
Ok for head, with only nit...
Please log in debug mode:
+ if (file == null || securityDelegate == null || linkedButton == null) {
+ this.setEnabled(false);
log here which are suddenly null.
+ } else {
Go on and push hten.
J.
More information about the distro-pkg-dev
mailing list