RFR: 8293232: Fix race condition in pkcs11 SessionManager

[zzambers]

On Tue, 6 Sep 2022 21:02:50 GMT, Valerie Peng <valeriep at openjdk.org> wrote:

>> There is a race condition in JDK's SessionManager, which can lead to random exceptions.
>> 
>> **Exception:**
>> 
>> javax.net.ssl.SSLException: Internal error: close session with active objects
>> 	at java.base/sun.security.ssl.Alert.createSSLException(Alert.java:133)
>> 	at java.base/sun.security.ssl.TransportContext.fatal(TransportContext.java:371)
>> 	at java.base/sun.security.ssl.TransportContext.fatal(TransportContext.java:314)
>> 	at java.base/sun.security.ssl.TransportContext.fatal(TransportContext.java:309)
>> 	at java.base/sun.security.ssl.SSLSocketImpl.handleException(SSLSocketImpl.java:1707)
>> 	at java.base/sun.security.ssl.SSLSocketImpl$AppInputStream.read(SSLSocketImpl.java:1080)
>> 	at java.base/sun.security.ssl.SSLSocketImpl$AppInputStream.read(SSLSocketImpl.java:971)
>> 	at SSLSocketServer.serverLoop(SSLSocketServer.java:133)
>> 	at SSLSocketServer$1.run(SSLSocketServer.java:75)
>> 	at java.base/java.lang.Thread.run(Thread.java:833)
>> Caused by: java.security.ProviderException: Internal error: close session with active objects
>> 	at jdk.crypto.cryptoki/sun.security.pkcs11.Session.close(Session.java:127)
>> 	at jdk.crypto.cryptoki/sun.security.pkcs11.Session.close(Session.java:114)
>> 	at jdk.crypto.cryptoki/sun.security.pkcs11.SessionManager.closeSession(SessionManager.java:237)
>> 	at jdk.crypto.cryptoki/sun.security.pkcs11.SessionManager$Pool.release(SessionManager.java:270)
>> 	at jdk.crypto.cryptoki/sun.security.pkcs11.SessionManager.demoteObjSession(SessionManager.java:210)
>> 	at jdk.crypto.cryptoki/sun.security.pkcs11.Session.removeObject(Session.java:101)
>> 	at jdk.crypto.cryptoki/sun.security.pkcs11.SessionKeyRef.updateNativeKey(P11Key.java:1396)
>> 	at jdk.crypto.cryptoki/sun.security.pkcs11.SessionKeyRef.removeNativeKey(P11Key.java:1377)
>> 	at jdk.crypto.cryptoki/sun.security.pkcs11.NativeKeyHolder.releaseKeyID(P11Key.java:1329)
>> 	at jdk.crypto.cryptoki/sun.security.pkcs11.P11Key.releaseKeyID(P11Key.java:156)
>> 	at jdk.crypto.cryptoki/sun.security.pkcs11.P11AEADCipher.reset(P11AEADCipher.java:529)
>> 	at jdk.crypto.cryptoki/sun.security.pkcs11.P11AEADCipher.ensureInitialized(P11AEADCipher.java:436)
>> 	at jdk.crypto.cryptoki/sun.security.pkcs11.P11AEADCipher.implDoFinal(P11AEADCipher.java:732)
>> 	at jdk.crypto.cryptoki/sun.security.pkcs11.P11AEADCipher.engineDoFinal(P11AEADCipher.java:624)
>> 	at java.base/javax.crypto.Cipher.doFinal(Cipher.java:2500)
>> 	at java.base/sun.security.ssl.SSLCipher$T12GcmReadCipherGenerator$GcmReadCipher.decrypt(SSLCipher.java:1659)
>> 	at java.base/sun.security.ssl.SSLSocketInputRecord.decodeInputRecord(SSLSocketInputRecord.java:260)
>> 	at java.base/sun.security.ssl.SSLSocketInputRecord.decode(SSLSocketInputRecord.java:181)
>> 	at java.base/sun.security.ssl.SSLTransport.decode(SSLTransport.java:111)
>> 	at java.base/sun.security.ssl.SSLSocketImpl.decode(SSLSocketImpl.java:1508)
>> 	at java.base/sun.security.ssl.SSLSocketImpl.readApplicationRecord(SSLSocketImpl.java:1479)
>> 	at java.base/sun.security.ssl.SSLSocketImpl$AppInputStream.read(SSLSocketImpl.java:1064)
>> 	... 4 more
>> 
>> 
>> **Reproducibility:**
>> I started getting this exception quite reliably on JDK17 on my machine with one particular test setup using ssl-tests testsuite. Unfortunately setup itself needed some RH specific patches and also ability to reproduce depends on other factors such number keys in keystore, machine where testing was performed... I tried a bit to create some reproducer, but I couldn't find a way to easily reproduce this issue :(
>> 
>> **Problem:**
>> SunPKCS11 provider does session pooling. This is done in SessionManager [1] (one per SunPKCS11 provider). Released sessions are kept by SessionManager for a while, for reuse (in limited number). This however is a bit complicated as some sessions can own objects (e.g. keys). So there are actually 2 pools. One for sessions with objects ("objSessions") and one for sessions without objects ("opSessions"). This is because sessions without objects, which are not being used, can be safely closed (SessionManager only keeps around limited amount of these), while sessions with objects cannot be safely closed (until all objects are removed from them). Session manager has methods for getting Session for given purpose (object creation or just doing other operations), prioritizing appropriate pool. Each session has counter (called "createdObjects") to track how many objects it owns. When session is being returned to pool this counter is checked and session is placed to appropriate pool. Also wh
 en counter for some Session in "objSessions" pool reaches zero it is moved ("demoted") to "opSessions" pool.
