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

[Deepak Bhole]

* 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.

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