Skip to content

Feature/refactoring #80

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Feature/refactoring #80

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
de194d2
Secure array resize
shuffle-c Aug 26, 2021
3fb6f45
Lazy checks in WinHelloProvider
shuffle-c Aug 26, 2021
0594780
Setter for CurrentCacheType
shuffle-c Aug 26, 2021
a63e4e2
Renamed SwitchCurrentCacheType to reduce misleading
shuffle-c Aug 26, 2021
93ff827
KeyManager always exists, single entry to handle auth exceptions
shuffle-c Aug 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions src/AuthProviders/IAuthProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,22 +13,23 @@ public interface IAuthProvider
byte[] Encrypt(byte[] data);
byte[] PromptToDecrypt(byte[] data);
void ClaimCurrentCacheType(AuthCacheType authCacheType);
AuthCacheType CurrentCacheType { get; }
AuthCacheType CurrentCacheType { get; set; }
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Наличие сеттера вводит в заблужение о цели существования ClaimCurrentCacheType

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Наверное, просто Claim... не должен быть частью AuthProviderа

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ещё надо подумать – можно ли как-то обойтись от зависимости на тип кэша у провайдера совсем. Например, этот же энам у нас используется и при создании storage'а, но там мы на разные реализации всё разнесли и свитч только в фабрике присутствует. Может и тут нам также поступить и декомпозировать ответственность за шифровку/дешивровку в отдельный класс, а за выбор ключа сделать ответственным кого-нибудь другого? Можно передавать провайдер ключа как зависимость AuthProvider'у, например. Что думаешь?

}

