Code Review Request JDK-8129988 JSSE should create a single instance of the cacerts KeyStore
Xuelei Fan
xuelei.fan at oracle.com
Thu Jan 5 01:17:57 UTC 2017
New webrev:
http://cr.openjdk.java.net/~xuelei/8129988/webrev.04/
On 1/4/2017 1:27 PM, Sean Mullan wrote:
> It's unfortunate that the previous code read the system properties each
> time a TrustManagerFactory was initialized. I think in practice, if
> applications are changing these system properties on the fly it isn't
> going to work predictably. This implementation would have been much
> simpler if the properties were read once, and then you could just check
> the lastModified time.
>
> But, I suppose we should err on the side of caution and maintain that
> behavior.
>
I don't like the old behavior, which is error-prone. But for safe we
may want to keep the behavior unchanged at this stage.
> A few comments:
>
> 160 // Not break, the file is inaccessbile.
>
> Typo: inaccessible
>
> 191 Objects.equals(this.storePassword,
> that.storePassword));
>
> Let's avoid this. I don't think it's possible for an app to leverage
> this to try to guess the password, but let's err on the side of caution.
> If the password has changed, then the file's lastModified time would
> need to be updated anyway, so you don't really need to check this.
>
Good. Updated.
> 326 this.ksRef = new WeakReference<>(ks);
>
> ks is a local variable that goes out of scope when this method is
> finished, so I don't think this will work as you expect.
>
Good catch! I made an update that the local key store reference will
not reserved any more.
Thanks,
Xuelei
> --Sean
>
>
> On 1/4/17 1:38 PM, Xuelei Fan wrote:
>> Updated to use synchronized method. Most of the time, the synchronized
>> only perform the read access to the variables. The impact on performance
>> should be acceptable.
>>
>> http://cr.openjdk.java.net/~xuelei/8129988/webrev.03/
>>
>> Only the TrustStoreManager.java implementation get updated in this
>> webrev.
>>
>> Thanks,
>> Xuelei
>>
>> On 12/23/2016 7:52 AM, Sean Mullan wrote:
>>> On 12/22/16 2:52 PM, Xuelei Fan wrote:
>>>> updated: http://cr.openjdk.java.net/~xuelei/8129988/webrev.02/
>>>
>>> I think there are still some race conditions. For example:
>>>
>>> 264 TrustStoreDescriptor temporaryDesc = this.descriptor;
>>> 265 KeyStore cachedKeyStore = ksRef.get();
>>> 266 if (descriptor.equals(temporaryDesc) && (cachedKeyStore
>>> != null)) {
>>> 267 return cachedKeyStore;
>>> 268 }
>>>
>>> There is no locking here.
>>>
>>> Maybe that's ok based on your explanation below, but it seems a bit
>>> fragile and could lead to problems that are hard to debug. Have you
>>> looked at the AtomicReference class? You could define a new class
>>> containing the descriptor, keystore, and certs and wrap that in an
>>> AtomicReference and then use the methods on that class to update it.
>>> Might be worth exploring that a bit more.
>>>
>>> --Sean
>>>
>>>
>>>> On 12/22/2016 9:32 AM, Sean Mullan wrote:
>>>>> On 12/20/16 3:21 PM, Xuelei Fan wrote:
>>>>>
>>>>>>> 213 if (storePassword != null &&
>>>>>>> !storePassword.isEmpty()) {
>>>>>>> 214 MessageDigest md =
>>>>>>> JsseJce.getMessageDigest("SHA-256");
>>>>>>> 215 result = 31 * result +
>>>>>>> 216 Arrays.hashCode(md.digest(storePassword.getBytes()));
>>>>>>> 217 }
>>>>>>>
>>>>>>> Why are you hashing the password here? Are you afraid this could be
>>>>>>> leaked or guessed somehow?
>>>>>> Yes. The hash code of the password part can be computed. I was
>>>>>> wondering the String.hashCode() may not have sufficient strength.
>>>>>>
>>>>>>> I would just leave the password out of the
>>>>>>> hashcode and equals. It doesn't matter, it's still the same file,
>>>>>>> right?
>>>>>>> I'm not sure if the type or provider matter either. Don't you just
>>>>>>> care
>>>>>>> about the name of the file and the modification time?
>>>>>>>
>>>>>> For file type key store, the file and the modification time should be
>>>>>> sufficient. But for non-file (PKCS11) key store, the provider and
>>>>>> password may be sensible. The basic idea is that, if one of the
>>>>>> system
>>>>>> property get updated, the key store should be reloaded. Checking
>>>>>> every
>>>>>> property update makes the code more straightforward.
>>>>>
>>>>> But the main focus of this performance issue is for the cacerts file,
>>>>> which is not PKCS11. So I would not use the password and other
>>>>> non-relevant or security-sensitive attributes. A hash of the password
>>>>> isn't sufficient against dictionary-type attacks, for example.
>>>>>
>>>> I see your point. The password hash code block is removed.
>>>>
>>>>>>> 268 if ((temporaryDesc != null) &&
>>>>>>>
>>>>>>> Why would a null descriptor ever be ok? Shouldn't you just let this
>>>>>>> throw NPE? Same comment on line 301.
>>>>>>>
>>>>>> The temporaryDesc is initialized as null. A singleton service
>>>>>> (TrustStoreManage.tam) is used and lazy loaded. Null means the
>>>>>> descriptor has not been assigned.
>>>>>
>>>>> I think there are thread-safeness issues in the TrustStoreManager
>>>>> class.
>>>>> You are not synchronizing when you read so looks like there can be
>>>>> various race conditions. For example, this.descriptor and this.ksRef
>>>>> can
>>>>> be updated by another thread in the middle of this code
>>>>>
>>>>> 267 TrustStoreDescriptor temporaryDesc = this.descriptor;
>>>>> 268 if ((temporaryDesc != null) &&
>>>>> 269 temporaryDesc.equals(descriptor)) {
>>>>> 270 KeyStore ks = ksRef.get();
>>>>> 271 if (ks != null) {
>>>>> 272 return ks;
>>>>> 273 }
>>>>> 274 }
>>>>>
>>>>> Maybe that doesn't really matter, but I'm not sure -- have you thought
>>>>> about it?
>>>>>
>>>> I thought about the issue. But I really missed to the double check
>>>> idiom. Updated.
>>>>
>>>> For performance consideration, I'm trying to mitigate the impact of
>>>> synchronization. Once the key store get loaded, there is a strong
>>>> reference, and it can be used safely. If another thread is trying to
>>>> modify the descriptor and key store, this thread will use the existing
>>>> key store, and another thread can use the new key store. If two
>>>> threads
>>>> try to modify the key store for the same descriptor, I added the double
>>>> check idiom so that the first thread will complete the update and the
>>>> 2nd thread will use the 1st thread updated key store. If two threads
>>>> try to modify the key store for different descriptor, each will get a
>>>> different key store and the 2nd thread will reset the final key store
>>>> for future use.
>>>>
>>>> In general, applications would not modify the system properties. So
>>>> the
>>>> use of the synchronized block should be very rare. It benefits the
>>>> performance in multiple threading computation environment.
>>>>
>>>> Xuelei
>>>>
>>>>> --Sean
More information about the security-dev
mailing list