From c3230312fe44dceffe4e2add9c497139133ce001 Mon Sep 17 00:00:00 2001 From: Dennis Dyall Date: Thu, 14 Nov 2024 16:20:24 +0100 Subject: [PATCH] Fixes #160. Refactor PivSession and update tests for management key algorithm handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Update management key algorithm refresh • Add FIPS-specific test cases • Enhance test coverage for key changes • Adjust for firmware version differences --- .../YubiKey/Piv/PivSession.ManagementKey.cs | 141 ++++++++++-------- .../src/Yubico/YubiKey/Piv/PivSession.cs | 27 +--- .../Yubico/YubiKey/Piv/ManagementKeyTests.cs | 88 ++++++++--- 3 files changed, 151 insertions(+), 105 deletions(-) diff --git a/Yubico.YubiKey/src/Yubico/YubiKey/Piv/PivSession.ManagementKey.cs b/Yubico.YubiKey/src/Yubico/YubiKey/Piv/PivSession.ManagementKey.cs index 12a7a2071..cf2e29d95 100644 --- a/Yubico.YubiKey/src/Yubico/YubiKey/Piv/PivSession.ManagementKey.cs +++ b/Yubico.YubiKey/src/Yubico/YubiKey/Piv/PivSession.ManagementKey.cs @@ -71,6 +71,12 @@ public sealed partial class PivSession : IDisposable /// public AuthenticateManagementKeyResult ManagementKeyAuthenticationResult { get; private set; } + private PivAlgorithm DefaultManagementKeyAlgorithm => + _yubiKeyDevice.HasFeature(YubiKeyFeature.PivAesManagementKey) && + _yubiKeyDevice.FirmwareVersion > FirmwareVersion.V5_7_0 + ? PivAlgorithm.Aes192 + : PivAlgorithm.TripleDes; + /// /// Try to authenticate the management key. /// @@ -531,7 +537,7 @@ public bool TryAuthenticateManagementKey(ReadOnlyMemory managementKey, boo /// authenticated. /// public bool TryChangeManagementKey(PivTouchPolicy touchPolicy = PivTouchPolicy.Default) => - TryChangeManagementKey(touchPolicy, PivAlgorithm.TripleDes); + TryChangeManagementKey(touchPolicy, DefaultManagementKeyAlgorithm); /// /// Try to change the management key. The new key will be the specified @@ -651,7 +657,8 @@ public bool TryChangeManagementKey(PivTouchPolicy touchPolicy = PivTouchPolicy.D /// public bool TryChangeManagementKey(PivTouchPolicy touchPolicy, PivAlgorithm newKeyAlgorithm) { - _log.LogInformation("Try to change the management key, touch policy = {TouchPolicy}, algorithm = {PivALgorithm}.", + _log.LogInformation( + "Try to change the management key, touch policy = {TouchPolicy}, algorithm = {PivALgorithm}.", touchPolicy.ToString(), newKeyAlgorithm.ToString()); CheckManagementKeyAlgorithm(newKeyAlgorithm, true); @@ -726,7 +733,7 @@ public bool TryChangeManagementKey(PivTouchPolicy touchPolicy, PivAlgorithm newK /// authenticated. /// public void ChangeManagementKey(PivTouchPolicy touchPolicy = PivTouchPolicy.Default) => - ChangeManagementKey(touchPolicy, PivAlgorithm.TripleDes); + ChangeManagementKey(touchPolicy, DefaultManagementKeyAlgorithm); /// /// Change the management key, throw an exception if the user cancels. @@ -761,7 +768,8 @@ public void ChangeManagementKey(PivTouchPolicy touchPolicy = PivTouchPolicy.Defa /// public void ChangeManagementKey(PivTouchPolicy touchPolicy, PivAlgorithm newKeyAlgorithm) { - _log.LogInformation("Change the management key, touch policy = {TouchPolicy}, algorithm = {PivAlgorithm}.", + _log.LogInformation( + "Change the management key, touch policy = {TouchPolicy}, algorithm = {PivAlgorithm}.", touchPolicy.ToString(), newKeyAlgorithm.ToString()); if (TryChangeManagementKey(touchPolicy, newKeyAlgorithm) == false) @@ -827,7 +835,7 @@ public void ChangeManagementKey(PivTouchPolicy touchPolicy, PivAlgorithm newKeyA public bool TryChangeManagementKey(ReadOnlyMemory currentKey, ReadOnlyMemory newKey, PivTouchPolicy touchPolicy = PivTouchPolicy.Default) => - TryChangeManagementKey(currentKey, newKey, touchPolicy, PivAlgorithm.TripleDes); + TryChangeManagementKey(currentKey, newKey, touchPolicy, DefaultManagementKeyAlgorithm); /// /// Try to change the management key. This method will use the @@ -915,68 +923,11 @@ private bool TryForcedChangeManagementKey(ReadOnlyMemory currentKey, } _log.LogInformation($"Failed to set management key. Message: {response.StatusMessage}"); - } return false; } - // Verify that and that the given algorithm is allowed. - // If checkMode is true, also check that the PIN-only mode is None. - // This is called by methods that set PIN-only mode or change the mgmt - // key. - // The algorithm can only be 3DES or AES, and it can only be AES if the - // YubiKey is 5.4.2 or later. - // It is not allowed to change the mgmt key if it is PIN-only, so those - // methods that change, will check the mode as well (they will pass true - // as the checkMode arg). - // If setting PIN-only, then the mode is not an issue, so don't check - // (pass false as the checkMode arg). - // If everything is fine, return, otherwise throw an exception. - private void CheckManagementKeyAlgorithm(PivAlgorithm algorithm, bool checkMode) - { - if (checkMode) - { - var pinOnlyMode = GetPinOnlyMode(); - if (pinOnlyMode.HasFlag(PivPinOnlyMode.PinProtected) || - pinOnlyMode.HasFlag(PivPinOnlyMode.PinDerived)) - { - throw new InvalidOperationException( - string.Format( - CultureInfo.CurrentCulture, - ExceptionMessages.MgmtKeyCannotBeChanged)); - } - } - - bool isValid = false; - - switch (algorithm) - { - case PivAlgorithm.TripleDes: - isValid = true; - - break; - - case PivAlgorithm.Aes128: - case PivAlgorithm.Aes192: - case PivAlgorithm.Aes256: - isValid = _yubiKeyDevice.HasFeature(YubiKeyFeature.PivAesManagementKey); - - break; - - default: - break; - } - - if (!isValid) - { - throw new ArgumentException( - string.Format( - CultureInfo.CurrentCulture, - ExceptionMessages.UnsupportedAlgorithm)); - } - } - // This is the actual Try code, shared by both TryAuth and TryChange. // The caller provides a KeyEntryData object set with the appropriate // request: @@ -1046,7 +997,8 @@ private bool TryAuthenticateManagementKey(bool mutualAuthentication, // off-card app authenticated, but the YubiKey itself did // not. // If case (3), throw an exception. - if (ManagementKeyAuthenticationResult == AuthenticateManagementKeyResult.MutualYubiKeyAuthenticationFailed) + if (ManagementKeyAuthenticationResult == + AuthenticateManagementKeyResult.MutualYubiKeyAuthenticationFailed) { throw new SecurityException( string.Format( @@ -1061,5 +1013,68 @@ private bool TryAuthenticateManagementKey(bool mutualAuthentication, return ManagementKeyAuthenticated; } + + private void RefreshManagementKeyAlgorithm() => ManagementKeyAlgorithm = GetManagementKeyAlgorithm(); + + private PivAlgorithm GetManagementKeyAlgorithm() + { + var response = Connection.SendCommand(new GetMetadataCommand(PivSlot.Management)); + if (response.Status != ResponseStatus.Success) + { + throw new InvalidOperationException(response.StatusMessage); + } + + var metadata = response.GetData(); + return metadata.Algorithm; + } + + // Verify that and that the given algorithm is allowed. + // If checkMode is true, also check that the PIN-only mode is None. + // This is called by methods that set PIN-only mode or change the mgmt + // key. + // The algorithm can only be 3DES or AES, and it can only be AES if the + // YubiKey is 5.4.2 or later. + // It is not allowed to change the mgmt key if it is PIN-only, so those + // methods that change, will check the mode as well (they will pass true + // as the checkMode arg). + // If setting PIN-only, then the mode is not an issue, so don't check + // (pass false as the checkMode arg). + // If everything is fine, return, otherwise throw an exception. + private void CheckManagementKeyAlgorithm(PivAlgorithm algorithm, bool checkMode) + { + if (checkMode) + { + var pinOnlyMode = GetPinOnlyMode(); + if (pinOnlyMode.HasFlag(PivPinOnlyMode.PinProtected) || + pinOnlyMode.HasFlag(PivPinOnlyMode.PinDerived)) + { + throw new InvalidOperationException( + string.Format( + CultureInfo.CurrentCulture, + ExceptionMessages.MgmtKeyCannotBeChanged)); + } + } + + bool isValid = IsValid(algorithm); + if (!isValid) + { + throw new ArgumentException( + string.Format( + CultureInfo.CurrentCulture, + ExceptionMessages.UnsupportedAlgorithm)); + } + + return; + + bool IsValid(PivAlgorithm pa) => + pa switch + { + PivAlgorithm.TripleDes => true, // Default for keys below fw version 5.7 + PivAlgorithm.Aes128 => _yubiKeyDevice.HasFeature(YubiKeyFeature.PivAesManagementKey), + PivAlgorithm.Aes192 => _yubiKeyDevice.HasFeature(YubiKeyFeature.PivAesManagementKey), + PivAlgorithm.Aes256 => _yubiKeyDevice.HasFeature(YubiKeyFeature.PivAesManagementKey), + _ => false + }; + } } } diff --git a/Yubico.YubiKey/src/Yubico/YubiKey/Piv/PivSession.cs b/Yubico.YubiKey/src/Yubico/YubiKey/Piv/PivSession.cs index 43c664c2e..1f7fa198c 100644 --- a/Yubico.YubiKey/src/Yubico/YubiKey/Piv/PivSession.cs +++ b/Yubico.YubiKey/src/Yubico/YubiKey/Piv/PivSession.cs @@ -249,7 +249,7 @@ private PivSession(StaticKeys? scp03Keys, IYubiKeyDevice yubiKey) : yubiKey.ConnectScp03(YubiKeyApplication.Piv, scp03Keys); ResetAuthenticationStatus(); - UpdateManagementKey(yubiKey); + RefreshManagementKeyAlgorithm(); _yubiKeyDevice = yubiKey; _disposed = false; @@ -313,14 +313,14 @@ public void Dispose() { _ = Connection.SendCommand(new SelectApplicationCommand(YubiKeyApplication.Management)); } -#pragma warning disable CA1031 + #pragma warning disable CA1031 catch (Exception e) -#pragma warning restore CA1031 + #pragma warning restore CA1031 { string message = string.Format( CultureInfo.CurrentCulture, ExceptionMessages.PivSessionDisposeUnknownError, e.GetType(), e.Message); - + // Example: // Exception caught when disposing PivSession: Yubico.PlatformInterop.SCardException, // Unable to begin a transaction with the given smart card. SCARD_E_SERVICE_STOPPED: The smart card resource manager has shut down. @@ -510,7 +510,7 @@ public void ResetApplication() // As resetting the PIV application resets the management key, // the management key must be updated to account for the case when the previous management key type // was not the default key type. - UpdateManagementKey(_yubiKeyDevice); + RefreshManagementKeyAlgorithm(); } /// @@ -671,22 +671,5 @@ private void TryBlock(byte slot) CultureInfo.CurrentCulture, ExceptionMessages.ApplicationResetFailure)); } - - private void UpdateManagementKey(IYubiKeyDevice yubiKey) => - ManagementKeyAlgorithm = yubiKey.HasFeature(YubiKeyFeature.PivAesManagementKey) - ? GetManagementKeyAlgorithm() - : PivAlgorithm.TripleDes; // Default for keys with firmware version < 5.7 - - private PivAlgorithm GetManagementKeyAlgorithm() - { - var response = Connection.SendCommand(new GetMetadataCommand(PivSlot.Management)); - if (response.Status != ResponseStatus.Success) - { - throw new InvalidOperationException(response.StatusMessage); - } - - var metadata = response.GetData(); - return metadata.Algorithm; - } } } diff --git a/Yubico.YubiKey/tests/integration/Yubico/YubiKey/Piv/ManagementKeyTests.cs b/Yubico.YubiKey/tests/integration/Yubico/YubiKey/Piv/ManagementKeyTests.cs index 6e7e5d716..6ecd5afa3 100644 --- a/Yubico.YubiKey/tests/integration/Yubico/YubiKey/Piv/ManagementKeyTests.cs +++ b/Yubico.YubiKey/tests/integration/Yubico/YubiKey/Piv/ManagementKeyTests.cs @@ -44,10 +44,12 @@ public ManagementKeyTests(ITestOutputHelper output) }; } - [Fact] - public void HasFeature_ReturnsCorrect() + [Theory] + [InlineData(StandardTestDevice.Fw5)] + [InlineData(StandardTestDevice.Fw5Fips)] + public void HasFeature_ReturnsCorrect(StandardTestDevice device) { - var testDevice = IntegrationTestDeviceEnumeration.GetTestDevice(StandardTestDevice.Fw5); + var testDevice = IntegrationTestDeviceEnumeration.GetTestDevice(device); var expectedResult = testDevice.FirmwareVersion >= new FirmwareVersion(major: 5, minor: 4, patch: 2); @@ -56,47 +58,93 @@ public void HasFeature_ReturnsCorrect() Assert.Equal(hasFeature, expectedResult); } - [Fact] - public void GetManagementAlgorithm_WhenReset_ReturnsCorrectType() + [Theory] + [InlineData(StandardTestDevice.Fw5)] + [InlineData(StandardTestDevice.Fw5Fips)] + public void GetManagementAlgorithm_WhenReset_ReturnsCorrectType(StandardTestDevice device) { - var testDevice = IntegrationTestDeviceEnumeration.GetTestDevice(StandardTestDevice.Fw5); - var shouldBeTripleDes = testDevice.FirmwareVersion < FirmwareVersion.V5_7_0; - var defaultManagementKeyType = shouldBeTripleDes - ? PivAlgorithm.TripleDes - : PivAlgorithm.Aes192; + var testDevice = IntegrationTestDeviceEnumeration.GetTestDevice(device); + var shouldBeAes = testDevice.FirmwareVersion >= FirmwareVersion.V5_7_0; + var mustBeAes = shouldBeAes && testDevice.IsFipsSeries; + var defaultManagementKeyType = shouldBeAes + ? PivAlgorithm.Aes192 + : PivAlgorithm.TripleDes; - var alternativeManagementKeyType = !shouldBeTripleDes - ? PivAlgorithm.TripleDes - : PivAlgorithm.Aes192; + var alternativeManagementKeyType = !shouldBeAes + ? PivAlgorithm.Aes192 + : PivAlgorithm.TripleDes; using var session = new PivSession(testDevice); session.KeyCollector = TestKeyCollectorDelegate; session.ResetApplication(); - session.ChangeManagementKey(PivTouchPolicy.None, alternativeManagementKeyType); - var firstCheckKeyType = session.ManagementKeyAlgorithm; + + // This must throw for FIPS devices. + if (mustBeAes) + { + Assert.Throws( + () => session.ChangeManagementKey(PivTouchPolicy.None, PivAlgorithm.TripleDes)); + } + else + { + session.ChangeManagementKey(PivTouchPolicy.None, alternativeManagementKeyType); + Assert.Equal(alternativeManagementKeyType, session.ManagementKeyAlgorithm); + + session.AuthenticateManagementKey(); + session.ResetApplication(); - Assert.Equal(alternativeManagementKeyType, firstCheckKeyType); + Assert.Equal(defaultManagementKeyType, session.ManagementKeyAlgorithm); + } + } - session.AuthenticateManagementKey(); + [Theory] + [InlineData(StandardTestDevice.Fw5)] + [InlineData(StandardTestDevice.Fw5Fips)] + public void ChangeManagementKey_WithDefaultParameters_UsesCorrectTypeForRespectiveVersion(StandardTestDevice device) + { + var testDevice = IntegrationTestDeviceEnumeration.GetTestDevice(device); + + var shouldBeAes = testDevice.FirmwareVersion >= FirmwareVersion.V5_7_0; + var mustBeAes = shouldBeAes && testDevice.IsFipsSeries; + var defaultManagementKeyType = shouldBeAes || mustBeAes + ? PivAlgorithm.Aes192 + : PivAlgorithm.TripleDes; + + using var session = new PivSession(testDevice); + session.KeyCollector = TestKeyCollectorDelegate; session.ResetApplication(); - var secondCheckKeyType = session.ManagementKeyAlgorithm; - Assert.Equal(defaultManagementKeyType, secondCheckKeyType); + // This must not throw. 5.7 FIPS requires management key to be AES192. + session.ChangeManagementKey(); + Assert.Equal(defaultManagementKeyType, session.ManagementKeyAlgorithm); + + // This must throw for FIPS devices. + if (mustBeAes) + { + Assert.Throws( + () => session.ChangeManagementKey(PivTouchPolicy.None, PivAlgorithm.TripleDes)); + } } [Theory] [InlineData(StandardTestDevice.Fw5)] + [InlineData(StandardTestDevice.Fw5Fips)] public void RandomKey_Authenticates(StandardTestDevice testDeviceType) { var testDevice = IntegrationTestDeviceEnumeration.GetTestDevice(testDeviceType); + var shouldBeAes = testDevice.FirmwareVersion >= FirmwareVersion.V5_7_0; + var mustBeAes = shouldBeAes && testDevice.IsFipsSeries; + var defaultManagementKeyType = shouldBeAes || mustBeAes + ? PivAlgorithm.Aes192 + : PivAlgorithm.TripleDes; + var isValid = false; var count = 10; for (var index = 0; index < count; index++) { GetRandomMgmtKey(); - isValid = ChangeMgmtKey(testDevice, PivAlgorithm.TripleDes); + isValid = ChangeMgmtKey(testDevice, defaultManagementKeyType); if (!isValid) { break;