Skip to content

Commit 786c5e9

Browse files
DennisDyalloclaude
andcommitted
fix(fido2,core): apply previewSign code-audit findings
Three audit fixes plus a regression test for the new precondition guard. All four are concrete, well-scoped changes from Plans/codeaudit-previewsign-port.md. 1. Crypto leak (HIGH): ArkgPrimitivesOpenSsl.BlBlindPublicKey now wraps its body in try/finally and zeroes the blinding scalar tauBytes via CryptographicOperations.ZeroMemory. Brings tauBytes in line with the other secret-bearing arrays in the same file (ephSkBytes, kPrime, mk, ikmTau, prk, full) which were already zeroed. 2. API footgun (HIGH): GetAssertionParameters.AddPreviewSignByCredentialExtension now throws InvalidOperationException with an actionable message when the parent's allow-list is null/empty. Without this guard, customers got a cryptic firmware "option or extension invalid" error from the YubiKey. The required precondition is enforced at the protocol level by the firmware (and documented in python-fido2's reference impl); the SDK should fail fast and explain how to fix the call site. 3. Documentation lies (LOW): MakeCredentialData.GetPreviewSignGeneratedKey and AuthenticatorData.GetPreviewSignSignature each had stale <exception cref="NotImplementedException"> tags inherited from a draft that were no longer accurate. Deleted. 4. Regression test: PreviewSignExtensionTests now asserts AddPreviewSignByCredentialExtension throws InvalidOperationException with "AllowCredential" in the message when the allow-list is empty. New helper BuildAuthenticatorInfoWithPreviewSign supports the test without hardware. Verification: Yubico.YubiKey.UnitTests: 3588 PASS (3587 baseline + 1 new test) Yubico.Core.UnitTests: 488 PASS / 21 SKIP (baseline match) Build: 0 warnings, 0 errors 3 ARKG NativeShims tests still fail with EntryPointNotFoundException for Native_EC_POINT_is_on_curve — expected interim state until the Yubico.NativeShims 1.16.1-prerelease.20260428.1 NuGet package lands. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent b3bf3e8 commit 786c5e9

5 files changed

Lines changed: 80 additions & 24 deletions

File tree

Yubico.Core/src/Yubico/Core/Cryptography/ArkgPrimitivesOpenSsl.cs

Lines changed: 25 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -213,26 +213,33 @@ private static BigInteger BlPrf(byte[] ikmTau, byte[] ctx)
213213
private static byte[] BlBlindPublicKey(byte[] pkBl, BigInteger tau)
214214
{
215215
byte[] tauBytes = ScalarToBytes(tau);
216-
using SafeEcGroup group = NativeMethods.EcGroupNewByCurveName(
217-
ECCurve.NamedCurves.nistP256.ToSslCurveId());
218-
using SafeEcPoint pkBlPoint = SecToPoint(group, pkBl);
219-
using SafeEcPoint result = NativeMethods.EcPointNew(group);
220-
using SafeBigNum tauBn = NativeMethods.BnBinaryToBigNum(tauBytes);
221-
using SafeBigNum oneBn = NativeMethods.BnBinaryToBigNum(new byte[] { 0x01 });
222-
223-
// r = tau*G + 1*pkBl => r = pkBl + tau*G.
224-
int rc = NativeMethods.EcPointMul(
225-
group,
226-
result,
227-
tauBn.DangerousGetHandle(),
228-
pkBlPoint.DangerousGetHandle(),
229-
oneBn.DangerousGetHandle());
230-
if (rc != 1)
216+
try
231217
{
232-
throw new CryptographicException("EC_POINT_mul failed in BlBlindPublicKey.");
233-
}
218+
using SafeEcGroup group = NativeMethods.EcGroupNewByCurveName(
219+
ECCurve.NamedCurves.nistP256.ToSslCurveId());
220+
using SafeEcPoint pkBlPoint = SecToPoint(group, pkBl);
221+
using SafeEcPoint result = NativeMethods.EcPointNew(group);
222+
using SafeBigNum tauBn = NativeMethods.BnBinaryToBigNum(tauBytes);
223+
using SafeBigNum oneBn = NativeMethods.BnBinaryToBigNum(new byte[] { 0x01 });
234224

235-
return PointToSec(group, result);
225+
// r = tau*G + 1*pkBl => r = pkBl + tau*G.
226+
int rc = NativeMethods.EcPointMul(
227+
group,
228+
result,
229+
tauBn.DangerousGetHandle(),
230+
pkBlPoint.DangerousGetHandle(),
231+
oneBn.DangerousGetHandle());
232+
if (rc != 1)
233+
{
234+
throw new CryptographicException("EC_POINT_mul failed in BlBlindPublicKey.");
235+
}
236+
237+
return PointToSec(group, result);
238+
}
239+
finally
240+
{
241+
CryptographicOperations.ZeroMemory(tauBytes);
242+
}
236243
}
237244

238245
// ---------------------------------------------------------------------