static class AuthProviderFactory
{
public static IAuthProvider GetInstance(IntPtr keePassWindowHandle, AuthCacheType authCacheType)
public static IAuthProvider Create(IntPtr keePassWindowHandle)
{
var authCacheType = Settings.Instance.GetAuthCacheType();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Не очень нравится что тут зависимость на настройки. Всё-таки кажется что раз метод статический, то хочется чтобы он был чистым.
Но, с другой стороны, тут так же есть UAC и мы жили с этим без напрягов

#if DEBUG
var provider = new XorProvider(authCacheType);
#else
var provider = WinHelloProvider.CreateInstance(authCacheType);
var provider = new WinHelloProvider(authCacheType);
#endif
if (UAC.IsCurrentProcessElevated)
return new WinHelloProviderForegroundDecorator(provider, keePassWindowHandle);
else
return provider;
}
}
}
}
168 changes: 71 additions & 97 deletions src/AuthProviders/WinHelloProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,15 +123,15 @@ private static extern SECURITY_STATUS NCryptDecrypt(SafeNCryptKeyHandle hKey,
#endregion

private static readonly Lazy<string> _currentSID = new Lazy<string>(WindowsIdentity.GetCurrent().User.ToString);

private static readonly object _mutex = new object();
private static WeakReference _instance;

private const string Domain = Settings.ProductName;
private const string SubDomain = "";
private const string PersistentName = Settings.ProductName;
private const string InvalidatedKeyMessage = "Persistent key has not met integrity requirements. It might be caused by a spoofing attack. Try to recreate the key.";

private AuthCacheType _currentCacheType;

private static string LocalKeyName
{
get
Expand All @@ -154,99 +154,45 @@ private static string PersistentKeyName

private string CurrentKeyName
{
get { return CurrentCacheType == AuthCacheType.Local ? LocalKeyName : PersistentKeyName; }
get { return _currentCacheType == AuthCacheType.Local ? LocalKeyName : PersistentKeyName; }
}

private WinHelloProvider(AuthCacheType authCacheType)
public WinHelloProvider(AuthCacheType authCacheType)
{
CurrentCacheType = authCacheType;

if (authCacheType == AuthCacheType.Local)
{
DeletePersistentKey();
}
else
{
System.Diagnostics.Debug.Assert(authCacheType == AuthCacheType.Persistent);

SafeNCryptKeyHandle ngcKeyHandle;
if (!TryOpenPersistentKey(out ngcKeyHandle))
throw new AuthProviderKeyNotFoundException("Persistent key does not exist.");

using (ngcKeyHandle)
{
if (!VerifyPersistentKeyIntegrity(ngcKeyHandle))
{
ngcKeyHandle.Close();
DeletePersistentKey();
throw new AuthProviderInvalidKeyException(InvalidatedKeyMessage);
}
}
}
_currentCacheType = authCacheType;
}

public AuthCacheType CurrentCacheType { get; private set; }

public static WinHelloProvider CreateInstance(AuthCacheType authCacheType)
public AuthCacheType CurrentCacheType
{
EnsureWinHelloAvailability();

lock (_mutex)
{
WinHelloProvider winHelloProvider = null;
if (_instance != null && (winHelloProvider = _instance.Target as WinHelloProvider) != null)
{
if (winHelloProvider.CurrentCacheType == authCacheType)
return winHelloProvider;
else
throw new AuthProviderException("Incompatible cache type with existing instance.");
}

winHelloProvider = new WinHelloProvider(authCacheType);
_instance = new WeakReference(winHelloProvider);

return winHelloProvider;
}
get { return EnsureKeyAvailability(_currentCacheType); }
set { _currentCacheType = EnsureKeyAvailability(value); }
}

public void ClaimCurrentCacheType(AuthCacheType authCacheType)
{
if (CurrentCacheType == authCacheType)
return;

lock (_mutex)
{
if (authCacheType == AuthCacheType.Local)
try
{
DeletePersistentKey();
CurrentCacheType = authCacheType;
}
else
catch (AuthProviderKeyNotFoundException)
{
Debug.Assert(authCacheType == AuthCacheType.Persistent);

SafeNCryptKeyHandle ngcKeyHandle;
if (TryOpenPersistentKey(out ngcKeyHandle))
{
try
{
if (!VerifyPersistentKeyIntegrity(ngcKeyHandle))
throw new AuthProviderInvalidKeyException(InvalidatedKeyMessage);
ngcKeyHandle.Dispose();
}
catch
{
ngcKeyHandle.Dispose();
DeletePersistentKey();
throw;
}
}
else
{
CreatePersistentKey(false).Dispose();
}
CreatePersistentKey(false).Dispose();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Раз уж тронули, добавь плиз overwriteExisting:

_currentCacheType = authCacheType;
}
catch (AuthProviderInvalidKeyException)
{
Debug.Assert(authCacheType == AuthCacheType.Persistent);
CreatePersistentKey(true).Dispose();
_currentCacheType = authCacheType;
}
finally
{
if (authCacheType == AuthCacheType.Local)
DeletePersistentKey();
}

CurrentCacheType = authCacheType;
}
}

Expand All @@ -256,7 +202,8 @@ public byte[] Encrypt(byte[] data)
{
try
{
return Encrypt(data, retry: i > 0);
var verifyIntegrity = _currentCacheType == AuthCacheType.Persistent;
return Encrypt(data, CurrentKeyName, verifyIntegrity);
}
catch (AuthProviderSystemErrorException ex)
{
Expand All @@ -283,7 +230,8 @@ public byte[] PromptToDecrypt(byte[] data)
{
try
{
return PromptToDecrypt(data, retry: i > 0);
var verifyIntegrity = _currentCacheType == AuthCacheType.Persistent;
return PromptToDecrypt(data, CurrentKeyName, verifyIntegrity, retry: i > 0);
}
catch (AuthProviderSystemErrorException ex)
{
Expand All @@ -304,44 +252,44 @@ public byte[] PromptToDecrypt(byte[] data)
}
}

private byte[] Encrypt(byte[] data, bool retry)
private static byte[] Encrypt(byte[] data, string keyName, bool verifyIntegrity)
{
byte[] cbResult;
SafeNCryptProviderHandle ngcProviderHandle;
NCryptOpenStorageProvider(out ngcProviderHandle, MS_NGC_KEY_STORAGE_PROVIDER, 0).ThrowOnError("NCryptOpenStorageProvider");
using (ngcProviderHandle)
{
SafeNCryptKeyHandle ngcKeyHandle;
NCryptOpenKey(ngcProviderHandle, out ngcKeyHandle, CurrentKeyName, 0, CngKeyOpenOptions.Silent).ThrowOnError("NCryptOpenKey");
NCryptOpenKey(ngcProviderHandle, out ngcKeyHandle, keyName, 0, CngKeyOpenOptions.Silent).ThrowOnError("NCryptOpenKey");
using (ngcKeyHandle)
{
if (CurrentCacheType == AuthCacheType.Persistent && !VerifyPersistentKeyIntegrity(ngcKeyHandle))
if (verifyIntegrity && !VerifyPersistentKeyIntegrity(ngcKeyHandle))
throw new AuthProviderInvalidKeyException(InvalidatedKeyMessage);

int pcbResult;
NCryptEncrypt(ngcKeyHandle, data, data.Length, IntPtr.Zero, null, 0, out pcbResult, NCRYPT_PAD_PKCS1_FLAG).ThrowOnError("NCryptEncrypt");

cbResult = new byte[pcbResult];
NCryptEncrypt(ngcKeyHandle, data, data.Length, IntPtr.Zero, cbResult, cbResult.Length, out pcbResult, NCRYPT_PAD_PKCS1_FLAG).ThrowOnError("NCryptEncrypt");
System.Diagnostics.Debug.Assert(cbResult.Length == pcbResult);
Debug.Assert(cbResult.Length == pcbResult);
}
}

return cbResult;
}

private byte[] PromptToDecrypt(byte[] data, bool retry)
private static byte[] PromptToDecrypt(byte[] data, string keyName, bool verifyIntegrity, bool retry)
{
byte[] cbResult;
SafeNCryptProviderHandle ngcProviderHandle;
NCryptOpenStorageProvider(out ngcProviderHandle, MS_NGC_KEY_STORAGE_PROVIDER, 0).ThrowOnError("NCryptOpenStorageProvider");
using (ngcProviderHandle)
{
SafeNCryptKeyHandle ngcKeyHandle;
NCryptOpenKey(ngcProviderHandle, out ngcKeyHandle, CurrentKeyName, 0, CngKeyOpenOptions.None).ThrowOnError("NCryptOpenKey");
NCryptOpenKey(ngcProviderHandle, out ngcKeyHandle, keyName, 0, CngKeyOpenOptions.None).ThrowOnError("NCryptOpenKey");
using (ngcKeyHandle)
{
if (CurrentCacheType == AuthCacheType.Persistent && !VerifyPersistentKeyIntegrity(ngcKeyHandle))
if (verifyIntegrity && !VerifyPersistentKeyIntegrity(ngcKeyHandle))
throw new AuthProviderInvalidKeyException(InvalidatedKeyMessage);

ApplyUIContext(ngcKeyHandle, retry);
Expand All @@ -353,14 +301,47 @@ private byte[] PromptToDecrypt(byte[] data, bool retry)
cbResult = new byte[data.Length * 2];
int pcbResult;
NCryptDecrypt(ngcKeyHandle, data, data.Length, IntPtr.Zero, cbResult, cbResult.Length, out pcbResult, NCRYPT_PAD_PKCS1_FLAG).ThrowOnError("NCryptDecrypt");
// TODO: secure resize
Array.Resize(ref cbResult, pcbResult);

if (cbResult.Length > pcbResult)
{
var res = new byte[pcbResult];
Array.Copy(cbResult, res, pcbResult);
Array.Clear(cbResult, 0, cbResult.Length);
cbResult = res;
}
}
}

return cbResult;
}

private static AuthCacheType EnsureKeyAvailability(AuthCacheType authCacheType)
{
EnsureWinHelloAvailability();

if (authCacheType == AuthCacheType.Persistent) lock (_mutex)
{
SafeNCryptKeyHandle ngcKeyHandle;
if (!TryOpenPersistentKey(out ngcKeyHandle))
throw new AuthProviderKeyNotFoundException("Persistent key does not exist.");

using (ngcKeyHandle)
{
if (!VerifyPersistentKeyIntegrity(ngcKeyHandle))
throw new AuthProviderInvalidKeyException(InvalidatedKeyMessage);
}
}
else if (authCacheType != AuthCacheType.Local)
throw new NotSupportedException("Unknown cache type: " + authCacheType);

return authCacheType;
}

private static void EnsureWinHelloAvailability()
{
var dummy = LocalKeyName; // throw an exception if not available
}

private static void RetrieveKeys(out string localKey, out string persistentKey)
{
NgcGetDefaultDecryptionKeyName(_currentSID.Value, 0, 0, out localKey);
Expand All @@ -371,11 +352,6 @@ private static void RetrieveKeys(out string localKey, out string persistentKey)
throw new AuthProviderIsUnavailableException("Windows Hello is not available.");
}

private static void EnsureWinHelloAvailability()
{
var dummy = LocalKeyName; // throw an exception if not available
}

private static void DeletePersistentKey()
{
SafeNCryptKeyHandle ngcKeyHandle;
Expand Down Expand Up @@ -423,10 +399,8 @@ private static bool VerifyPersistentKeyIntegrity(SafeNCryptKeyHandle ngcKeyHandl
{
NCryptGetProperty(ngcKeyHandle, NCRYPT_NGC_CACHE_TYPE_PROPERTY_DEPRECATED, ref cacheType, sizeof(int), out pcbResult, CngPropertyOptions.None).ThrowOnError("NCRYPT_NGC_CACHE_TYPE_PROPERTY_DEPRECATED");
}
if (cacheType != NCRYPT_NGC_CACHE_TYPE_PROPERTY_AUTH_MANDATORY_FLAG)
return false;

return true;
return cacheType == NCRYPT_NGC_CACHE_TYPE_PROPERTY_AUTH_MANDATORY_FLAG;
}

private static SafeNCryptKeyHandle CreatePersistentKey(bool overwriteExisting)
Expand Down
6 changes: 2 additions & 4 deletions src/AuthProviders/WinHelloProviderForegroundDecorator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,8 @@ public WinHelloProviderForegroundDecorator(IAuthProvider provider, IntPtr keePas

public AuthCacheType CurrentCacheType
{
get
{
return _winHelloProvider.CurrentCacheType;
}
get { return _winHelloProvider.CurrentCacheType; }
set { _winHelloProvider.CurrentCacheType = value; }
}

public void ClaimCurrentCacheType(AuthCacheType newType)
Expand Down
6 changes: 3 additions & 3 deletions src/AuthProviders/XorProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,10 @@ public XorProvider(AuthCacheType authCacheType)
CurrentCacheType = authCacheType;
}

public AuthCacheType CurrentCacheType { get; private set; } // TDB
public AuthCacheType CurrentCacheType { get; set; }

public void ClaimCurrentCacheType(AuthCacheType newType)
{
CurrentCacheType = newType;

if (newType == AuthCacheType.Persistent)
{
string message = "Default message for persistent auth type";
Expand All @@ -34,6 +32,8 @@ public void ClaimCurrentCacheType(AuthCacheType newType)
{
MessageBox.Show(AuthProviderUIContext.Current, "Switched to local.", "Keys removed", MessageBoxButtons.OK, MessageBoxIcon.Information);
}

CurrentCacheType = newType;
}

public byte[] Encrypt(byte[] data)
Expand Down
2 changes: 1 addition & 1 deletion src/Exceptions/AuthProviderKeyNotFoundException.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
namespace KeePassWinHello
{
[Serializable]
public class AuthProviderKeyNotFoundException : AuthProviderException // TODO: consider inherit from AuthProviderInvalidKeyException
public class AuthProviderKeyNotFoundException : AuthProviderInvalidKeyException
{
public override bool IsPresentable { get { return true; } }

Expand Down
1 change: 0 additions & 1 deletion src/KeePassWinHello.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@
</ItemGroup>
<ItemGroup>
<Compile Include="Exceptions\AuthProviderKeyNotFoundException.cs" />
<Compile Include="KeyManagement\KeyManagerProvider.cs" />
<Compile Include="Properties\Resources.Designer.cs">
<AutoGen>True</AutoGen>
<DesignTime>True</DesignTime>
Expand Down
Loading