Skip to content

Commit 909ddaa

Browse files
DeagleGrossDeagleGross
committed
try more changes
1 parent cdf9957 commit 909ddaa

File tree

7 files changed

+169
-52
lines changed

7 files changed

+169
-52
lines changed

‎src/Antiforgery/src/Internal/BinaryBlob.cs‎

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ namespace Microsoft.AspNetCore.Antiforgery;
1010
// Represents a binary blob (token) that contains random data.
1111
// Useful for binary data inside a serialized stream.
1212
[DebuggerDisplay("{DebuggerString}")]
13-
internal sealed class BinaryBlob : IEquatable<BinaryBlob>
13+
internal sealed class BinaryBlob : IEquatable<BinaryBlob>, IEquatable<ReadOnlySpan<byte>>
1414
{
1515
private readonly byte[] _data;
1616

@@ -50,6 +50,16 @@ public override bool Equals(object? obj)
5050
return Equals(obj as BinaryBlob);
5151
}
5252

53+
public bool Equals(ReadOnlySpan<byte> other)
54+
{
55+
if (other.Length != _data.Length)
56+
{
57+
return false;
58+
}
59+
60+
return AreSpanEqual(_data, other);
61+
}
62+
5363
public bool Equals(BinaryBlob? other)
5464
{
5565
if (other == null)
@@ -58,7 +68,7 @@ public bool Equals(BinaryBlob? other)
5868
}
5969

6070
Debug.Assert(_data.Length == other._data.Length);
61-
return AreByteArraysEqual(_data, other._data);
71+
return AreSpanEqual(_data, other._data);
6272
}
6373

6474
public byte[] GetData()
@@ -83,16 +93,16 @@ private static byte[] GenerateNewToken(int bitLength)
8393