Yubico.YubiKey/src/Yubico/YubiKey/Fido2/AuthenticatorData.cs

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -401,9 +401,6 @@ public CredProtectPolicy GetCredProtectExtension()
401401
/// <returns>
402402
/// The signature bytes if the extension was used and returned data; otherwise, <c>null</c>.
403403
/// </returns>
404-
/// <exception cref="NotImplementedException">
405-
/// This method is not yet implemented.
406-
/// </exception>
407404
public byte[]? GetPreviewSignSignature()
408405
{
409406
if (!TryGetExtensionData(Fido2.Extensions.PreviewSign, out Memory<byte> encodedValue))

Yubico.YubiKey/src/Yubico/YubiKey/Fido2/GetAssertionParameters.cs

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -443,6 +443,10 @@ public void EncodeHmacSecretExtension(PinUvAuthProtocolBase authProtocol)
443443
/// <exception cref="NotSupportedException">
444444
/// The YubiKey does not support the previewSign extension.
445445
/// </exception>
446+
/// <exception cref="InvalidOperationException">
447+
/// The allow-list is null or empty. The previewSign extension requires at least one credential
448+
/// in the allow-list.
449+
/// </exception>
446450
public void AddPreviewSignByCredentialExtension(
447451
AuthenticatorInfo authenticatorInfo,
448452
PreviewSignDerivedKey derivedKey,
@@ -457,6 +461,13 @@ public void AddPreviewSignByCredentialExtension(
457461
throw new NotSupportedException(ExceptionMessages.NotSupportedByYubiKeyVersion);
458462
}
459463

464+
if (AllowList is null || AllowList.Count == 0)
465+
{
466+
throw new InvalidOperationException(
467+
"The previewSign extension's sign-by-credential mode requires at least one credential in the allow-list. " +
468+
"Call AllowCredential() with the credential ID returned from MakeCredential before invoking AddPreviewSignByCredentialExtension().");
469+
}
470+
460471
byte[] tbs;
461472
using (SHA256 sha = CryptographyProviders.Sha256Creator())
462473
{

Yubico.YubiKey/src/Yubico/YubiKey/Fido2/MakeCredentialData.cs

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -286,9 +286,6 @@ private bool ReadAttestation(CborMap<int> map)
286286
/// A <see cref="PreviewSignGeneratedKey"/> if the extension was used and returned
287287
/// data; otherwise, <c>null</c>.
288288
/// </returns>
289-
/// <exception cref="NotImplementedException">
290-
/// This method is not yet implemented.
291-
/// </exception>
292289
public PreviewSignGeneratedKey? GetPreviewSignGeneratedKey()
293290
{
294291
if (UnsignedExtensionOutputs is null

Yubico.YubiKey/tests/unit/Yubico/YubiKey/Fido2/PreviewSignExtensionTests.cs

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -179,6 +179,25 @@ public void AddPreviewSignByCredential_ThrowsWhenExtensionUnsupported()
179179
new byte[] { 0xAA }));
180180
}
181181

182+
[Fact]
183+
public void AddPreviewSignByCredential_ThrowsWhenAllowListEmpty()
184+
{
185+
var info = BuildAuthenticatorInfoWithPreviewSign();
186+
var parameters = new GetAssertionParameters(
187+
new RelyingParty("rp.example"),
188+
new byte[32]);
189+
190+
PreviewSignDerivedKey derivedKey = BuildDerivedKeyFixture();
191+
192+
var ex = Assert.Throws<InvalidOperationException>(
193+
() => parameters.AddPreviewSignByCredentialExtension(
194+
info,
195+
derivedKey,
196+
new byte[] { 0xAA }));
197+
198+
Assert.Contains("AllowCredential", ex.Message);
199+
}
200+
182201
// ==================================================================
183202
// Helpers
184203
// ==================================================================
@@ -403,6 +422,31 @@ private static AuthenticatorInfo BuildAuthenticatorInfoWithoutPreviewSign()
403422
return new AuthenticatorInfo(cbor.Encode());
404423
}
405424

425+
// Builds a synthetic AuthenticatorInfo WITH previewSign in its
426+
// Extensions list.
427+
private static AuthenticatorInfo BuildAuthenticatorInfoWithPreviewSign()
428+
{
429+
byte[] aaguid = new byte[16];
430+
var cbor = new CborWriter(CborConformanceMode.Ctap2Canonical, convertIndefiniteLengthEncodings: true);
431+
cbor.WriteStartMap(3);
432+
433+
cbor.WriteInt32(1);
434+
cbor.WriteStartArray(1);
435+
cbor.WriteTextString("FIDO_2_1");
436+
cbor.WriteEndArray();
437+
438+
cbor.WriteInt32(2);
439+
cbor.WriteStartArray(1);
440+
cbor.WriteTextString(Extensions.PreviewSign);
441+
cbor.WriteEndArray();
442+
443+
cbor.WriteInt32(3);
444+
cbor.WriteByteString(aaguid);
445+
446+
cbor.WriteEndMap();
447+
return new AuthenticatorInfo(cbor.Encode());
448+
}
449+
406450
private static byte[] BuildSec1Generator()
407451
{
408452
// Fixed P-256 generator — valid SEC1 point used as a placeholder

0 commit comments

Comments
 (0)