[RFC][icedtea-web] PR742: Fix checking multiple levels of JAR certs for trust

[Danesh Dadachanji]

On 11/08/11 12:23 PM, Deepak Bhole wrote:
> * Danesh Dadachanji<ddadacha at redhat.com>  [2011-08-11 12:20]:
>>
>>
>>
>> On 10/08/11 06:01 PM, Deepak Bhole wrote:
>>> * Danesh Dadachanji<ddadacha at redhat.com>   [2011-08-10 17:20]:
>>>> Hello,
>>>>
>>>> Here's an update for this bug. It took so long because of the (at
>>>> the time) mysterious PR771. This will now check all the certificates
>>>> along the certPath for trust in the store. It also displays a new
>>>> icon[1] and automatically selects the "Always Trust" checkbox when
>>>> an applet is verified. Along the way I found a miscalculation in the
>>>> window size of the dialog. It was too small to display the entire
>>>> icon so I increased the height.
>>>>
>>>> I've tested it on all of the certificate holding JNLPs on the test
>>>> wiki page.
>>>>
>>>> The original reporter's applet is signed by an older version of a
>>>> Thawte CA which I was unable to find online. The newer version is
>>>> technically considered a different certificate (public keys are
>>>> different) so this patch still won't verify their applet.
>>>>
>>>
>>> Hi Danesh,
>>>
>>> Patch looks good. Couple of minor things:
>>>
>>
>> Hi Deepak,
>>
>> Thanks for the review!
>>
>> I pulled the icon stuff out and into another patch because it isn't
>> related to PR742. I will email a patch in a few moments. The subject
>> will be: Update UI for SecurityDialog
>>
>
> Hi Danesh,
>
> Looks good! The changelog lines seem to start with a space instead of a
> tab. I assume this is just an error from pasting though. Please
> make sure they start with tabs as the other others.
>
> After that, ok for HEAD.

Yep, it was a copy paste error.

Pushed to HEAD, thanks for the review!

Danesh

>
> Cheers,
> Deepak
>
>>> When a cert has been verified and we show a prompt with the new question
>>> icon, the title still says "Warning - Security" ... we should probably
>>> update this to something that doesn't look as alarming ... maybe
>>> "Security approval needed" or something?
>>>
>>> Also, one code related fix below:
>>>
>>>> -                rootInCacerts = CertificateUtils.inKeyStores(root, caKeyStores);
>>>> +                // Check entire cert path for a trusted CA
>>>> +                List<? extends Certificate>   certList = certPath.getCertificates();
>>>> +                for (int i = 0; i != certList.size(); i++) {
>>>> +                	if ((rootInCacerts = CertificateUtils.inKeyStores(
>>>> +                            (X509Certificate) certList.get(i), caKeyStores))) {
>>>> +                        break;
>>>> +                    }
>>>> +                }
>>>
>>> It would be cleaner to use the Java foreach syntax with
>>> certPath.getCertificates() directly... e.g.:
>>>
>>> for (Certificate c: certPath.getCertificates()) {
>>> 	...
>>> }
>>
>> I changed it to this =)
>>
>> ChangeLog
>> +2011-08-11  Danesh Dadachanji<ddadacha at redhat.com>
>> +
>> + PR742: IcedTea-Web checks certs only upto 1 level deep before declaring
>> + them untrusted.
>> + * NEWS: Updated.
>> + * netx/net/sourceforge/jnlp/tools/JarSigner.java:
>> + (checkTrustedCerts): All certs along certPath are now checked for trust.
>> +
>>
>>>
>>> Cheers,
>>> Deepak
>>
>
>> diff -r 27f08d58854f NEWS
>> --- a/NEWS	Tue Aug 09 17:34:35 2011 -0400
>> +++ b/NEWS	Thu Aug 11 12:16:08 2011 -0400
>> @@ -19,6 +19,7 @@ New in release 1.2 (2011-XX-XX):
>>   Common
>>     - PR768: Signed applets/Web Start apps don't work with OpenJDK7 and up
>>     - PR771: IcedTea-Web certificate verification code does not use the right API
>> +  - PR742: IcedTea-Web checks certs only upto 1 level deep before declaring them untrusted.
>>
>>   New in release 1.1 (2011-XX-XX):
>>   * Security updates
>> diff -r 27f08d58854f netx/net/sourceforge/jnlp/tools/JarSigner.java
>> --- a/netx/net/sourceforge/jnlp/tools/JarSigner.java	Tue Aug 09 17:34:35 2011 -0400
>> +++ b/netx/net/sourceforge/jnlp/tools/JarSigner.java	Thu Aug 11 12:16:08 2011 -0400
>> @@ -373,7 +373,13 @@ public class JarSigner implements CertVe
>>                   alreadyTrustPublisher = CertificateUtils.inKeyStores(publisher, certKeyStores);
>>                   X509Certificate root = (X509Certificate) getRoot();
>>                   KeyStore[] caKeyStores = KeyStores.getCAKeyStores();
>> -                rootInCacerts = CertificateUtils.inKeyStores(root, caKeyStores);
>> +                // Check entire cert path for a trusted CA
>> +                for (Certificate c : certPath.getCertificates()) {
>> +                	if ((rootInCacerts = CertificateUtils.inKeyStores(
>> +                            (X509Certificate) c, caKeyStores))) {
>> +                        break;
>> +                    }
>> +                }
>>               } catch (Exception e) {
>>                   // TODO: Warn user about not being able to
>>                   // look through their cacerts/trusted.certs
>