8362268 : NPE thrown from SASL GSSAPI impl when TLS is used with QOP auth-int against Active Directory #26566
Conversation
…ed with QOP auth-int against Active Directory
…LS is used with QOP auth-int against Active Directory" This reverts commit ea2d289.
…ed with QOP auth-int against Active Directory
|
👋 Welcome back wxiao! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
|
Webrevs
|
|
The bug title says "Java 11+" but the affects version field also contains 8u401 - does it also affect 8u? In general, I would avoid putting release versions in the title of the bug as the affects version field is the right place to add that info, so please remove "on Java 11+" from the title. |
|
Since this fix in the security-libs area, I think the component and subcomponent should be changed to security-libs/javax.security. Also, please add a "noreg-hard" label to the bug with a comment explaining why it is too hard to write a regression test. |
weibxiao
changed the title
|
Original report in OpenJDK mail list mentioned it could not be duplicated in jdk8, but actually the defect exists also in JDK8. The failure only happened when Sasl.QOP set with the value of auth-int or auth-conf. It needs a ldap server with the setting of SASL authentication. It is not available in OpenJDK community. |
|
I am the reporter of this bug in the mailing list. @seanjmullan, yes it is also present in JDK 8. confirmed myself on HP-UX where the JDK is provided by HPE. They have either cherry-picked the faulty commit or used an already updated tree. @weibxiao I do not fully understand this fix. It does not really fix the issue, does it? It converts one NPE into another. From my PoV the regression should be reverted and another, better fix should be employed. As @wangweij writes here, let LDAP complete the |
|
My "here" was
However, I'm not sure if this correct. This means the security guaranteed by the SASL layer is lost and I also don't know if the peer can parse it correctly. @michael-o What have JDK 8 and |
|
I can not revert the previous change. It will close the unused sockets in JVM before next GC to clean them. Once simple fix in application cod for this NPE is increasing buffer size by setting javax.security.sasl.maxbuffer in the context to overwrite the hard coded buffer size in AbstractSaslImpl.java. This NPE is more or less related to the timing. The context was cleared earlier than output stream got flush, but the later code is actually running earlier, but completed later. |
I will get back to you tomorrow or Monday as soon as I have access to the environment. |
I see.
No, that will not solve the problem here at all. Active Directory does not support auth-int/-conf on a TLS wrapped connection. It sends a message to notify the client. The client (Java JNDI) does not know this and reads the first bytes assuming the SASL buffer size, but it is a non-wrapped message. I can provide pcaps, if you like.
Yes, but how will this change solve the problem? Do you intend to add another PR to address it? |
JDK 8 is here: Create a TLS connection to Active Directory, bind with SASL GSSAPI and
libldap does properly signal in the invalid buffer size and shows the I think I have described the issue quite well in https://mail.openjdk.org/pipermail/security-dev/2025-April/045574.html. pcap and keylog file are available for inspection. The opposite side does not send an |
|
So, it seems we should NOT revert to the raw stream. We can either return earlier in |
I agree that if the opposite side did close the connection without properly advertising it and we try to send a request and it fails, it should be clearly signalled to the user. |
Were you able to check it meanwhile? |
|
Not quite sure what you like me to check. I confirmed the context was disposed. With my testing code, I got same error as what you got. Certainly there is no more NPE. |
Good. I am fine with the PR. |
|
/label add core-libs |
|
@seanjmullan |
|
Since the fix is in the |
| } finally { | ||
|
|
||
| flushAndCloseOutputStream(); | ||
| // 8313657 socket is not closed until GC is run |
There was a problem hiding this comment.
Would it be worth keeping a comment here about why sock is not closed, or at least mentioning 8362268?
| } | ||
| } | ||
|
|
||
| private static class CustomSocket extends Socket { |
There was a problem hiding this comment.
minor: could you please cleanup unused imports after this change?
|
|
||
| private static class CustomSocket extends Socket { | ||
| private int closeMethodCalled = 0; | ||
| private LdapOutputStream output = new LdapOutputStream(); |
There was a problem hiding this comment.
I believe these are local objects which are no longer used after removal of this. I'd personally remove the private static class LdapInputStream extends InputStream { and private static class LdapOutputStream extends OutputStream { further down the file
| private void closeOpenedResource() { | ||
| try { | ||
| if (conn != null) { | ||
| if (conn.outStream != null) { | ||
| conn.outStream.close(); | ||
| } | ||
|
|
||
| if (conn.sock != null && !conn.sock.isClosed()) { | ||
| conn.sock.close(); | ||
| } | ||
| } | ||
| } catch (IOException ioEx) { | ||
| //ignore the error; | ||
| } |
There was a problem hiding this comment.
[OK - I missed that the cleanup method had been modified to no longer close the socket]
But another issue is that this method attempts to modify the state of the connection without holding the connection lock. This is not good.
There was a problem hiding this comment.
One possibility could be to move this code to the connection so that it can participate in the locking.
However - I'm concerned that this proposed fix will reintroduced https://bugs.openjdk.org/browse/JDK-8313657
dfuch
left a comment
dfuch
left a comment
There was a problem hiding this comment.
The proposed solution needs more explaining, and integrate properly with the connection lock.
| } | ||
|
|
||
| // flush and close output stream | ||
| private void flushAndCloseOutputStream() { |
There was a problem hiding this comment.
If this method no longer closes the output stream it should no longer be called flushAndCloseOutputStream()
| private void closeOpenedResource() { | ||
| try { | ||
| if (conn != null) { | ||
| if (conn.outStream != null) { | ||
| conn.outStream.close(); | ||
| } | ||
|
|
||
| if (conn.sock != null && !conn.sock.isClosed()) { | ||
| conn.sock.close(); | ||
| } | ||
| } | ||
| } catch (IOException ioEx) { | ||
| //ignore the error; | ||
| } |
There was a problem hiding this comment.
One possibility could be to move this code to the connection so that it can participate in the locking.
However - I'm concerned that this proposed fix will reintroduced https://bugs.openjdk.org/browse/JDK-8313657
|
Updated the code to address the review comments. In JDK, at the line , the buffer length was larger than default value 65536, it would trigger the exception. At the line of 451 of Connnection.java, the exception was caught. Connection::cleanup got called. At line , the method of closeOpenedSocket will close the output stream. For Sasl implementation, this output stream is SaslOutputStream.java. So that SaslOutputStream::close is used to close the stream. Inside this method, GssKrb5Client::dispose was called, its implementation is base class GssKrb5Base::dispose, which is disposing the GSS Context and set it to be null. At mean time, Sasl output stream was till in the middle to handle the buffer by using GssKrb5Base::wrap. Since Gss Context already set to be null, it caused the NPE at line ofJNDI connection was created inside the constructor o LdapClient.java. When Connection object was created, the output stream and input stream was created also. For Sasl implementation, it was SaslnputStream and SaslOutputStream. Both resources were created inside LdapClient when Connection object was created. It seems ok to close them in LdapClient.java. Consider locking the connection, the change was called inside LdapClient::close, which was using ReentrantLock to control the access. Hope to get more info to refine the code here. |
dfuch
left a comment
dfuch
left a comment
There was a problem hiding this comment.
I am not quite sure I understand how this fix works. If all tests are passing then it may be OK. I hope it isn't going to re-introduce a resource leak though. Synchronization/locking must be fixed however, and I have suggseted some changes below that will ensure it integrates correctly with the locking strategy in the Connection class.
It would be good to get @AlekseiEfimov review.
| conn.cleanup(reqCtls, false); | ||
| closeOpenedResource(); |
There was a problem hiding this comment.
Please replace these two lines with:
conn.cleanupAndClose(reqCtls);
Then add a method in Connection:
void cleanupAndClose(Control[] reqCtls) {
lock.lock();
try {
cleanup(reqCtls, false);
// 8313657 socket is not closed until GC is run
// it caused the bug 8362268, hence moved here
if (outStream != null) {
outStream.close();
}
if (!sock.isClosed()) {
sock.close();
}
} catch (IOException ignored) {
// we're closing, ignore IO.
} finally {
lock.unlock();
}
}
| conn.cleanup(null, false); | ||
| closeOpenedResource(); |
There was a problem hiding this comment.
same here. replace these two lines with:
conn.cleanupAndClose(null);
| } | ||
| } catch (IOException ioEx) { | ||
| //ignore the error; | ||
| } |
There was a problem hiding this comment.
Please revert this change. Use conn.cleanupAnadClose(...) instead.
|
Updated the code accordingly. |
7 participants
webrev.zip
NPE thrown from SASL GSSAPI impl when TLS is used with QOP auth-int against Active Directory.
When the exception is triggered, LDAP Connection will do "clean-up" operation and output stream get flushed and closed the context while GssKrb5Client is still wrapping the message, and tried to send the abandoned info to the client at line https://github.com/openjdk/jdk/blob/master/src/jdk.security.jgss/share/classes/com/sun/security/sasl/gsskerb/GssKrb5Base.java#L140. That's the reason to throw NPE.
The change is going to close socket and output stream in LdapClient.java. It would allow SASL client code to send the abandoned request to client; then dispose GSS context. This will avoid NPE to thrown at line 140 of GssKrb5Base.java.
No test file is attached for this MR since it needs Sasl LDAP server with security setup. Attached the updated webrev for the reference.
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/26566/head:pull/26566$ git checkout pull/26566Update a local copy of the PR:
$ git checkout pull/26566$ git pull https://git.openjdk.org/jdk.git pull/26566/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 26566View PR using the GUI difftool:
$ git pr show -t 26566Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26566.diff
Using Webrev
Link to Webrev Comment