8494
// Need to mark it with NoInlining and NoOptimization attributes to ensure that the
8595
// operation runs in constant time.
86-
[MethodImplAttribute(MethodImplOptions.NoInlining | MethodImplOptions.NoOptimization)]
87-
private static bool AreByteArraysEqual(byte[] a, byte[] b)
96+
[MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.NoOptimization)]
97+
private static bool AreSpanEqual(ReadOnlySpan<byte> a, ReadOnlySpan<byte> b)
8898
{
89-
if (a == null || b == null || a.Length != b.Length)
99+
if (a.Length != b.Length)
90100
{
91101
return false;
92102
}
93103

94104
var areEqual = true;
95-
for (var i = 0; i < a.Length; i++)
105+
for (int i = 0; i < a.Length; i++)
96106
{
97107
areEqual &= (a[i] == b[i]);
98108
}

‎src/Antiforgery/src/Internal/DefaultAntiforgeryTokenGenerator.cs‎

Lines changed: 19 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
// Licensed to the .NET Foundation under one or more agreements.
22
// The .NET Foundation licenses this file to you under the MIT license.
33

4+
using System.Buffers;
45
using System.Diagnostics.CodeAnalysis;
56
using System.Security.Claims;
67
using System.Security.Principal;
@@ -137,15 +138,28 @@ public bool TryValidateTokenSet(
137138

138139
// Is the incoming token meant for the current user?
139140
var currentUsername = string.Empty;
140-
BinaryBlob? currentClaimUid = null;
141141

142142
var authenticatedIdentity = GetAuthenticatedIdentity(httpContext.User);
143143
if (authenticatedIdentity != null)
144144
{
145-
currentClaimUid = GetClaimUidBlob(_claimUidExtractor.ExtractClaimUid(httpContext.User));
146-
if (currentClaimUid == null)
145+
var claimUidBytes = ArrayPool<byte>.Shared.Rent(1024);
146+
try
147147
{
148-
currentUsername = authenticatedIdentity.Name ?? string.Empty;
148+
_claimUidExtractor.ExtractClaimUid(httpContext.User, claimUidBytes.AsSpan(), out var bytesWritten);
149+
if (bytesWritten is 0)
150+
{
151+
currentUsername = authenticatedIdentity.Name ?? string.Empty;
152+
}
153+
154+
if (requestToken.ClaimUid?.Equals(claimUidBytes.AsSpan(0, bytesWritten)) != true)
155+
{
156+
message = Resources.AntiforgeryToken_ClaimUidMismatch;
157+
return false;
158+
}
159+
}
160+
finally
161+
{
162+
ArrayPool<byte>.Shared.Return(claimUidBytes);
149163
}
150164
}
151165

@@ -164,14 +178,8 @@ public bool TryValidateTokenSet(
164178
return false;
165179
}
166180

167-
if (!object.Equals(requestToken.ClaimUid, currentClaimUid))
168-
{
169-
message = Resources.AntiforgeryToken_ClaimUidMismatch;
170-
return false;
171-
}
172-
173181
// Is the AdditionalData valid?
174-
if (_additionalDataProvider != null &&
182+
if (_additionalDataProvider is not null &&
175183
!_additionalDataProvider.ValidateAdditionalData(httpContext, requestToken.AdditionalData))
176184
{
177185
message = Resources.AntiforgeryToken_AdditionalDataCheckFailed;

‎src/Antiforgery/src/Internal/DefaultClaimUidExtractor.cs‎

Lines changed: 28 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -2,11 +2,11 @@
22
// The .NET Foundation licenses this file to you under the MIT license.
33

44
using System.Buffers;
5-
using System.Buffers.Binary;
65
using System.Diagnostics;
76
using System.Security.Claims;
87
using System.Security.Cryptography;
98
using System.Text;
9+
using Microsoft.AspNetCore.Shared;
1010
using Microsoft.Extensions.ObjectPool;
1111

1212
namespace Microsoft.AspNetCore.Antiforgery;
@@ -23,6 +23,22 @@ public DefaultClaimUidExtractor(ObjectPool<AntiforgerySerializationContext> pool
2323
_pool = pool;
2424
}
2525

26+
/// <inheritdoc />
27+
public void ExtractClaimUid(ClaimsPrincipal claimsPrincipal, Span<byte> destination, out int bytesWritten)
28+
{
29+
Debug.Assert(claimsPrincipal != null);
30+
31+
var uniqueIdentifierParameters = GetUniqueIdentifierParameters(claimsPrincipal.Identities);
32+
if (uniqueIdentifierParameters == null)
33+
{
34+
// No authenticated identities containing claims found.
35+
bytesWritten = 0;
36+
return;
37+
}
38+
39+
ComputeSha256(uniqueIdentifierParameters, destination, out bytesWritten);
40+
}
41+
2642
/// <inheritdoc />
2743
public string? ExtractClaimUid(ClaimsPrincipal claimsPrincipal)
2844
{
@@ -37,9 +53,7 @@ public DefaultClaimUidExtractor(ObjectPool<AntiforgerySerializationContext> pool
3753

3854
// todo skip allocations here as well
3955
var claimUidBytes = ComputeSha256(uniqueIdentifierParameters);
40-
41-
Convert.TryToBase64Chars(claimUidBytes, out var str, out int charsWritten);
42-
return str.ToString;
56+
return Convert.ToBase64String(claimUidBytes);
4357
}
4458

4559
public static IList<string>? GetUniqueIdentifierParameters(IEnumerable<ClaimsIdentity> claimsIdentities)
@@ -125,53 +139,42 @@ public DefaultClaimUidExtractor(ObjectPool<AntiforgerySerializationContext> pool
125139
return identifierParameters;
126140
}
127141

128-
private void ComputeSha256(IEnumerable<string> parameters, Span<byte> output)
142+
private static void ComputeSha256(IEnumerable<string> parameters, Span<byte> output, out int bytesWritten)
129143
{
130-
// compute total size
131-
int totalSize = 0;
144+
int estimatedSize = 0;
132145
foreach (var param in parameters)
133146
{
134147
int byteCount = Encoding.UTF8.GetByteCount(param);
135-
totalSize += 4 + byteCount; // 4 bytes for length prefix
148+
estimatedSize += 5 + byteCount; // max 5 bytes for 7-bit length + content
136149
}
137150

138-
byte[]? rented = null;
139-
var buffer = totalSize <= 256
140-
? stackalloc byte[256]
141-
: (rented = ArrayPool<byte>.Shared.Rent(totalSize));
142-
buffer = buffer[..totalSize];
151+
byte[] buffer = ArrayPool<byte>.Shared.Rent(estimatedSize);
143152

144153
try
145154
{
146155
int offset = 0;
147-
148156
foreach (var param in parameters)
149157
{
150158
int byteCount = Encoding.UTF8.GetByteCount(param);
151159

152-
// Write 4-byte length prefix
153-
BinaryPrimitives.WriteInt32LittleEndian(buffer.Slice(offset, 4), byteCount);
154-
offset += 4;
160+
// Write 7-bit encoded length prefix
161+
offset += buffer.AsSpan(offset).Write7BitEncodedInt(byteCount);
155162

156163
// Write UTF-8 bytes
157-
Encoding.UTF8.GetBytes(param, buffer.Slice(offset, byteCount));
164+
Encoding.UTF8.GetBytes(param, buffer.AsSpan(offset, byteCount));
158165
offset += byteCount;
159166
}
160167

161-
SHA256.TryHashData(buffer.Slice(0, totalSize), output, out int bytesWritten);
168+
SHA256.TryHashData(buffer.AsSpan(0, offset), output, out bytesWritten);
162169
Debug.Assert(bytesWritten == 32);
163170
}
164171
finally
165172
{
166-
buffer.Clear(); // security ?
167-
if (rented is not null)
168-
{
169-
ArrayPool<byte>.Shared.Return(rented);
170-
}
173+
ArrayPool<byte>.Shared.Return(buffer);
171174
}
172175
}
173176

174-
private byte[] ComputeSha256StreamWriter(IEnumerable<string> parameters)
177+
private byte[] ComputeSha256(IEnumerable<string> parameters)
175178
{
176179
var serializationContext = _pool.Get();
177180

‎src/Antiforgery/src/Internal/IClaimUidExtractor.cs‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,14 @@ namespace Microsoft.AspNetCore.Antiforgery;
1010
/// </summary>
1111
internal interface IClaimUidExtractor
1212
{
13+
/// <summary>
14+
/// Extracts claims identifier and writes to <paramref name="destination"/>.
15+
/// </summary>
16+
/// <param name="claimsPrincipal">The <see cref="ClaimsPrincipal"/>.</param>
17+
/// <param name="destination">destination for claimUid bytes</param>
18+
/// <param name="bytesWritten">length of bytes written to <paramref name="destination"/></param>
19+
void ExtractClaimUid(ClaimsPrincipal claimsPrincipal, Span<byte> destination, out int bytesWritten);
20+
1321
/// <summary>
1422
/// Extracts claims identifier.
1523
/// </summary>

‎src/Antiforgery/test/DefaultAntiforgeryTokenGeneratorTest.cs‎

Lines changed: 58 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
using System.Security.Cryptography;
77
using Microsoft.AspNetCore.Http;
88
using Microsoft.AspNetCore.InternalTesting;
9+
using Microsoft.Extensions.ObjectPool;
910
using Moq;
1011

1112
namespace Microsoft.AspNetCore.Antiforgery.Internal;
@@ -401,12 +402,13 @@ public void TryValidateTokenSet_UsernameMismatch(string identityUsername, string
401402
IsCookieToken = false
402403
};
403404

404-
var mockClaimUidExtractor = new Mock<IClaimUidExtractor>();
405-
mockClaimUidExtractor.Setup(o => o.ExtractClaimUid(It.Is<ClaimsPrincipal>(c => c.Identity == identity)))
406-
.Returns((string)null);
405+
var mockClaimUidExtractor = new MockClaimUidExtractor(bytesFunc: (claims, bytes) =>
406+
{
407+
return 0;
408+
});
407409

408410
var tokenProvider = new DefaultAntiforgeryTokenGenerator(
409-
claimUidExtractor: mockClaimUidExtractor.Object,
411+
claimUidExtractor: mockClaimUidExtractor,
410412
additionalDataProvider: null);
411413

412414
string expectedMessage =
@@ -439,12 +441,16 @@ public void TryValidateTokenSet_ClaimUidMismatch()
439441
};
440442

441443
var differentToken = new BinaryBlob(256);
442-
var mockClaimUidExtractor = new Mock<IClaimUidExtractor>();
443-
mockClaimUidExtractor.Setup(o => o.ExtractClaimUid(It.Is<ClaimsPrincipal>(c => c.Identity == identity)))
444-
.Returns(Convert.ToBase64String(differentToken.GetData()));
444+
445+
var mockClaimUidExtractor = new MockClaimUidExtractor(
446+
bytesFunc: (claimsPrincipal, bytes) =>
447+
{
448+
bytes = differentToken.GetData();
449+
return differentToken.BitLength;
450+
});
445451

446452
var tokenProvider = new DefaultAntiforgeryTokenGenerator(
447-
claimUidExtractor: mockClaimUidExtractor.Object,
453+
claimUidExtractor: mockClaimUidExtractor,
448454
additionalDataProvider: null);
449455

450456
string expectedMessage =
@@ -542,18 +548,27 @@ public void TryValidateTokenSet_Success_AuthenticatedUserWithUsername()
542548
var cookieToken = new AntiforgeryToken() { IsCookieToken = true };
543549
var fieldtoken = new AntiforgeryToken()
544550
{
551+
ClaimUid = new BinaryBlob(32),
545552
SecurityToken = cookieToken.SecurityToken,
546553
Username = "THE-USER",
547554
IsCookieToken = false,
548555
AdditionalData = "some-additional-data"
549556
};
550557

558+
var mockClaimUidExtractor = new MockClaimUidExtractor(
559+
(claimsPrincipal, bytes) =>
560+
{
561+
var data = fieldtoken.ClaimUid.GetData();
562+
data.CopyTo(bytes);
563+
return data.Length;
564+
});
565+
551566
var mockAdditionalDataProvider = new Mock<IAntiforgeryAdditionalDataProvider>();
552567
mockAdditionalDataProvider.Setup(o => o.ValidateAdditionalData(httpContext, "some-additional-data"))
553568
.Returns(true);
554569

555570
var tokenProvider = new DefaultAntiforgeryTokenGenerator(
556-
claimUidExtractor: new Mock<IClaimUidExtractor>().Object,
571+
claimUidExtractor: mockClaimUidExtractor,
557572
additionalDataProvider: mockAdditionalDataProvider.Object);
558573

559574
// Act
@@ -616,5 +631,39 @@ public override string Name
616631
get { return String.Empty; }
617632
}
618633
}
634+
635+
private class MockClaimUidExtractor : IClaimUidExtractor
636+
{
637+
private readonly Func<ClaimsPrincipal, Span<byte>, int> _bytesFunc;
638+
private readonly Func<ClaimsPrincipal, string> _claimsFunc;
639+
640+
public MockClaimUidExtractor(
641+
Func<ClaimsPrincipal, Span<byte>, int> bytesFunc = null,
642+
Func<ClaimsPrincipal, string> claimsFunc = null)
643+
{
644+
_bytesFunc = bytesFunc;
645+
_claimsFunc = claimsFunc;
646+
}
647+
648+
public void ExtractClaimUid(ClaimsPrincipal claimsPrincipal, Span<byte> destination, out int bytesWritten)
649+
{
650+
if (_bytesFunc is null)
651+
{
652+
throw new NotImplementedException($"Explicitly define {nameof(_bytesFunc)} invocation for testing");
653+
}
654+
655+
bytesWritten = _bytesFunc(claimsPrincipal, destination);
656+
}
657+
658+
public string ExtractClaimUid(ClaimsPrincipal claimsPrincipal)
659+
{
660+
if (_claimsFunc is null)
661+
{
662+
throw new NotImplementedException($"Explicitly define {nameof(_claimsFunc)} invocation for testing");
663+
}
664+
665+
return _claimsFunc(claimsPrincipal);
666+
}
667+
}
619668
}
620669
#nullable restore

0 commit comments

Comments
 (0)