diff --git a/src/libraries/Common/src/Microsoft/Win32/SafeHandles/SafeLocalAllocHandle.cs b/src/libraries/Common/src/Microsoft/Win32/SafeHandles/SafeLocalAllocHandle.cs index 65ca127215cb8..94f23c5900f5a 100644 --- a/src/libraries/Common/src/Microsoft/Win32/SafeHandles/SafeLocalAllocHandle.cs +++ b/src/libraries/Common/src/Microsoft/Win32/SafeHandles/SafeLocalAllocHandle.cs @@ -21,6 +21,14 @@ internal static SafeLocalAllocHandle LocalAlloc(int cb) return h; } + internal static SafeLocalAllocHandle LocalAlloc(nuint cb) + { + var h = new SafeLocalAllocHandle(); + h.SetHandle(Marshal.AllocHGlobal(unchecked((nint)cb))); + h.Initialize(cb); + return h; + } + // 0 is an Invalid Handle internal SafeLocalAllocHandle(IntPtr handle) : base(true) { diff --git a/src/libraries/System.Diagnostics.EventLog/tests/System/Diagnostics/Reader/EventLogSessionTests.cs b/src/libraries/System.Diagnostics.EventLog/tests/System/Diagnostics/Reader/EventLogSessionTests.cs index 22ca9849f8dc2..4ddc9327996e6 100644 --- a/src/libraries/System.Diagnostics.EventLog/tests/System/Diagnostics/Reader/EventLogSessionTests.cs +++ b/src/libraries/System.Diagnostics.EventLog/tests/System/Diagnostics/Reader/EventLogSessionTests.cs @@ -8,6 +8,8 @@ using System.Security; using Xunit; +#pragma warning disable OBS0001 // SecureString ctors are obsolete + namespace System.Diagnostics.Tests { public class EventLogSessionTests : FileCleanupTestBase diff --git a/src/libraries/System.Diagnostics.Process/tests/ProcessStartInfoTests.cs b/src/libraries/System.Diagnostics.Process/tests/ProcessStartInfoTests.cs index 3636f8ea3fd4e..8911e326425ac 100644 --- a/src/libraries/System.Diagnostics.Process/tests/ProcessStartInfoTests.cs +++ b/src/libraries/System.Diagnostics.Process/tests/ProcessStartInfoTests.cs @@ -19,6 +19,8 @@ using Microsoft.Win32.SafeHandles; using Xunit; +#pragma warning disable OBS0001 // SecureString ctors are obsolete + namespace System.Diagnostics.Tests { public class ProcessStartInfoTests : ProcessTestBase diff --git a/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs b/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs index 167776c769ee1..620b80f253fec 100644 --- a/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs +++ b/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs @@ -20,6 +20,8 @@ using Xunit; using Xunit.Sdk; +#pragma warning disable OBS0001 // SecureString ctors are obsolete + namespace System.Diagnostics.Tests { public partial class ProcessTests : ProcessTestBase diff --git a/src/libraries/System.Management/src/System/Management/ManagementOptions.cs b/src/libraries/System.Management/src/System/Management/ManagementOptions.cs index 16012dee59487..a7c2752665675 100644 --- a/src/libraries/System.Management/src/System/Management/ManagementOptions.cs +++ b/src/libraries/System.Management/src/System/Management/ManagementOptions.cs @@ -5,6 +5,8 @@ using System.ComponentModel; using System.Security; +#pragma warning disable OBS0001 // SecureString ctor is obsolete + namespace System.Management { /// diff --git a/src/libraries/System.Net.Primitives/src/System/Net/NetworkCredential.cs b/src/libraries/System.Net.Primitives/src/System/Net/NetworkCredential.cs index 04186f27adbed..b138e0f8bd6c2 100644 --- a/src/libraries/System.Net.Primitives/src/System/Net/NetworkCredential.cs +++ b/src/libraries/System.Net.Primitives/src/System/Net/NetworkCredential.cs @@ -106,13 +106,8 @@ public SecureString SecurePassword { get { - string? str = _password as string; - if (str != null) - { - return MarshalToSecureString(str); - } - SecureString? sstr = _password as SecureString; - return sstr != null ? sstr.Copy() : new SecureString(); + return (_password as SecureString)?.Copy() + ?? MarshalToSecureString(_password as string); } set { @@ -175,8 +170,9 @@ private string MarshalToString(SecureString sstr) return result; } - private unsafe SecureString MarshalToSecureString(string str) + private unsafe SecureString MarshalToSecureString(string? str) { +#pragma warning disable OBS0001 // SecureString ctor is obsolete if (string.IsNullOrEmpty(str)) { return new SecureString(); @@ -186,6 +182,7 @@ private unsafe SecureString MarshalToSecureString(string str) { return new SecureString(ptr, str.Length); } +#pragma warning restore OBS0001 } } } diff --git a/src/libraries/System.Net.Primitives/tests/FunctionalTests/NetworkCredentialTest.cs b/src/libraries/System.Net.Primitives/tests/FunctionalTests/NetworkCredentialTest.cs index e8ad4f25ad931..62f27eb3ab96b 100644 --- a/src/libraries/System.Net.Primitives/tests/FunctionalTests/NetworkCredentialTest.cs +++ b/src/libraries/System.Net.Primitives/tests/FunctionalTests/NetworkCredentialTest.cs @@ -7,6 +7,8 @@ using System.Security; using Xunit; +#pragma warning disable OBS0001 // SecureString ctors are obsolete + namespace System.Net.Primitives.Functional.Tests { public static partial class NetworkCredentialTest diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/NegotiateStreamKerberosTest.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/NegotiateStreamKerberosTest.cs index 8b10562979898..ad0ad315a3fcc 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/NegotiateStreamKerberosTest.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/NegotiateStreamKerberosTest.cs @@ -14,9 +14,10 @@ using System.Text; using System.Threading; using System.Threading.Tasks; - using Xunit; +#pragma warning disable OBS0001 // SecureString ctors are obsolete + namespace System.Net.Security.Tests { using Configuration = System.Net.Test.Common.Configuration; diff --git a/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems b/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems index 6f2855c237f96..12a41237baff1 100644 --- a/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems +++ b/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems @@ -748,6 +748,7 @@ + @@ -1561,8 +1562,8 @@ + - @@ -1772,8 +1773,8 @@ + - diff --git a/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/ShroudedBuffer.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/ShroudedBuffer.Unix.cs new file mode 100644 index 0000000000000..8e84785ad6b86 --- /dev/null +++ b/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/ShroudedBuffer.Unix.cs @@ -0,0 +1,18 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +namespace System.Runtime.InteropServices +{ + internal sealed partial class ShroudedBufferHandle + { + internal IntPtr AllocateCore() + => Marshal.AllocHGlobal((nint)_cbData); + + private bool ReleaseHandleCore() + { + Marshal.FreeHGlobal(handle); + return true; + } + } +} diff --git a/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/ShroudedBuffer.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/ShroudedBuffer.Windows.cs new file mode 100644 index 0000000000000..8a5e212f1652d --- /dev/null +++ b/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/ShroudedBuffer.Windows.cs @@ -0,0 +1,55 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +namespace System.Runtime.InteropServices +{ + internal sealed partial class ShroudedBufferHandle + { + private static readonly IntPtr _hHeap = GetOrCreateHeap(); + + internal IntPtr AllocateCore() + => Interop.HeapAlloc(_hHeap, 0, _cbData); + + private static IntPtr GetOrCreateHeap() + { + // In 64-bit processes where the virtual address space is larger, + // we use our own private heap to store the shrouded data. This + // somewhat isolates the shrouded data's storage space from that of + // the rest of the application, providing limited mitigation of a + // use-after-free elsewhere in the application accidentally pointing + // to an address used by a shrouded buffer instance. + + IntPtr hHeap = (IntPtr.Size == 8) ? Interop.HeapCreate(0, 0, 0) : Interop.GetProcessHeap(); + if (hHeap == IntPtr.Zero) + { + Marshal.ThrowExceptionForHR(Marshal.GetHRForLastWin32Error()); + Environment.FailFast("Couldn't get heap information."); + } + + return hHeap; + } + + private bool ReleaseHandleCore() + { + return Interop.HeapFree(_hHeap, 0, handle); + } + + private static class Interop + { + private const string KERNEL32_LIB = "kernel32.dll"; + + [DllImport(KERNEL32_LIB, CallingConvention = CallingConvention.Winapi, SetLastError = true)] + internal static extern IntPtr GetProcessHeap(); + + [DllImport(KERNEL32_LIB, CallingConvention = CallingConvention.Winapi, SetLastError = true)] + internal static extern IntPtr HeapCreate(uint flOptions, nuint dwInitialSize, nuint dwMaximumSize); + + [DllImport(KERNEL32_LIB, CallingConvention = CallingConvention.Winapi, SetLastError = false)] + internal static extern IntPtr HeapAlloc(IntPtr hHeap, uint dwFlags, nuint dwBytes); + + [DllImport(KERNEL32_LIB, CallingConvention = CallingConvention.Winapi, SetLastError = true)] + internal static extern bool HeapFree(IntPtr hHeap, uint dwFlags, IntPtr lpMem); + } + } +} diff --git a/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/ShroudedBuffer.cs b/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/ShroudedBuffer.cs new file mode 100644 index 0000000000000..16032ca1504a6 --- /dev/null +++ b/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/ShroudedBuffer.cs @@ -0,0 +1,184 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using Internal.Runtime.CompilerServices; +using Microsoft.Win32.SafeHandles; + +namespace System.Runtime.InteropServices +{ + /* + * Represents data which should be "shrouded" from the rest of the application. + * Shrouded data is sensitive data which should be difficult to *accidentally* + * disclose, even accounting for some types of application bugs. For example, + * we don't want accidental ArrayPool misuse to disclose the contents of shrouded + * buffers. But there's no effort to thwart *intentional* disclosure of these + * contents, such as through a debugger or memory dump utility. + * + * Some design philosophies and mitigations provided by this type: + * + * 1) The public surface area of this API works solely in Span, not string + * or T[] or anything else. This gives the caller the ability to use pre- + * pinned memory if they so desire. + * + * 2) The virtual address space used to back the shrouded contents should be + * isolated from the virtual address space used by the rest of the + * framework or application code where practical. This helps avoid the + * case where the app has a dangling pointer or object reference that can + * be used to access the shrouded data. + * + * 3) The data is immutable once shrouded. If a caller wishes to mutate the + * contents, they must create a new instance. Immutability allows this + * object to be used by multiple callers simultaneously. (The Dispose + * method is not thread-safe.) + * + * 4) No reference is ever provided to the raw backing contents. This allows + * some future-proofing of the type, such as allowing the actual contents + * to be stored in a different process. (Think lsass / lsaiso.) + * + * *** THIS TYPE MAKES NO SECURITY GUARANTEES. *** + * + * This type is intended only to prevent accidental disclosure of the shrouded + * contents. If protection against an active adversary is warranted, the + * machine administrator should take additional steps such as: isolating the + * process handling the sensitive data into its own service account, enabling + * extra runtime safety checks, disabling memory dump collection, enabling OS- + * wide hardware-backed safety mechanisms, and restricting access to the host. + */ + public unsafe class ShroudedBuffer : IDisposable where T : unmanaged + { + private readonly ShroudedBufferHandle _hnd; + + /// + /// Creates a new from the provided contents. + /// + /// The contents to copy into the new instance. + /// + /// The newly-returned instance maintains its + /// own copy of the data separate from . + /// + public ShroudedBuffer(ReadOnlySpan contents) + { + // multiplication below will never overflow + _hnd = new ShroudedBufferHandle((nuint)Unsafe.SizeOf() * (uint)contents.Length); + Length = contents.Length; + + bool refAdded = false; + try + { + _hnd.DangerousAddRef(ref refAdded); + contents.CopyTo(new Span((void*)_hnd.DangerousGetHandle(), Length)); + } + finally + { + if (refAdded) + { + _hnd.DangerousRelease(); + } + } + } + + /// + /// Returns the length (in elements) of this buffer. + /// + public int Length { get; } + + /// + /// Copies the contents of this instance to + /// a destination buffer. + /// + /// + /// The destination buffer which should receive the contents. + /// This buffer must be at least elements in length. + /// + /// + /// 's length is smaller than . + /// + /// + /// This instance has already been disposed. + /// + public void CopyTo(Span destination) + { + bool refAdded = false; + try + { + _hnd.DangerousAddRef(ref refAdded); + ReadOnlySpan source = new ReadOnlySpan((void*)_hnd.DangerousGetHandle(), Length); + source.CopyTo(destination); + } + finally + { + if (refAdded) + { + _hnd.DangerousRelease(); + } + } + } + + internal ShroudedBuffer DeepClone() + { + bool refAdded = false; + try + { + _hnd.DangerousAddRef(ref refAdded); + return new ShroudedBuffer(new ReadOnlySpan((void*)_hnd.DangerousGetHandle(), Length)); + } + finally + { + if (refAdded) + { + _hnd.DangerousRelease(); + } + } + } + + /// + /// Disposes of this instance, including any unmanaged resources. + /// The contents will no longer be accessible once the instance is disposed. + /// + public void Dispose() + { + Dispose(true); + GC.SuppressFinalize(this); + } + + /// + /// Disposes of this instance, including any unmanaged resources. + /// The contents will no longer be accessible once the instance is disposed. + /// + /// + /// if this instance is being disposed through a call + /// to the method; if this instance + /// is being finalized. + /// + protected virtual void Dispose(bool disposing) + { + _hnd.Dispose(); + } + } + + internal sealed partial class ShroudedBufferHandle : SafeHandleZeroOrMinusOneIsInvalid + { + private readonly nuint _cbData; // used to clear memory before freeing + + // allocates memory; does not guarantee zero-init + internal ShroudedBufferHandle(nuint cbData) + : base(ownsHandle: true) + { + _cbData = cbData; + SetHandle(AllocateCore()); + + if (IsInvalid) + { + throw new OutOfMemoryException(); + } + } + + protected override unsafe bool ReleaseHandle() + { + // zero the memory before releasing it + SpanHelpers.ClearWithoutReferences(ref Unsafe.AsRef((void*)handle), _cbData); + return ReleaseHandleCore(); + } + } +} diff --git a/src/libraries/System.Private.CoreLib/src/System/Security/SecureString.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/Security/SecureString.Unix.cs deleted file mode 100644 index ca17a89876c42..0000000000000 --- a/src/libraries/System.Private.CoreLib/src/System/Security/SecureString.Unix.cs +++ /dev/null @@ -1,33 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. -// See the LICENSE file in the project root for more information. - -namespace System.Security -{ - // SecureString attempts to provide a defense-in-depth solution. - // - // On Windows, this is done with several mechanisms: - // 1. keeping the data in unmanaged memory so that copies of it aren't implicitly made by the GC moving it around - // 2. zero'ing out that unmanaged memory so that the string is reliably removed from memory when done with it - // 3. encrypting the data while it's not being used (it's unencrypted to manipulate and use it) - // - // On Unix, we do 1 and 2, but we don't do 3 as there's no CryptProtectData equivalent. - - public sealed partial class SecureString - { - private static int GetAlignedByteSize(int length) - { - return Math.Max(length, 1) * sizeof(char); - } - - private void ProtectMemory() - { - _encrypted = true; - } - - private void UnprotectMemory() - { - _encrypted = false; - } - } -} diff --git a/src/libraries/System.Private.CoreLib/src/System/Security/SecureString.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/Security/SecureString.Windows.cs deleted file mode 100644 index b2ff3196ef6d4..0000000000000 --- a/src/libraries/System.Private.CoreLib/src/System/Security/SecureString.Windows.cs +++ /dev/null @@ -1,51 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. -// See the LICENSE file in the project root for more information. - -using System.Diagnostics; -using System.Runtime.InteropServices; -using System.Security.Cryptography; - -namespace System.Security -{ - public sealed partial class SecureString - { - private static int GetAlignedByteSize(int length) - { - int byteSize = Math.Max(length, 1) * sizeof(char); - - const int blockSize = (int)Interop.Crypt32.CRYPTPROTECTMEMORY_BLOCK_SIZE; - return ((byteSize + (blockSize - 1)) / blockSize) * blockSize; - } - - private void ProtectMemory() - { - Debug.Assert(_buffer != null); - Debug.Assert(!_buffer.IsInvalid, "Invalid buffer!"); - - if (_decryptedLength != 0 && - !_encrypted && - !Interop.Crypt32.CryptProtectMemory(_buffer, (uint)_buffer.ByteLength, Interop.Crypt32.CRYPTPROTECTMEMORY_SAME_PROCESS)) - { - throw new CryptographicException(Marshal.GetLastWin32Error()); - } - - _encrypted = true; - } - - private void UnprotectMemory() - { - Debug.Assert(_buffer != null); - Debug.Assert(!_buffer.IsInvalid, "Invalid buffer!"); - - if (_decryptedLength != 0 && - _encrypted && - !Interop.Crypt32.CryptUnprotectMemory(_buffer, (uint)_buffer.ByteLength, Interop.Crypt32.CRYPTPROTECTMEMORY_SAME_PROCESS)) - { - throw new CryptographicException(Marshal.GetLastWin32Error()); - } - - _encrypted = false; - } - } -} diff --git a/src/libraries/System.Private.CoreLib/src/System/Security/SecureString.cs b/src/libraries/System.Private.CoreLib/src/System/Security/SecureString.cs index 35deabaf8ad82..8db8020499825 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Security/SecureString.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Security/SecureString.cs @@ -3,18 +3,17 @@ // See the LICENSE file in the project root for more information. using System.Diagnostics; +using System.Diagnostics.CodeAnalysis; using System.Runtime.InteropServices; using System.Threading; namespace System.Security { - public sealed partial class SecureString : IDisposable + public unsafe sealed partial class SecureString : IDisposable { private const int MaxLength = 65536; private readonly object _methodLock = new object(); - private UnmanagedBuffer? _buffer; - private int _decryptedLength; - private bool _encrypted; + private ShroudedBuffer? _buffer; private bool _readOnly; public SecureString() @@ -23,7 +22,7 @@ public SecureString() } [CLSCompliant(false)] - public unsafe SecureString(char* value, int length) + public SecureString(char* value, int length) { if (value == null) { @@ -43,33 +42,12 @@ public unsafe SecureString(char* value, int length) private void Initialize(ReadOnlySpan value) { - _buffer = UnmanagedBuffer.Allocate(GetAlignedByteSize(value.Length)); - _decryptedLength = value.Length; - - SafeBuffer? bufferToRelease = null; - try - { - Span span = AcquireSpan(ref bufferToRelease); - value.CopyTo(span); - } - finally - { - ProtectMemory(); - bufferToRelease?.DangerousRelease(); - } + _buffer = new ShroudedBuffer(value); } - private SecureString(SecureString str) + private SecureString(ShroudedBuffer ownedBuffer) { - Debug.Assert(str._buffer != null, "Expected other SecureString's buffer to be non-null"); - Debug.Assert(str._encrypted, "Expected to be used only on encrypted SecureStrings"); - - _buffer = UnmanagedBuffer.Allocate((int)str._buffer.ByteLength); - Debug.Assert(_buffer != null); - UnmanagedBuffer.Copy(str._buffer, _buffer, str._buffer.ByteLength); - - _decryptedLength = str._decryptedLength; - _encrypted = str._encrypted; + _buffer = ownedBuffer; } public int Length @@ -77,28 +55,8 @@ public int Length get { EnsureNotDisposed(); - return Volatile.Read(ref _decryptedLength); - } - } - - private void EnsureCapacity(int capacity) - { - if (capacity > MaxLength) - { - throw new ArgumentOutOfRangeException(nameof(capacity), SR.ArgumentOutOfRange_Capacity); - } - - Debug.Assert(_buffer != null); - if ((uint)capacity * sizeof(char) <= _buffer.ByteLength) - { - return; + return _buffer.Length; } - - UnmanagedBuffer oldBuffer = _buffer; - UnmanagedBuffer newBuffer = UnmanagedBuffer.Allocate(GetAlignedByteSize(capacity)); - UnmanagedBuffer.Copy(oldBuffer, newBuffer, (uint)_decryptedLength * sizeof(char)); - _buffer = newBuffer; - oldBuffer.Dispose(); } public void AppendChar(char c) @@ -108,24 +66,19 @@ public void AppendChar(char c) EnsureNotDisposed(); EnsureNotReadOnly(); - Debug.Assert(_buffer != null); - - SafeBuffer? bufferToRelease = null; - - try + Span newCharBuffer = GetNewCharBuffer(_buffer.Length + 1); + fixed (char* pChars = newCharBuffer) // prevent the GC from moving this array { - UnprotectMemory(); - - EnsureCapacity(_decryptedLength + 1); - - Span span = AcquireSpan(ref bufferToRelease); - span[_decryptedLength] = c; - _decryptedLength++; - } - finally - { - ProtectMemory(); - bufferToRelease?.DangerousRelease(); + try + { + _buffer.CopyTo(newCharBuffer); + newCharBuffer[^1] = c; + SetNewBufferUnderLock(newCharBuffer); + } + finally + { + newCharBuffer.Clear(); + } } } } @@ -138,20 +91,7 @@ public void Clear() EnsureNotDisposed(); EnsureNotReadOnly(); - Debug.Assert(_buffer != null); - - _decryptedLength = 0; - - SafeBuffer? bufferToRelease = null; - try - { - Span span = AcquireSpan(ref bufferToRelease); - span.Clear(); - } - finally - { - bufferToRelease?.DangerousRelease(); - } + SetNewBufferUnderLock(ReadOnlySpan.Empty); } } @@ -161,7 +101,7 @@ public SecureString Copy() lock (_methodLock) { EnsureNotDisposed(); - return new SecureString(this); + return new SecureString(_buffer.DeepClone()); } } @@ -177,37 +117,42 @@ public void Dispose() } } + private char[] GetNewCharBuffer(int capacity) + { + if (capacity > MaxLength) + { + throw new ArgumentOutOfRangeException(nameof(capacity), SR.ArgumentOutOfRange_Capacity); + } + + return new char[capacity]; + } + public void InsertAt(int index, char c) { lock (_methodLock) { - if (index < 0 || index > _decryptedLength) - { - throw new ArgumentOutOfRangeException(nameof(index), SR.ArgumentOutOfRange_IndexString); - } - EnsureNotDisposed(); EnsureNotReadOnly(); - Debug.Assert(_buffer != null); - - SafeBuffer? bufferToRelease = null; - - try + if (index < 0 || index > _buffer.Length) { - UnprotectMemory(); - - EnsureCapacity(_decryptedLength + 1); - - Span span = AcquireSpan(ref bufferToRelease); - span.Slice(index, _decryptedLength - index).CopyTo(span.Slice(index + 1)); - span[index] = c; - _decryptedLength++; + throw new ArgumentOutOfRangeException(nameof(index), SR.ArgumentOutOfRange_IndexString); } - finally + + Span charBuffer = GetNewCharBuffer(_buffer.Length + 1); + fixed (char* pChars = charBuffer) // prevent the GC from moving this array { - ProtectMemory(); - bufferToRelease?.DangerousRelease(); + try + { + _buffer.CopyTo(charBuffer); + charBuffer[index..^1].CopyTo(charBuffer.Slice(index + 1)); + charBuffer[index] = c; + SetNewBufferUnderLock(charBuffer); + } + finally + { + charBuffer.Clear(); + } } } } @@ -228,30 +173,27 @@ public void RemoveAt(int index) { lock (_methodLock) { - if (index < 0 || index >= _decryptedLength) - { - throw new ArgumentOutOfRangeException(nameof(index), SR.ArgumentOutOfRange_IndexString); - } - EnsureNotDisposed(); EnsureNotReadOnly(); - Debug.Assert(_buffer != null); - - SafeBuffer? bufferToRelease = null; - - try + if (index < 0 || index >= _buffer.Length) { - UnprotectMemory(); - - Span span = AcquireSpan(ref bufferToRelease); - span.Slice(index + 1, _decryptedLength - (index + 1)).CopyTo(span.Slice(index)); - _decryptedLength--; + throw new ArgumentOutOfRangeException(nameof(index), SR.ArgumentOutOfRange_IndexString); } - finally + + Span charBuffer = GetNewCharBuffer(_buffer.Length); + fixed (char* pChars = charBuffer) // prevent the GC from moving this array { - ProtectMemory(); - bufferToRelease?.DangerousRelease(); + try + { + _buffer.CopyTo(charBuffer); + charBuffer.Slice(index + 1).CopyTo(charBuffer.Slice(index)); + SetNewBufferUnderLock(charBuffer[..^1]); + } + finally + { + charBuffer.Clear(); + } } } } @@ -260,43 +202,36 @@ public void SetAt(int index, char c) { lock (_methodLock) { - if (index < 0 || index >= _decryptedLength) - { - throw new ArgumentOutOfRangeException(nameof(index), SR.ArgumentOutOfRange_IndexString); - } - EnsureNotDisposed(); EnsureNotReadOnly(); - Debug.Assert(_buffer != null); - - SafeBuffer? bufferToRelease = null; - - try + if (index < 0 || index >= _buffer.Length) { - UnprotectMemory(); - - Span span = AcquireSpan(ref bufferToRelease); - span[index] = c; + throw new ArgumentOutOfRangeException(nameof(index), SR.ArgumentOutOfRange_IndexString); } - finally + + Span charBuffer = GetNewCharBuffer(_buffer.Length); + fixed (char* pChars = charBuffer) // prevent the GC from moving this array { - ProtectMemory(); - bufferToRelease?.DangerousRelease(); + try + { + _buffer.CopyTo(charBuffer); + charBuffer[index] = c; + SetNewBufferUnderLock(charBuffer); + } + finally + { + charBuffer.Clear(); + } } } } - private unsafe Span AcquireSpan(ref SafeBuffer? bufferToRelease) + private void SetNewBufferUnderLock(ReadOnlySpan newContents) { - SafeBuffer buffer = _buffer!; - - bool ignore = false; - buffer.DangerousAddRef(ref ignore); - - bufferToRelease = buffer; - - return new Span((byte*)buffer.DangerousGetHandle(), (int)(buffer.ByteLength / 2)); + ShroudedBuffer? oldBuffer = _buffer; + _buffer = new ShroudedBuffer(newContents); + oldBuffer?.Dispose(); } private void EnsureNotReadOnly() @@ -307,6 +242,7 @@ private void EnsureNotReadOnly() } } + [MemberNotNull(nameof(_buffer))] private void EnsureNotDisposed() { if (_buffer == null) @@ -315,27 +251,21 @@ private void EnsureNotDisposed() } } - internal unsafe IntPtr MarshalToBSTR() + internal IntPtr MarshalToBSTR() { lock (_methodLock) { EnsureNotDisposed(); - UnprotectMemory(); - - SafeBuffer? bufferToRelease = null; IntPtr ptr = IntPtr.Zero; - int length = 0; + int length = _buffer.Length; try { - Span span = AcquireSpan(ref bufferToRelease); - - length = _decryptedLength; ptr = Marshal.AllocBSTR(length); - span.Slice(0, length).CopyTo(new Span((void*)ptr, length)); + _buffer.CopyTo(new Span((void*)ptr, length)); IntPtr result = ptr; - ptr = IntPtr.Zero; + ptr = IntPtr.Zero; // so we don't free the new BSTR return result; } finally @@ -346,135 +276,103 @@ internal unsafe IntPtr MarshalToBSTR() new Span((void*)ptr, length).Clear(); Marshal.FreeBSTR(ptr); } - - ProtectMemory(); - bufferToRelease?.DangerousRelease(); } } } - internal unsafe IntPtr MarshalToString(bool globalAlloc, bool unicode) + internal IntPtr MarshalToString(bool globalAlloc, bool unicode) { lock (_methodLock) { EnsureNotDisposed(); - UnprotectMemory(); + // Always start by getting the contents of the string as unicode - SafeBuffer? bufferToRelease = null; - IntPtr ptr = IntPtr.Zero; - int byteLength = 0; - try + IntPtr ptrContentsAsUnicode = MarshalToStringUnicodeUnderLock(globalAlloc); // unmanaged buffer is terminated by a null char + if (unicode) { - Span span = AcquireSpan(ref bufferToRelease).Slice(0, _decryptedLength); + return ptrContentsAsUnicode; + } - if (unicode) - { - byteLength = (span.Length + 1) * sizeof(char); - } - else - { - byteLength = Marshal.GetAnsiStringByteCount(span); - } + // If we reached this point, we need to convert to ANSI. - if (globalAlloc) - { - ptr = Marshal.AllocHGlobal(byteLength); - } - else - { - ptr = Marshal.AllocCoTaskMem(byteLength); - } + Span spanContentsAsUnicode = new Span((void*)ptrContentsAsUnicode, _buffer.Length); // span *does not* encapsulate terminating null + int ansiLength = 0; + IntPtr ptrContentsAsAnsi = IntPtr.Zero; - if (unicode) - { - Span resultSpan = new Span((void*)ptr, byteLength / sizeof(char)); - span.CopyTo(resultSpan); - resultSpan[resultSpan.Length - 1] = '\0'; - } - else - { - Marshal.GetAnsiStringBytes(span, new Span((void*)ptr, byteLength)); - } + try + { + ansiLength = Marshal.GetAnsiStringByteCount(spanContentsAsUnicode); // byte count includes room for terminating null + ptrContentsAsAnsi = (globalAlloc) ? Marshal.AllocHGlobal(ansiLength) : Marshal.AllocCoTaskMem(ansiLength); + Marshal.GetAnsiStringBytes(spanContentsAsUnicode, new Span((void*)ptrContentsAsAnsi, ansiLength)); // auto-appends null terminator - IntPtr result = ptr; - ptr = IntPtr.Zero; + IntPtr result = ptrContentsAsAnsi; + ptrContentsAsAnsi = IntPtr.Zero; // so we don't free the newly allocated memory return result; } finally { - // If we failed for any reason, free the new buffer - if (ptr != IntPtr.Zero) + // Always free the unneeded Unicode buffer + spanContentsAsUnicode.Clear(); + if (globalAlloc) { - new Span((void*)ptr, byteLength).Clear(); + Marshal.FreeHGlobal(ptrContentsAsUnicode); + } + else + { + Marshal.FreeCoTaskMem(ptrContentsAsUnicode); + } + // If we failed for any reason, free the ANSI buffer + if (ptrContentsAsAnsi != IntPtr.Zero) + { + new Span((void*)ptrContentsAsAnsi, ansiLength).Clear(); if (globalAlloc) { - Marshal.FreeHGlobal(ptr); + Marshal.FreeHGlobal(ptrContentsAsAnsi); } else { - Marshal.FreeCoTaskMem(ptr); + Marshal.FreeCoTaskMem(ptrContentsAsAnsi); } } - - ProtectMemory(); - bufferToRelease?.DangerousRelease(); } } } - /// SafeBuffer for managing memory meant to be kept confidential. - private sealed class UnmanagedBuffer : SafeBuffer + private IntPtr MarshalToStringUnicodeUnderLock(bool globalAlloc) { - // A local copy of byte length to be able to access it in ReleaseHandle without the risk of throwing exceptions - private int _byteLength; - - private UnmanagedBuffer() : base(true) { } + Debug.Assert(_buffer != null); - public static UnmanagedBuffer Allocate(int byteLength) + IntPtr ptr = IntPtr.Zero; + int length = _buffer.Length; + try { - Debug.Assert(byteLength >= 0); - UnmanagedBuffer buffer = new UnmanagedBuffer(); - buffer.SetHandle(Marshal.AllocHGlobal(byteLength)); - buffer.Initialize((ulong)byteLength); - buffer._byteLength = byteLength; - return buffer; + nuint byteLength = (nuint)sizeof(char) * (uint)checked(length + 1); // includes room for terminating null + ptr = (globalAlloc) ? Marshal.AllocHGlobal((nint)byteLength) : Marshal.AllocCoTaskMem(checked((int)byteLength)); + _buffer.CopyTo(new Span((void*)ptr, length)); + ((char*)ptr)[length] = '\0'; // append null terminator manually + + IntPtr result = ptr; + ptr = IntPtr.Zero; // so we don't free the newly allocated memory + return result; } - - internal static unsafe void Copy(UnmanagedBuffer source, UnmanagedBuffer destination, ulong bytesLength) + finally { - if (bytesLength == 0) - { - return; - } - - byte* srcPtr = null, dstPtr = null; - try + // If we failed for any reason, free the new buffer + if (ptr != IntPtr.Zero) { - source.AcquirePointer(ref srcPtr); - destination.AcquirePointer(ref dstPtr); - Buffer.MemoryCopy(srcPtr, dstPtr, destination.ByteLength, bytesLength); - } - finally - { - if (dstPtr != null) + new Span((void*)ptr, length).Clear(); + if (globalAlloc) { - destination.ReleasePointer(); + Marshal.FreeHGlobal(ptr); } - if (srcPtr != null) + else { - source.ReleasePointer(); + Marshal.FreeCoTaskMem(ptr); } } } - - protected override unsafe bool ReleaseHandle() - { - new Span((void*)handle, _byteLength).Clear(); - Marshal.FreeHGlobal(handle); - return true; - } } } } diff --git a/src/libraries/System.Runtime.InteropServices/ref/System.Runtime.InteropServices.cs b/src/libraries/System.Runtime.InteropServices/ref/System.Runtime.InteropServices.cs index 96e3af5a431ca..3f2abb05310c8 100644 --- a/src/libraries/System.Runtime.InteropServices/ref/System.Runtime.InteropServices.cs +++ b/src/libraries/System.Runtime.InteropServices/ref/System.Runtime.InteropServices.cs @@ -1721,8 +1721,10 @@ namespace System.Security { public sealed partial class SecureString : System.IDisposable { + [System.ObsoleteAttribute("SecureString is deprecated and should not be used for new code.", DiagnosticId = "OBS0001", UrlFormat = "https://aka.ms/securestring")] public SecureString() { } [System.CLSCompliantAttribute(false)] + [System.ObsoleteAttribute("SecureString is deprecated and should not be used for new code.", DiagnosticId = "OBS0001", UrlFormat = "https://aka.ms/securestring")] public unsafe SecureString(char* value, int length) { } public int Length { get { throw null; } } public void AppendChar(char c) { } diff --git a/src/libraries/System.Runtime.InteropServices/tests/System.Runtime.InteropServices.Tests.csproj b/src/libraries/System.Runtime.InteropServices/tests/System.Runtime.InteropServices.Tests.csproj index ac8067e6b8b1b..292076770ae93 100644 --- a/src/libraries/System.Runtime.InteropServices/tests/System.Runtime.InteropServices.Tests.csproj +++ b/src/libraries/System.Runtime.InteropServices/tests/System.Runtime.InteropServices.Tests.csproj @@ -3,6 +3,8 @@ true $(NetCoreAppCurrent);$(NetCoreAppCurrent)-Windows_NT;$(NetCoreAppCurrent)-Unix true + + $(NoWarn);OBS0001 diff --git a/src/libraries/System.Security.Cryptography.Csp/tests/CspParametersTests.cs b/src/libraries/System.Security.Cryptography.Csp/tests/CspParametersTests.cs index 718a5c6b5f2bb..5c17d20e51032 100644 --- a/src/libraries/System.Security.Cryptography.Csp/tests/CspParametersTests.cs +++ b/src/libraries/System.Security.Cryptography.Csp/tests/CspParametersTests.cs @@ -4,6 +4,8 @@ using Xunit; +#pragma warning disable OBS0001 // SecureString ctors are obsolete + namespace System.Security.Cryptography.Csp.Tests { public static class CspParametersTests diff --git a/src/libraries/System.Security.Cryptography.X509Certificates/tests/ImportTests.cs b/src/libraries/System.Security.Cryptography.X509Certificates/tests/ImportTests.cs index 4f93181657f33..264e2b3c030af 100644 --- a/src/libraries/System.Security.Cryptography.X509Certificates/tests/ImportTests.cs +++ b/src/libraries/System.Security.Cryptography.X509Certificates/tests/ImportTests.cs @@ -5,6 +5,8 @@ using System.Security; using Xunit; +#pragma warning disable OBS0001 // SecureString ctors are obsolete + namespace System.Security.Cryptography.X509Certificates.Tests { public static class ImportTests diff --git a/src/libraries/System.Security.Cryptography.X509Certificates/tests/TestData.cs b/src/libraries/System.Security.Cryptography.X509Certificates/tests/TestData.cs index ff9363f7c8bf3..f37953f14b785 100644 --- a/src/libraries/System.Security.Cryptography.X509Certificates/tests/TestData.cs +++ b/src/libraries/System.Security.Cryptography.X509Certificates/tests/TestData.cs @@ -4,6 +4,8 @@ using Test.Cryptography; +#pragma warning disable OBS0001 // SecureString ctors are obsolete + namespace System.Security.Cryptography.X509Certificates.Tests { internal static class TestData diff --git a/src/libraries/System.Security.SecureString/tests/SecureStringTests.cs b/src/libraries/System.Security.SecureString/tests/SecureStringTests.cs index 14ba897dd7f03..d0d2b5868c182 100644 --- a/src/libraries/System.Security.SecureString/tests/SecureStringTests.cs +++ b/src/libraries/System.Security.SecureString/tests/SecureStringTests.cs @@ -9,6 +9,8 @@ using System.Threading.Tasks; using Xunit; +#pragma warning disable OBS0001 // SecureString ctors are obsolete + namespace System.Security.Tests { public static class SecureStringTests