From 467350843e1d1348dec6234b75f37fd61372e46f Mon Sep 17 00:00:00 2001 From: J Wyman Date: Thu, 5 May 2016 15:18:48 -0400 Subject: [PATCH] Security: Do not persists ADA Refesh Tokens Azure Directory Refresh Tokens are over-privileged for any use the GCM would need them for, therefore they should be discarded as soon as they're no longer needed. The only use of ADA refresh tokens is to use them to acquire VSTS personal access tokens - therefore they should only be persisted long enough to do so, then they should be discared. Additionally, since the GCM has been caching them in the Windows Credential Manager historically, the GCM should make efforts to removed the ADA tokens it has cached. --- .../BaseSecureStore.cs | 35 +++++++++++++++++++ .../BaseVstsAuthentication.cs | 21 ++++++++++- Microsoft.Alm.Authentication/NativeMethods.cs | 33 +++++++++++++++++ Microsoft.Alm.Authentication/SecretStore.cs | 8 +++++ 4 files changed, 96 insertions(+), 1 deletion(-) diff --git a/Microsoft.Alm.Authentication/BaseSecureStore.cs b/Microsoft.Alm.Authentication/BaseSecureStore.cs index 2eb243f91..80a8e6a88 100644 --- a/Microsoft.Alm.Authentication/BaseSecureStore.cs +++ b/Microsoft.Alm.Authentication/BaseSecureStore.cs @@ -39,6 +39,41 @@ protected void Delete(string targetName) protected abstract string GetTargetName(TargetUri targetUri); + protected void PurgeCredentials(string @namespace) + { + string filter = @namespace + "*"; + int count; + IntPtr credentialArrayPtr; + + if (NativeMethods.CredEnumerate(filter, 0, out count, out credentialArrayPtr)) + { + for (int i = 0; i < count; i += 1) + { + int offset = i * Marshal.SizeOf(typeof(IntPtr)); + IntPtr credentialPtr = Marshal.ReadIntPtr(credentialArrayPtr, offset); + + if (credentialPtr != IntPtr.Zero) + { + NativeMethods.Credential credential = Marshal.PtrToStructure(credentialPtr); + + if (!NativeMethods.CredDelete(credential.TargetName, credential.Type, 0)) + { + int error = Marshal.GetLastWin32Error(); + Debug.Fail("Failed with error code " + error.ToString("X")); + } + + } + } + + NativeMethods.CredFree(credentialArrayPtr); + } + else + { + int error = Marshal.GetLastWin32Error(); + Debug.Fail("Failed with error code " + error.ToString("X")); + } + } + protected Credential ReadCredentials(string targetName) { Trace.WriteLine("BaseSecureStore::ReadCredentials"); diff --git a/Microsoft.Alm.Authentication/BaseVstsAuthentication.cs b/Microsoft.Alm.Authentication/BaseVstsAuthentication.cs index 9b454d3e0..2d8a7281a 100644 --- a/Microsoft.Alm.Authentication/BaseVstsAuthentication.cs +++ b/Microsoft.Alm.Authentication/BaseVstsAuthentication.cs @@ -3,6 +3,8 @@ using System.Net; using System.Threading.Tasks; using Microsoft.IdentityModel.Clients.ActiveDirectory; +using System.Text; +using System.Runtime.InteropServices; namespace Microsoft.Alm.Authentication { @@ -27,11 +29,14 @@ private BaseVstsAuthentication(VstsTokenScope tokenScope, ICredentialStore perso AdalTrace.TraceSource.Switch.Level = SourceLevels.Off; AdalTrace.LegacyTraceSwitch.Level = TraceLevel.Off; + // attempt to purge any cached ada tokens. + SecurityPurgeAdaTokens(new SecretStore(AdalRefreshPrefx)); + this.ClientId = DefaultClientId; this.Resource = DefaultResource; this.TokenScope = tokenScope; this.PersonalAccessTokenStore = personalAccessTokenStore; - this.AdaRefreshTokenStore = new SecretStore(AdalRefreshPrefx); + this.AdaRefreshTokenStore = new SecretCache(AdalRefreshPrefx); this.VstsAuthority = new VstsAzureAuthority(); } /// @@ -337,5 +342,19 @@ public static bool GetAuthentication( return authentication != null; } + + /// + /// Attempts to enumerate and delete any and all Azure Directory Authentication + /// Refresh tokens caches by GCM asynchronously. + /// + /// A for the async action. + private static Task SecurityPurgeAdaTokens(SecretStore adaStore) + { + // this can and should be done asynchronously to minimize user impact + return Task.Run(() => + { + adaStore.PurgeCredentials(); + }); + } } } diff --git a/Microsoft.Alm.Authentication/NativeMethods.cs b/Microsoft.Alm.Authentication/NativeMethods.cs index b4acf2485..c54d46b7a 100644 --- a/Microsoft.Alm.Authentication/NativeMethods.cs +++ b/Microsoft.Alm.Authentication/NativeMethods.cs @@ -74,6 +74,39 @@ internal static class NativeMethods [DllImport(Advapi32, CharSet = CharSet.Unicode, EntryPoint = "CredFree", SetLastError = true)] internal static extern void CredFree(IntPtr credential); + /// + /// Enumerates the credentials from the user's credential set. The credential set used + /// is the one associated with the logon session of the current token. The token must + /// not have the user's SID disabled. + /// + /// + /// Pointer to a null-terminated string that contains the filter for the returned + /// credentials. Only credentials with a TargetName matching the filter will be returned. + /// The filter specifies a name prefix followed by an asterisk. For instance, the filter + /// "FRED*" will return all credentials with a TargetName beginning with the string + /// "FRED". + /// If is specified, all credentials will be returned. + /// + /// The value of this parameter can be zero or more values combined + /// with a bitwise-OR operation. + /// Count of the credentials returned in the . + /// + /// Pointer to an array of pointers to credentials. The returned credential is a + /// single allocated block. Any pointers contained within the buffer are pointers to + /// locations within this single allocated block. + /// The single returned buffer must be freed by calling . + /// + /// + [DllImport(Advapi32, CharSet = CharSet.Unicode, EntryPoint = "CredEnumerateW", SetLastError = true)] + internal static extern bool CredEnumerate(string targetNameFilter, CredentialEnumerateFlags flags, out int count, out IntPtr credenitalsArrayPtr); + + [Flags] + internal enum CredentialEnumerateFlags : uint + { + None = 0, + AllCredentials = 1 << 0, + } + [Flags] internal enum CredentialFlags : uint { diff --git a/Microsoft.Alm.Authentication/SecretStore.cs b/Microsoft.Alm.Authentication/SecretStore.cs index 931646ca5..5aa19742b 100644 --- a/Microsoft.Alm.Authentication/SecretStore.cs +++ b/Microsoft.Alm.Authentication/SecretStore.cs @@ -73,6 +73,14 @@ public void DeleteToken(TargetUri targetUri) _tokenCache.DeleteToken(targetUri); } + /// + /// Purges all credenitals from the store. + /// + public void PurgeCredentials() + { + PurgeCredentials(_namespace); + } + /// /// Reads credentials for a target URI from the credential store ///