From 649aee187b397d72dcfcb1dabdb3e9cca9fc5637 Mon Sep 17 00:00:00 2001 From: Brandon White Date: Mon, 29 Jan 2018 11:56:36 -0800 Subject: [PATCH] Ensure disposal of native auth state after executing Http Method. Depending on the selected authorization scheme, it's possible for some native OS handles to leak once the HttpMethodDirector has finished executing an HttpMethod. Leaks have been observed for Kerberos (via SPNEGO - Negotiate) but also seem possible using NTLM authentication. Examples of potentially leaked handles include: * credentials handles (e.g. missing call to FreeCredentialsHandle) * security context (e.g. missing call to DeleteSecurityContext) The HttpMethodDirector now ensures that the selected authorization scheme has a chance to cleanup outstanding native OS state once the HttpMethod has finished execution. --- .../core/httpclient/HttpMethodDirector.java | 13 ++++++++ .../tfs/core/httpclient/auth/AuthScheme.java | 5 ++++ .../tfs/core/httpclient/auth/AuthState.java | 10 ++++++- .../tfs/core/httpclient/auth/BasicScheme.java | 5 ++++ .../httpclient/auth/CookieAuthScheme.java | 5 ++++ .../core/httpclient/auth/DigestScheme.java | 5 ++++ .../core/httpclient/auth/JwtAuthScheme.java | 5 ++++ .../tfs/core/httpclient/auth/NTLMScheme.java | 16 ++++++---- .../core/httpclient/auth/NegotiateScheme.java | 30 ++++++++----------- 9 files changed, 71 insertions(+), 23 deletions(-) diff --git a/source/com.microsoft.tfs.core.httpclient/src/com/microsoft/tfs/core/httpclient/HttpMethodDirector.java b/source/com.microsoft.tfs.core.httpclient/src/com/microsoft/tfs/core/httpclient/HttpMethodDirector.java index 9fbbbce53..fad3ed810 100644 --- a/source/com.microsoft.tfs.core.httpclient/src/com/microsoft/tfs/core/httpclient/HttpMethodDirector.java +++ b/source/com.microsoft.tfs.core.httpclient/src/com/microsoft/tfs/core/httpclient/HttpMethodDirector.java @@ -224,6 +224,8 @@ public void executeMethod(final HttpMethod method) throws IOException, HttpExcep } // end of retry loop } finally { + cleanupHostAndProxyAuthState(method); + if (conn != null) { conn.setLocked(false); } @@ -237,6 +239,17 @@ public void executeMethod(final HttpMethod method) throws IOException, HttpExcep } } } + + private void cleanupHostAndProxyAuthState(final HttpMethod method) { + cleanupAuthState(method.getHostAuthState()); + cleanupAuthState(method.getProxyAuthState()); + } + + private void cleanupAuthState(AuthState authState) { + if (authState != null) { + authState.invalidate(); + } + } private Credentials getPreemptiveCredentials(final String host, final int port) { if (host == null) { diff --git a/source/com.microsoft.tfs.core.httpclient/src/com/microsoft/tfs/core/httpclient/auth/AuthScheme.java b/source/com.microsoft.tfs.core.httpclient/src/com/microsoft/tfs/core/httpclient/auth/AuthScheme.java index 52752fad2..24a5100ba 100644 --- a/source/com.microsoft.tfs.core.httpclient/src/com/microsoft/tfs/core/httpclient/auth/AuthScheme.java +++ b/source/com.microsoft.tfs.core.httpclient/src/com/microsoft/tfs/core/httpclient/auth/AuthScheme.java @@ -79,6 +79,11 @@ public interface AuthScheme { */ public boolean supportsCredentials(Credentials credentials); + /** + * Perform optional cleanup of auth scheme state (e.g. dispose of native OS handles, etc..). + */ + public void cleanup(); + /** * Processes the given challenge token. Some authentication schemes may * involve multiple challenge-response exchanges. Such schemes must be able diff --git a/source/com.microsoft.tfs.core.httpclient/src/com/microsoft/tfs/core/httpclient/auth/AuthState.java b/source/com.microsoft.tfs.core.httpclient/src/com/microsoft/tfs/core/httpclient/auth/AuthState.java index 48ad6ddf3..cb1283006 100644 --- a/source/com.microsoft.tfs.core.httpclient/src/com/microsoft/tfs/core/httpclient/auth/AuthState.java +++ b/source/com.microsoft.tfs.core.httpclient/src/com/microsoft/tfs/core/httpclient/auth/AuthState.java @@ -76,7 +76,11 @@ public AuthState() { * Invalidates the authentication state by resetting its parameters. */ public void invalidate() { - authScheme = null; + if (authScheme != null) { + authScheme.cleanup(); + authScheme = null; + } + authRequested = false; authAttempted = false; preemptive = false; @@ -193,6 +197,10 @@ public void setAuthScheme(final AuthScheme authScheme) { preemptive = false; authAttempted = false; } + + if (this.authScheme != null) { + this.authScheme.cleanup(); + } this.authScheme = authScheme; } diff --git a/source/com.microsoft.tfs.core.httpclient/src/com/microsoft/tfs/core/httpclient/auth/BasicScheme.java b/source/com.microsoft.tfs.core.httpclient/src/com/microsoft/tfs/core/httpclient/auth/BasicScheme.java index 302234c71..358acfe86 100644 --- a/source/com.microsoft.tfs.core.httpclient/src/com/microsoft/tfs/core/httpclient/auth/BasicScheme.java +++ b/source/com.microsoft.tfs.core.httpclient/src/com/microsoft/tfs/core/httpclient/auth/BasicScheme.java @@ -253,4 +253,9 @@ public static String authenticate(final UsernamePasswordCredentials credentials, + EncodingUtil.getAsciiString(Base64.encodeBase64(EncodingUtil.getBytes(buffer.toString(), charset))); } + @Override + public void cleanup() { + + } + } diff --git a/source/com.microsoft.tfs.core.httpclient/src/com/microsoft/tfs/core/httpclient/auth/CookieAuthScheme.java b/source/com.microsoft.tfs.core.httpclient/src/com/microsoft/tfs/core/httpclient/auth/CookieAuthScheme.java index 4782425bf..7b352ff2e 100644 --- a/source/com.microsoft.tfs.core.httpclient/src/com/microsoft/tfs/core/httpclient/auth/CookieAuthScheme.java +++ b/source/com.microsoft.tfs.core.httpclient/src/com/microsoft/tfs/core/httpclient/auth/CookieAuthScheme.java @@ -14,6 +14,11 @@ public boolean supportsCredentials(final Credentials credentials) { return (credentials instanceof CookieCredentials); } + @Override + public void cleanup() { + //No disposing of credentials needed + } + @Override public void processChallenge(final String challenge) throws MalformedChallengeException { throw new MalformedChallengeException("Cookie authentication is not challenge/response"); //$NON-NLS-1$ diff --git a/source/com.microsoft.tfs.core.httpclient/src/com/microsoft/tfs/core/httpclient/auth/DigestScheme.java b/source/com.microsoft.tfs.core.httpclient/src/com/microsoft/tfs/core/httpclient/auth/DigestScheme.java index e10569ad8..2f6b8e9d7 100644 --- a/source/com.microsoft.tfs.core.httpclient/src/com/microsoft/tfs/core/httpclient/auth/DigestScheme.java +++ b/source/com.microsoft.tfs.core.httpclient/src/com/microsoft/tfs/core/httpclient/auth/DigestScheme.java @@ -609,4 +609,9 @@ public static String createCnonce() { return cnonce; } + + @Override + public void cleanup() { + + } } diff --git a/source/com.microsoft.tfs.core.httpclient/src/com/microsoft/tfs/core/httpclient/auth/JwtAuthScheme.java b/source/com.microsoft.tfs.core.httpclient/src/com/microsoft/tfs/core/httpclient/auth/JwtAuthScheme.java index 9e9a5f593..1aa34a8c1 100644 --- a/source/com.microsoft.tfs.core.httpclient/src/com/microsoft/tfs/core/httpclient/auth/JwtAuthScheme.java +++ b/source/com.microsoft.tfs.core.httpclient/src/com/microsoft/tfs/core/httpclient/auth/JwtAuthScheme.java @@ -21,6 +21,11 @@ public boolean supportsCredentials(final Credentials credentials) { return credentials instanceof JwtCredentials; } + @Override + public void cleanup() { + //No disposing of credentials needed + } + @Override public void processChallenge(final String challenge) throws MalformedChallengeException { complete = true; diff --git a/source/com.microsoft.tfs.core.httpclient/src/com/microsoft/tfs/core/httpclient/auth/NTLMScheme.java b/source/com.microsoft.tfs.core.httpclient/src/com/microsoft/tfs/core/httpclient/auth/NTLMScheme.java index 2449d0ccc..dd8159a2a 100644 --- a/source/com.microsoft.tfs.core.httpclient/src/com/microsoft/tfs/core/httpclient/auth/NTLMScheme.java +++ b/source/com.microsoft.tfs.core.httpclient/src/com/microsoft/tfs/core/httpclient/auth/NTLMScheme.java @@ -70,6 +70,16 @@ public boolean supportsCredentials(final Credentials credentials) { return supportsCredentials(credentials.getClass()); } + @Override + public void cleanup() { + if(ntlmClient != null) { + ntlmClient.dispose(); + ntlmClient = null; + } + + inputToken = null; + } + public static boolean supportsCredentials(final Class credentialClass) { if (credentialClass == null || !isSupported()) { return false; @@ -221,11 +231,7 @@ else if (status != STATUS_EXCHANGING || ntlmClient == null || inputToken == null if (ntlmClient.isComplete()) { status = STATUS_COMPLETE; - /* Clean up */ - ntlmClient.dispose(); - - ntlmClient = null; - inputToken = null; + cleanup(); } else { status = STATUS_EXCHANGING; } diff --git a/source/com.microsoft.tfs.core.httpclient/src/com/microsoft/tfs/core/httpclient/auth/NegotiateScheme.java b/source/com.microsoft.tfs.core.httpclient/src/com/microsoft/tfs/core/httpclient/auth/NegotiateScheme.java index 6947149a6..62e188fca 100644 --- a/source/com.microsoft.tfs.core.httpclient/src/com/microsoft/tfs/core/httpclient/auth/NegotiateScheme.java +++ b/source/com.microsoft.tfs.core.httpclient/src/com/microsoft/tfs/core/httpclient/auth/NegotiateScheme.java @@ -70,6 +70,16 @@ public boolean supportsCredentials(final Credentials credentials) { return supportsCredentials(credentials.getClass()); } + @Override + public void cleanup() { + if(negotiateClient != null) { + negotiateClient.dispose(); + negotiateClient = null; + } + + inputToken = null; + } + public static boolean supportsCredentials(final Class credentialClass) { if (credentialClass == null || !isSupported()) { return false; @@ -150,16 +160,7 @@ public void processChallenge(final String challenge) throws MalformedChallengeEx * We may be called for retry at an arbitrary time. If that's the * case, tear down any existing objects */ - if (negotiateClient != null) { - try { - negotiateClient.dispose(); - } catch (final Exception e) { - } - } - - negotiateClient = null; - inputToken = null; - + cleanup(); status = STATUS_INITIATED; } /* The server has responded with a challenge to our request */ @@ -193,7 +194,7 @@ protected String authenticate(final AuthScope authscope, final Credentials crede if (status == STATUS_INITIATED && negotiateClient == null && inputToken == null) { negotiateClient = (NegotiateClient) NegotiateEngine.getInstance().newClient(); negotiateClient.setTarget("http@" + authscope.getHost().toUpperCase()); - + if (credentials instanceof DefaultNTCredentials) { negotiateClient.setCredentialsDefault(); } else if (credentials instanceof UsernamePasswordCredentials) { @@ -221,12 +222,7 @@ else if (status != STATUS_EXCHANGING || negotiateClient == null || inputToken == if (negotiateClient.isComplete()) { status = STATUS_COMPLETE; - - /* Clean up */ - negotiateClient.dispose(); - - negotiateClient = null; - inputToken = null; + cleanup(); } else { status = STATUS_EXCHANGING; }