[rfc][icedtea-web] missing jnlpfile and securtydelegate for TemporaryPermissionsButton calls
Andrew Azores
aazores at redhat.com
Sat Sep 13 17:24:46 UTC 2014
On 09/12/2014 07:30 AM, Jiri Vanek wrote:
> 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.
Done. Wasn't there a bugzilla for this? I didn't find it to put the bug
ID in the commit message or at least go comment on the report.
Thanks,
--
Andrew Azores
More information about the distro-pkg-dev
mailing list