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

[Omair Majid]

On 11/11/2010 11:10 AM, Deepak Bhole wrote:
> * Omair Majid<omajid at redhat.com>  [2010-11-11 11:07]:
>> On 11/11/2010 10:42 AM, Deepak Bhole wrote:
>>> * Omair Majid<omajid at redhat.com>   [2010-11-11 10:39]:
>>>> On 11/11/2010 10:02 AM, Deepak Bhole wrote:
>>>>> * 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.
>>>>>
>>>>
>>>> That's true. On further consideration, I dont think any methods in
>>>> CertificateUtils need security checks though. They all operate on
>>>> generic Certificate or KeyStore objects. There is nothing here that
>>>> is netx specific or that an untrusted application could not do by
>>>> itself. The actual action of obtaining the netx-specific KeyStores
>>>> to operate on is the responsibility of KeyStores (see comment below
>>>> for more about that). Do you think I should add checks anyway?
>>>>
>>>
>>> Ah okay. Nope, in that case no security checks are needed for those
>>> functions!
>>>
>>>>>>>> +    /**
>>>>>>>> +     * 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..
>>>>>
>>>>
>>>> Yes, that's a good point. Thanks for pointing it out. I have added
>>>> an AllPermission check in getKeyStore(Level,Type,boolean) which
>>>> almost all public methods end up calling to access the KeyStores.
>>>> The only public methods that dont call this check are
>>>> getKeyStoreLocation (which is supposed to query the configuration
>>>> object) and toTranslatableString and toDisplayableString (which are
>>>> translation methods). Do you see any problems with this scheme?
>>>>
>>>
>>> Translation methods don't need a check. But getKeyStoreLocation... can
>>> that me made to return a location in user.home/* ? If so, it provides
>>> access to user.home indirectly and needs a check.
>>>
>>
>> getKeyStoreLocation is essentially a wrapper around
>> JNLPRuntime.getConfiguration().getProperty(String). The only thing
>> it does is find the appropriate String argument to use with
>> getProperty(String). As such, I think it is OK in this one case to
>> trust security to DeploymentConfiguration. But if you want, I can
>> add a check here anyway.
>>
>
> Nope, not necessary in that case. Just wanted to make sure user.home was
> protected.
>
>
> Okay I have no other issues with the patch. Ok for HEAD.
>

Thanks for the extensive reviews! I have pushed the change.

Cheers,
Omair