[rfc][icedtea-web] missing jnlpfile and securtydelegate for TemporaryPermissionsButton calls
Andrew Azores
aazores at redhat.com
Thu Sep 4 20:46:15 UTC 2014
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,
--
Andrew A
-------------- next part --------------
A non-text attachment was scrubbed...
Name: temporarypermissionsbutton-npe-fix.patch
Type: text/x-patch
Size: 6120 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20140904/46c138a3/temporarypermissionsbutton-npe-fix.patch>
More information about the distro-pkg-dev
mailing list