[icedtea-web] RFC: integrate support for multiple KeyStores into the various validators

[Deepak Bhole]

* Omair Majid <omajid at redhat.com> [2010-11-11 09:53]:
> On 11/11/2010 08:42 AM, Deepak Bhole wrote:
> 
...
> I based this code off code in KeyTool, and yes, it is slightly
> confusing. A KeyStore can be considered a HashMap: it maps a String,
> the alias, to a Certificate. The code first checks if the
> certificate already exists by trying to find an alias for the given
> certificate. If an alias is found, we know the certificate is in the
> KeyStore already. If the certificate is not in the KeyStore, we want
> to add a new entry to the HashMap, but we do not want to overwrite
> an existing certificate. So we create random aliases until we get an
> alias which has no mapping in the HashMap.
> 


Ah okay then.

> >Also, those methods should not be public static as untrusted apps could
> >access them. The keystores shouldn't be either (if they are, either now
> >or from before).
> >
> 
> I dont think this will be a problem. For one, every time it is
> called, it reads the configuration (which untrusted apps are not
> allowed to do) and then it reads the keystore from the disk (which
> untrusted apps are not allowed to do). Making it non-public would
> mean it can not be used by classes in other packages (for example,
> JarSigner is in net.sourceforge.jnlp.tools and it uses this class).
> Please let me know if this is not enough; I will see what else I can
> do.
> 

Reliance on other methods to do security is bad imo. There is no
guarantee that the design will stay the same, If it changes and another
way to do those functions is implemented, security may be compromised.
It is fine to keep them public for other class usage, but please add an
AllPermission check in that case.

> >>+    /**
> >>+     * Checks whether an X509Certificate is already in one of the keystores
> >>+     * @param c the certificate
> >>+     * @param keyStores the KeyStores to check in
> >>+     * @return true if the certificate is present in one of the keystores, false otherwise
> >>+     */
> >>+    public static final boolean inKeyStores(X509Certificate c, KeyStore[] keyStores) {
> >>+        for (int i = 0; i<  keyStores.length; i++) {
...
> >
> >
> >What does printRfc represent? And if it's always true, why the else?
> >
> 
> This code was originally from net.sourceforge.jnlp.tools.KeyTool and
> was moved over to CertificateUtils. n.s.j.t.KeyTool was adapted from
> sun.security.tools.KeyTool which allows dumping certificate in more
> than one format. printRfc indicates that the certifcate should be
> dumped in the RFC 4945 format
> (http://tools.ietf.org/html/rfc4945#section-6.1). Since this is the
> only option we support, I have removed that if condition; it is
> fixed in the updated patch.
> 

Great!

...
...
> >
> >Should something like this be public static? Untrusted apps will be able
> >to access it and modify it...
> >
> 
> I dont think this is a problem. getClientKeyStores() calls
> getKeyStore() which reads the configuration (which untrusted apps
> are not allowed to do) and then reads the acutal keystore from disk
> (which untrusted apps are again not allowed to do). So I dont think
> untrusted apps can actually get access to these keystores. If
> untrusted apps call getClientKeyStores (or the related methods
> getCAKeyStores or getCertKeyStores), they should get a
> SecurityException from DeploymentConfiguration.
> 
> A KeyStore is an in-memory version of a keystore file on disk. Even
> if untrusted apps were somehow able to access it, saving the
> contents to disk would require permissions to write to disk.
> 

Same as with above. An AllPermission check in that case..

> >>+    /**
> >>       * Returns the location of a KeyStore corresponding to the given level and type.
> >>       * @param level
> >>       * @param type
> >>@@ -334,4 +357,5 @@
> >>          return ks;
> >>      }
> >>
> >>+
> >>  }
> >Rest looks fine to me.
> >
> 
> Oh, and just to clarify something which might not have been obvious
> from the patch - in general, netx does not cache KeyStores. When it
> needs to make a security decision related to certificates, it will
> create appropriate KeyStore objects, do whatever operation it needs
> to and then discard the KeyStore. This is not true for
> VariableX509TrustManager, but it is true for JarSigner, Certificate
> Viewer and the various security prompts.
> 

That is fine. Normally I am all for optimizing by reducing disk access,
but in this case I think the decision is valid. Other apps may update
the keystore while one is running, in which case the running one should
have the latest information (especially if another app deleted a cert).
Perhaps VariableX509 should be changed to do the same?

Also, are the trust store files locked when write happens? If not, they
should be.

Cheers,
Deepak