>> 
>> And here comes complicated part. As far as I understand it, Session.addObject() [2] (which increases "createdObjects" counter) is always being called by thread "holding" session which owns the created object. (That is: thread gets a session, uses it to create an object and calls Session.addObject() on that session to increase the counter, before returning the session to pool. See e.g.: [3]) However this is not true for Session.removeObject() [4]. (That is: thread gets session, which is not necessary the same one owning object being removed, performs object removal, but then calls Session.removeObject() on session which owned that object. See e.g.: [5]) That is Session.removeObject() can be called on Session which is in "objSessions" pool or which is being used be other thread. (object removal can happen as result of releasing key, either explicitly or as result of GC etc..). 
>> 
>> And finally, there is a problem in code handling object removal from a session. Session.removeObject() [4] first checks if "createdObjects" counter reached zero. If so, it calls SessionManager.demoteObjSession(this) [6], which attempts to remove Session from objSessions pool, if session is successfully removed from there, meaning no other thread "holds" this session, session is put to opSessions pool, if not (meaning other thread "holds" it), method just returns, since that other thread puts this session to appropriate pool, when it is done with it by calling SessionManager.releaseSession(session).
>> 
>> There is race condition here. Consider following scenario:
>> 
>> // Thread T1 runs:
>> Session.removeObject() // [4]
>> createdObjects.decrementAndGet() // returns zero
>> 
>> // Thread T2 steps in (operating on the same session instance):
>> Session.addObject() // increases "createdObjects" counter [2]
>> SessionManager.releaseSession(session) // releases session to objSessions pool
>> 
>> // Thread T1 continues:
>> SessionManager.demoteObjSession(this) // [6]
>> objSessions.remove(session) // returns true
>> opSessions.release(session)  // puts session (with objects!) to opSessions pool
>> // if opSessions is already full, close of session with objects is attempted throwing Exception..
>> 
>> 
>> **Fix:**
>> SessionManager.demoteObjSession [6] method was changed, so that check for objects is done once again if session was successfully removed from "objSessions" pool (now that it is out of pool and other threads should not be adding objects to it). Based on this check session is either released to "opSessions" pool or returned to "objSessions" pool. This can be achieved by calling releaseSession(session) instead of opSessions.release(session).
>> 
>> **Testing:**
>> jdk_security tests passed for me locally with this change.
>> I have also tested this change on top of custom JDK17 build which allows scenario, where I can reproduce this issue. Problem got fixed.
>> 
>> [1] https://github.com/openjdk/jdk/blob/9444a081cc9873caa7b5c6a78df0d1aecda6e4f1/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SessionManager.java
>> [2] https://github.com/openjdk/jdk/blob/9444a081cc9873caa7b5c6a78df0d1aecda6e4f1/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/Session.java#L93
>> [3] https://github.com/openjdk/jdk/blob/9444a081cc9873caa7b5c6a78df0d1aecda6e4f1/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Key.java#L1339
>> [4] https://github.com/openjdk/jdk/blob/9444a081cc9873caa7b5c6a78df0d1aecda6e4f1/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/Session.java#L98
>> [5] https://github.com/openjdk/jdk/blob/9444a081cc9873caa7b5c6a78df0d1aecda6e4f1/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Key.java#L1360
>> [6] https://github.com/openjdk/jdk/blob/9444a081cc9873caa7b5c6a78df0d1aecda6e4f1/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SessionManager.java#L195
>
> src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SessionManager.java line 210:
> 
>> 208:             return;
>> 209:         }
>> 210:         releaseSession(session);
> 
> With the described race condition, have you tried fixing it by adding a if-condition check before doing line 204-210, i.e. if (!session.hasObjects()) { .... }?

I am afraid putting check before line 204 would not solve the issue (just lowered it's likelihood). Problem is, that operation consisting of check for objects on a session and then removing it from objSessions pool is not atomic. Session still could be obtained from objSessions pool by other thread after session.hasObjects() was called, object added to it and released back to objSessions pool before objSessions.remove(session) is called. I think this check for objects can only be trusted after session was successfully removed from objSessions (that is, session was in objSessions pool (no tread "holds" it) and was removed).

Actually whole call of demoteObjSession method is already behind one check for zero objects (but that check cannot be trusted), and needs to be redone after objSessions.remove(session),  because of problem described higher . See:
https://github.com/openjdk/jdk/blob/9444a081cc9873caa7b5c6a78df0d1aecda6e4f1/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/Session.java#L100

-------------

PR: https://git.openjdk.org/jdk/pull/10125