Skip to content

Commit

Permalink
Improve "unsafe" code
Browse files Browse the repository at this point in the history
  • Loading branch information
@dorssel
dorssel committed Dec 31, 2024
1 parent b7a3640 commit 6e1a4fe
Show file tree
Hide file tree
Showing 3 changed files with 80 additions and 79 deletions.
2 changes: 1 addition & 1 deletion UnitTests/Internal.UnitTests/OpaqueStructuresTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
namespace Internal.UnitTests;

[TestClass]
sealed unsafe class OpaqueStructuresTests
sealed class OpaqueStructuresTests
{
[TestMethod]
public void XMSS_SIGNING_CONTEXT_SIZE()
Expand Down
4 changes: 2 additions & 2 deletions Xmss/Internal/Errors.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,9 @@ static partial class UnsafeNativeMethods
{
[LibraryImport("xmss", StringMarshalling = StringMarshalling.Custom, StringMarshallingCustomType = typeof(ErrorStringMarshaller))]
[DefaultDllImportSearchPaths(DllImportSearchPath.AssemblyDirectory)]
internal static unsafe partial string xmss_error_to_name(XmssError error);
internal static partial string xmss_error_to_name(XmssError error);

[LibraryImport("xmss", StringMarshalling = StringMarshalling.Custom, StringMarshallingCustomType = typeof(ErrorStringMarshaller))]
[DefaultDllImportSearchPaths(DllImportSearchPath.AssemblyDirectory)]
internal static unsafe partial string xmss_error_to_description(XmssError error);
internal static partial string xmss_error_to_description(XmssError error);
}
153 changes: 77 additions & 76 deletions Xmss/Xmss.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -227,59 +227,56 @@ public void GeneratePrivateKey(IXmssStateManager stateManager, XmssParameterSet

XmssError result;

// Step 3: Create key.

using var keyContext = new CriticalXmssKeyContextHandle();
using var signingContext = new CriticalXmssSigningContextHandle();
using var privateKeyStatelessBlob = new CriticalXmssPrivateKeyStatelessBlobHandle();
using var privateKeyStatefulBlob = new CriticalXmssPrivateKeyStatefulBlobHandle();

unsafe
{
// Step 3: Create key.

using var keyContext = new CriticalXmssKeyContextHandle();
using var signingContext = new CriticalXmssSigningContextHandle();
{
result = UnsafeNativeMethods.xmss_context_initialize(ref signingContext.AsPointerRef(), (XmssParameterSetOID)parameterSet,
&UnmanagedFunctions.Realloc, &UnmanagedFunctions.Free, &UnmanagedFunctions.Zeroize);
XmssException.ThrowIfNotOkay(result);
}
result = UnsafeNativeMethods.xmss_context_initialize(ref signingContext.AsPointerRef(), (XmssParameterSetOID)parameterSet,
&UnmanagedFunctions.Realloc, &UnmanagedFunctions.Free, &UnmanagedFunctions.Zeroize);
XmssException.ThrowIfNotOkay(result);

using var privateKeyStatelessBlob = new CriticalXmssPrivateKeyStatelessBlobHandle();
using var privateKeyStatefulBlob = new CriticalXmssPrivateKeyStatefulBlobHandle();
{
var allRandomPtr = stackalloc byte[96 + 32];
var allRandom = new Span<byte>(allRandomPtr, 96 + 32);
var allRandomPtr = stackalloc byte[96 + 32];
var allRandom = new Span<byte>(allRandomPtr, 96 + 32);

RandomNumberGenerator.Fill(allRandom);
RandomNumberGenerator.Fill(allRandom);

XmssBuffer secure_random = new() { data = allRandomPtr, data_size = 96 };
XmssBuffer random = new() { data = allRandomPtr + 96, data_size = 32 };
XmssBuffer secure_random = new() { data = allRandomPtr, data_size = 96 };
XmssBuffer random = new() { data = allRandomPtr + 96, data_size = 32 };

result = UnsafeNativeMethods.xmss_generate_private_key(ref keyContext.AsPointerRef(), ref privateKeyStatelessBlob.AsPointerRef(),
ref privateKeyStatefulBlob.AsPointerRef(), secure_random, enableIndexObfuscation
? XmssIndexObfuscationSetting.XMSS_INDEX_OBFUSCATION_ON : XmssIndexObfuscationSetting.XMSS_INDEX_OBFUSCATION_OFF,
random, signingContext.AsRef());
XmssException.ThrowIfNotOkay(result);
result = UnsafeNativeMethods.xmss_generate_private_key(ref keyContext.AsPointerRef(), ref privateKeyStatelessBlob.AsPointerRef(),
ref privateKeyStatefulBlob.AsPointerRef(), secure_random, enableIndexObfuscation
? XmssIndexObfuscationSetting.XMSS_INDEX_OBFUSCATION_ON : XmssIndexObfuscationSetting.XMSS_INDEX_OBFUSCATION_OFF,
random, signingContext.AsRef());
XmssException.ThrowIfNotOkay(result);

CryptographicOperations.ZeroMemory(allRandom);
}
CryptographicOperations.ZeroMemory(allRandom);
}

// Step 4: Store state (failure erases any partial storage, then throws).
// Step 4: Store state (failure erases any partial storage, then throws).

try
{
wrappedStateManager.Store(XmssKeyPart.PrivateStateless, privateKeyStatelessBlob.Data);
wrappedStateManager.Store(XmssKeyPart.PrivateStateful, privateKeyStatefulBlob.Data);
}
catch (XmssStateManagerException ex)
{
wrappedStateManager.DeleteAllAfterFailure(ex);
throw;
}
try
{
wrappedStateManager.Store(XmssKeyPart.PrivateStateless, privateKeyStatelessBlob.Data);
wrappedStateManager.Store(XmssKeyPart.PrivateStateful, privateKeyStatefulBlob.Data);
}
catch (XmssStateManagerException ex)
{
wrappedStateManager.DeleteAllAfterFailure(ex);
throw;
}

// Step 5: Replace KeyContext.
// Step 5: Replace KeyContext.

ResetState();
ParameterSet = parameterSet;
PrivateKey = new(wrappedStateManager);
PrivateKey.KeyContext.SwapWith(keyContext);
PrivateKey.StatefulBlob.SwapWith(privateKeyStatefulBlob);
}
ResetState();
ParameterSet = parameterSet;
PrivateKey = new(wrappedStateManager);
PrivateKey.KeyContext.SwapWith(keyContext);
PrivateKey.StatefulBlob.SwapWith(privateKeyStatefulBlob);
}

