Yubico / Yubico.NET.SDK

A YubiKey SDK for .NET developers
Apache License 2.0
96 stars 48 forks source link

Incorrect TOTP values from Yubico.Yubikey .net nuget SDK? #17

Closed JonathonQuinn closed 1 year ago

JonathonQuinn commented 1 year ago

Hello - we are seeing rather different TOTP results from our C# code than the ykman cli application. The ykman application will return the same TOTP for multiple requests in the same 30s window - our C# doesn't. We've been able to perform MFA with AWS using the ykman TOTP values but not the C# TOTP values.

// somewhat equivalent of: ykman oath accounts code issuer:name && echo %time%

int serial = 123456789; // modified for privacy

if (!YubiKeyDevice.TryGetYubiKey(serial, out IYubiKeyDevice device))
{
    using (OathSession session = new OathSession(device))
    {
        Console.Out.WriteLine("Waiting for touch");
        var code = session.CalculateCredential("issuer", "name", CredentialType.Totp, CredentialPeriod.Period30, ResponseFormat.Full);
        Console.Out.WriteLine(code.Value);
        Console.Out.WriteLine(code.ValidUntil);
    }
}

We've seen this with .NET framework and Yubico.YubiKey Nuget 1.2.0 package as well as .NET 6.0 and Yubico.YubiKey Nuget 1.4.0.

We've noticed the SDK calls a generate challenge https://github.com/Yubico/Yubico.NET.SDK/blob/c22abf723ddfbe7f5fa73038308bce067a0e1156/Yubico.YubiKey/src/Yubico/YubiKey/Oath/Commands/CalculateCredentialCommand.cs#L103 That is pure random data https://github.com/Yubico/Yubico.NET.SDK/blob/c22abf723ddfbe7f5fa73038308bce067a0e1156/Yubico.YubiKey/src/Yubico/YubiKey/Oath/Commands/OathChallengeResponseBaseCommand.cs#L38 While Ykman seems to use a timestamp divided by the period https://github.com/Yubico/yubikey-manager/blob/1076459ba1e308fc71da2c3eb301b9777d7ae9b5/ykman/oath.py#L46 https://github.com/Yubico/yubikey-manager/blob/1076459ba1e308fc71da2c3eb301b9777d7ae9b5/yubikit/oath.py#L426 https://github.com/Yubico/yubikey-manager/blob/1076459ba1e308fc71da2c3eb301b9777d7ae9b5/yubikit/oath.py#L225

Really appreciate the open source and the packages/document available btw

GregDomzalski commented 1 year ago

Thanks for reporting this issue! We'll take a look.

JonathonQuinn commented 1 year ago

Here is a work around - only real difference between our OverriddenCalculateCredentialCommand and CalculateCredentialCommand is the challenge generation

    // example invocation with a Oath session and credential
    OverriddenCalculateCredentialCommand yubiKeyCommand = new OverriddenCalculateCredentialCommand(credential, ResponseFormat.Truncated);
    var totp = session.Connection.SendCommand(yubiKeyCommand).GetData();

    internal class OverriddenCalculateCredentialCommand : IYubiKeyCommand<CalculateCredentialResponse>
    {
        //
        // Summary:
        //     The credential to calculate.
        public Credential Credential { get; set; }
        //
        // Summary:
        //     Full or truncated response to receive back.
        //
        // Value:
        //     The default value for the response is truncated.
        public ResponseFormat ResponseFormat { get; set; } = ResponseFormat.Truncated;
        //
        // Summary:
        //     Gets the YubiKeyApplication to which this command belongs.
        //
        // Value:
        //     YubiKeyApplication.Oath
        public YubiKeyApplication Application => YubiKeyApplication.Oath;
        //
        // Summary:
        //     Constructs an instance of the Yubico.YubiKey.Oath.Commands.CalculateCredentialCommand
        //     class. The ResponseFormat will be set to its default value which is truncated.
        public OverriddenCalculateCredentialCommand()
        {
        }

        public OverriddenCalculateCredentialCommand(Credential credential, ResponseFormat responseFormat)
        {
            if ((object)credential == null)
            {
                throw new ArgumentNullException("credential");
            }

            Credential = credential;
            ResponseFormat = responseFormat;
        }

        public CommandApdu CreateCommandApdu()
        {
            if ((object)Credential == null)
            {
                throw new InvalidOperationException("InvalidCredential");
            }

            if (!Credential!.Type.HasValue)
            {
                throw new InvalidOperationException("InvalidCredentialType");
            }

            byte[] bytes = Encoding.UTF8.GetBytes(Credential!.Name);
            TlvWriter tlvWriter = new TlvWriter();
            tlvWriter.WriteValue(113, bytes);
            if (Credential!.Type == CredentialType.Totp)
            {
                tlvWriter.WriteValue(116, GenerateChallenge(Credential.Period));
            }
            else
            {
                tlvWriter.WriteValue(116, null);
            }

            return new CommandApdu
            {
                Ins = 162,
                P2 = (byte)ResponseFormat,
                Data = tlvWriter.Encode()
            };
        }

        /// <summary>
        /// Custom GenerateChallenge that actually gives the timestamp / period length
        /// </summary>
        private static byte[] GenerateChallenge(CredentialPeriod? period = CredentialPeriod.Period30)
        {
            if (period == null)
                period = CredentialPeriod.Period30;

            long numPeriods = DateTimeOffset.UtcNow.ToUnixTimeSeconds() / (int)period;
#if DEBUG
            Prompt.DebugLog($"Time periods - {numPeriods}");
#endif
            byte[] array = BitConverter.GetBytes(numPeriods);
            if (BitConverter.IsLittleEndian)
            {
                Array.Reverse(array);
            }
            return array;
        }

        public CalculateCredentialResponse CreateResponseForApdu(ResponseApdu responseApdu)
        {
            if ((object)Credential == null)
            {
                throw new InvalidOperationException("InvalidCredential");
            }

            return new CalculateCredentialResponse(responseApdu, Credential);
        }
    }
GregDomzalski commented 1 year ago

Thanks for sharing your solution! I really appreciate that.

We will work on releasing an update soon. ETA is another week or two for it to hit NuGet.

GregDomzalski commented 1 year ago

Hi @JonathonQuinn - sorry this took longer than two weeks. We have issued a 1.4.1 patch release yesterday, and this issue should hopefully now be resolved. Please let me know if you are still encountering problems.

JonathonQuinn commented 1 year ago

Hi thanks Greg, thats working on our side.