From 3b8403ded5d2f7b3b58971e8e5bd506d0440e5aa Mon Sep 17 00:00:00 2001 From: antonfirsov Date: Sun, 10 Mar 2024 22:42:58 +0100 Subject: [PATCH 1/4] thread safe id generation with RandomNumberGenerator --- src/DnsClient/DnsRequestHeader.cs | 19 +++++++++++++++++-- test/DnsClient.Tests/DnsRequestHeaderTest.cs | 18 +++++++++++++++++- 2 files changed, 34 insertions(+), 3 deletions(-) diff --git a/src/DnsClient/DnsRequestHeader.cs b/src/DnsClient/DnsRequestHeader.cs index 5a8b48aa..585d9d26 100644 --- a/src/DnsClient/DnsRequestHeader.cs +++ b/src/DnsClient/DnsRequestHeader.cs @@ -1,11 +1,18 @@ using System; +using System.Security.Cryptography; using DnsClient.Protocol; namespace DnsClient { internal class DnsRequestHeader { - private static readonly Random s_random = new Random(); + // A cryptographically strong random number generator is preferred. See: + // https://msrc.microsoft.com/blog/2008/04/ms08-020-how-predictable-is-the-dns-transaction-id/ + // https://github.com/miekg/dns/issues/1043 +#if !(NETSTANDARD2_1_OR_GREATER || NET6_0_OR_GREATER) + private static readonly RandomNumberGenerator s_random = RandomNumberGenerator.Create(); + private static readonly byte[] s_randomBytes = new byte[2]; +#endif public const int HeaderLength = 12; private ushort _flags = 0; @@ -102,7 +109,15 @@ public void RefreshId() private static ushort GetNextUniqueId() { - return (ushort)s_random.Next(1, ushort.MaxValue); +#if NETSTANDARD2_1_OR_GREATER || NET6_0_OR_GREATER + return (ushort)RandomNumberGenerator.GetInt32(1, ushort.MaxValue); +#else + lock (s_random) + { + s_random.GetBytes(s_randomBytes); + return (ushort)(s_randomBytes[0] << 8 | s_randomBytes[1]); + } +#endif } } } diff --git a/test/DnsClient.Tests/DnsRequestHeaderTest.cs b/test/DnsClient.Tests/DnsRequestHeaderTest.cs index 962c9da9..b33240f5 100644 --- a/test/DnsClient.Tests/DnsRequestHeaderTest.cs +++ b/test/DnsClient.Tests/DnsRequestHeaderTest.cs @@ -1,4 +1,6 @@ using System; +using System.Collections.Concurrent; +using System.Threading.Tasks; using Xunit; namespace DnsClient.Tests @@ -49,5 +51,19 @@ public void DnsRequestHeader_ChangeRecursion() Assert.Equal(DnsOpCode.Notify, header.OpCode); } + + [Fact] + public void DnsRequestHeader_IdIsPseudoUnique() + { + ConcurrentDictionary ids = new ConcurrentDictionary(); + + Parallel.For(0, 1000, i => + { + var header = new DnsRequestHeader(DnsOpCode.Query); + ids.TryAdd(header.Id, 0); + }); + + Assert.True(ids.Count > 900); + } } -} \ No newline at end of file +} From 0094ef942a7bb53dd077a4eefb707ef67ef618e1 Mon Sep 17 00:00:00 2001 From: antonfirsov Date: Sun, 10 Mar 2024 23:04:28 +0100 Subject: [PATCH 2/4] error message in test --- test/DnsClient.Tests/DnsRequestHeaderTest.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/DnsClient.Tests/DnsRequestHeaderTest.cs b/test/DnsClient.Tests/DnsRequestHeaderTest.cs index b33240f5..194e81d1 100644 --- a/test/DnsClient.Tests/DnsRequestHeaderTest.cs +++ b/test/DnsClient.Tests/DnsRequestHeaderTest.cs @@ -63,7 +63,7 @@ public void DnsRequestHeader_IdIsPseudoUnique() ids.TryAdd(header.Id, 0); }); - Assert.True(ids.Count > 900); + Assert.True(ids.Count > 950, $"Only {ids.Count} of 1000 ids are unique!"); } } } From 7a833084335d37f8efd28b843e0dcf46dcfab2dc Mon Sep 17 00:00:00 2001 From: antonfirsov Date: Mon, 11 Mar 2024 01:57:43 +0100 Subject: [PATCH 3/4] a dumb way to disallow id=0 --- src/DnsClient/DnsRequestHeader.cs | 9 +++++++-- test/DnsClient.Tests/DnsRequestHeaderTest.cs | 3 +++ 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/src/DnsClient/DnsRequestHeader.cs b/src/DnsClient/DnsRequestHeader.cs index 585d9d26..e8655e51 100644 --- a/src/DnsClient/DnsRequestHeader.cs +++ b/src/DnsClient/DnsRequestHeader.cs @@ -114,8 +114,13 @@ private static ushort GetNextUniqueId() #else lock (s_random) { - s_random.GetBytes(s_randomBytes); - return (ushort)(s_randomBytes[0] << 8 | s_randomBytes[1]); + ushort result; + do + { + s_random.GetBytes(s_randomBytes); + result = (ushort)(s_randomBytes[0] << 8 | s_randomBytes[1]); + } while (result == 0); + return result; } #endif } diff --git a/test/DnsClient.Tests/DnsRequestHeaderTest.cs b/test/DnsClient.Tests/DnsRequestHeaderTest.cs index 194e81d1..b9188c07 100644 --- a/test/DnsClient.Tests/DnsRequestHeaderTest.cs +++ b/test/DnsClient.Tests/DnsRequestHeaderTest.cs @@ -1,5 +1,7 @@ using System; using System.Collections.Concurrent; +using System.Runtime.CompilerServices; +using System.Security.Cryptography; using System.Threading.Tasks; using Xunit; @@ -60,6 +62,7 @@ public void DnsRequestHeader_IdIsPseudoUnique() Parallel.For(0, 1000, i => { var header = new DnsRequestHeader(DnsOpCode.Query); + Assert.NotEqual(0, header.Id); ids.TryAdd(header.Id, 0); }); From a2ee64cc57138a575ba36a3349b41c71bf93e1d4 Mon Sep 17 00:00:00 2001 From: antonfirsov Date: Mon, 11 Mar 2024 03:41:20 +0100 Subject: [PATCH 4/4] do not use secure random --- src/DnsClient/DnsRequestHeader.cs | 21 +++++--------------- test/DnsClient.Tests/DnsRequestHeaderTest.cs | 5 +---- 2 files changed, 6 insertions(+), 20 deletions(-) diff --git a/src/DnsClient/DnsRequestHeader.cs b/src/DnsClient/DnsRequestHeader.cs index e8655e51..699a33b9 100644 --- a/src/DnsClient/DnsRequestHeader.cs +++ b/src/DnsClient/DnsRequestHeader.cs @@ -1,17 +1,12 @@ using System; -using System.Security.Cryptography; using DnsClient.Protocol; namespace DnsClient { internal class DnsRequestHeader { - // A cryptographically strong random number generator is preferred. See: - // https://msrc.microsoft.com/blog/2008/04/ms08-020-how-predictable-is-the-dns-transaction-id/ - // https://github.com/miekg/dns/issues/1043 -#if !(NETSTANDARD2_1_OR_GREATER || NET6_0_OR_GREATER) - private static readonly RandomNumberGenerator s_random = RandomNumberGenerator.Create(); - private static readonly byte[] s_randomBytes = new byte[2]; +#if !NET6_0_OR_GREATER + private static readonly Random s_random = new Random(); #endif public const int HeaderLength = 12; private ushort _flags = 0; @@ -109,18 +104,12 @@ public void RefreshId() private static ushort GetNextUniqueId() { -#if NETSTANDARD2_1_OR_GREATER || NET6_0_OR_GREATER - return (ushort)RandomNumberGenerator.GetInt32(1, ushort.MaxValue); +#if NET6_0_OR_GREATER + return (ushort)Random.Shared.Next(1, ushort.MaxValue); #else lock (s_random) { - ushort result; - do - { - s_random.GetBytes(s_randomBytes); - result = (ushort)(s_randomBytes[0] << 8 | s_randomBytes[1]); - } while (result == 0); - return result; + return (ushort)s_random.Next(1, ushort.MaxValue); } #endif } diff --git a/test/DnsClient.Tests/DnsRequestHeaderTest.cs b/test/DnsClient.Tests/DnsRequestHeaderTest.cs index b9188c07..935646f9 100644 --- a/test/DnsClient.Tests/DnsRequestHeaderTest.cs +++ b/test/DnsClient.Tests/DnsRequestHeaderTest.cs @@ -1,7 +1,4 @@ -using System; -using System.Collections.Concurrent; -using System.Runtime.CompilerServices; -using System.Security.Cryptography; +using System.Collections.Concurrent; using System.Threading.Tasks; using Xunit;