/// <summary>
Expand All @@ -296,57 +293,62 @@ public void ImportPrivateKey(IXmssStateManager stateManager)

XmssError result;

unsafe
using var privateKeyStatefulBlob = CriticalXmssPrivateKeyStatefulBlobHandle.Alloc();
using var privateKeyStatelessBlob = CriticalXmssPrivateKeyStatelessBlobHandle.Alloc();
wrappedStateManager.Load(XmssKeyPart.PrivateStateless, privateKeyStatelessBlob.Data);
wrappedStateManager.Load(XmssKeyPart.PrivateStateful, privateKeyStatefulBlob.Data);

foreach (var oid in Enum.GetValues<XmssParameterSetOID>())
{
using var privateKeyStatefulBlob = CriticalXmssPrivateKeyStatefulBlobHandle.Alloc();
using var privateKeyStatelessBlob = CriticalXmssPrivateKeyStatelessBlobHandle.Alloc();
wrappedStateManager.Load(XmssKeyPart.PrivateStateless, privateKeyStatelessBlob.Data);
wrappedStateManager.Load(XmssKeyPart.PrivateStateful, privateKeyStatefulBlob.Data);
using var keyContext = new CriticalXmssKeyContextHandle();
using var signingContext = new CriticalXmssSigningContextHandle();

foreach (var oid in Enum.GetValues<XmssParameterSetOID>())
unsafe

Check notice

Code scanning / devskim

The unsafe keyword denotes an unsafe context, which is required for any operation involving pointers. Unsafe code in is not necessarily dangerous; it is just code whose safety cannot be verified by the CLR. Note

Review unsafe code
{
using var keyContext = new CriticalXmssKeyContextHandle();
using var signingContext = new CriticalXmssSigningContextHandle();
result = UnsafeNativeMethods.xmss_context_initialize(ref signingContext.AsPointerRef(), oid,
&UnmanagedFunctions.Realloc, &UnmanagedFunctions.Free, &UnmanagedFunctions.Zeroize);
XmssException.ThrowIfNotOkay(result);

result = UnsafeNativeMethods.xmss_load_private_key(ref keyContext.AsPointerRef(),
privateKeyStatelessBlob.AsRef(), privateKeyStatefulBlob.AsRef(), signingContext.AsRef());
if (result == XmssError.XMSS_OKAY)
}
if (result == XmssError.XMSS_OKAY)
{
ResetState();
ParameterSet = (XmssParameterSet)oid;
PrivateKey = new(wrappedStateManager);
PrivateKey.KeyContext.SwapWith(keyContext);
PrivateKey.StatefulBlob.SwapWith(privateKeyStatefulBlob);

// Now try to load the internal public key part, but failure is not fatal.
try
{
ResetState();
ParameterSet = (XmssParameterSet)oid;
PrivateKey = new(wrappedStateManager);
PrivateKey.KeyContext.SwapWith(keyContext);
PrivateKey.StatefulBlob.SwapWith(privateKeyStatefulBlob);

// Now try to load the internal public key part, but failure is not fatal.
try
{
try
using var publicKeyInternalBlob = CriticalXmssPublicKeyInternalBlobHandle.Alloc(XmssCacheType.XMSS_CACHE_TOP, 0,
ParameterSet);
wrappedStateManager.Load(XmssKeyPart.Public, publicKeyInternalBlob.Data);

unsafe

Check notice

Code scanning / devskim

The unsafe keyword denotes an unsafe context, which is required for any operation involving pointers. Unsafe code in is not necessarily dangerous; it is just code whose safety cannot be verified by the CLR. Note

Review unsafe code
{
using var publicKeyInternalBlob = CriticalXmssPublicKeyInternalBlobHandle.Alloc(XmssCacheType.XMSS_CACHE_TOP, 0,
ParameterSet);
wrappedStateManager.Load(XmssKeyPart.Public, publicKeyInternalBlob.Data);
// The cache will be automatically freed with the key context; we don't need it.
XmssInternalCache* cache = null;
result = UnsafeNativeMethods.xmss_load_public_key(ref cache, ref PrivateKey.KeyContext.AsRef(),
publicKeyInternalBlob.AsRef());
XmssException.ThrowIfNotOkay(result);

result = UnsafeNativeMethods.xmss_export_public_key(out PublicKey, PrivateKey.KeyContext.AsRef());
XmssException.ThrowIfNotOkay(result);
HasPublicKey = true;
}
catch (Exception ex)
{
throw new IgnoreException(ex);
}
result = UnsafeNativeMethods.xmss_export_public_key(out PublicKey, PrivateKey.KeyContext.AsRef());
XmssException.ThrowIfNotOkay(result);
HasPublicKey = true;
}
catch (Exception ex)
{
throw new IgnoreException(ex);
}
catch (IgnoreException) { }
return;
}
catch (IgnoreException) { }

return;
}

// None of the OIDs worked.
Expand Down Expand Up @@ -598,11 +600,10 @@ Task OptionalDelayTask()
result = UnsafeNativeMethods.xmss_finish_calculate_public_key(ref publicKeyInternalBlob.AsPointerRef(),
ref keyGenerationContext.AsPointerRef(), ref PrivateKey.KeyContext.AsRef());
XmssException.ThrowIfNotOkay(result);

result = UnsafeNativeMethods.xmss_export_public_key(out PublicKey, PrivateKey.KeyContext.AsRef());
XmssException.ThrowIfNotOkay(result);
HasPublicKey = true;
}
result = UnsafeNativeMethods.xmss_export_public_key(out PublicKey, PrivateKey.KeyContext.AsRef());
XmssException.ThrowIfNotOkay(result);
HasPublicKey = true;

// NOTE: our KeyContext and StateManager are now out of sync, but that does not affect security (only the public part)

Expand Down

0 comments on commit 6e1a4fe

Please sign in to comment.