Closed antonfirsov closed 3 months ago
I was actually adding a fix for this in my open PR, too: https://github.com/MichaCo/DnsClient.NET/pull/207/files#diff-6ce9119d23f07ed09bd510d31b8f53054f0909b741bcfad887ab7b0d8aeba0dc
Now I'm wondering, why did you remove the version with RandomNumberGenerator in favor of a lock? I do get roughly the same result with your unit test with my version at least ;)
private static ushort GetRandom()
{
var block = new byte[2];
s_generator.GetBytes(block);
return BitConverter.ToUInt16(block, 0);
}
In #207 the two implementation paths have different characteristics: RandomNumberGenerator
is cryptographically secure (but slower) while Random
is not secure (but fast). I orginally used RandomNumberGenerator
, but removed it in a2ee64c for simplicity. What made the code more complicated than ideal is the exlusion of id==1
which you don't do in #207 for old targets. I fail to understand why is id != 1
needed, but I wanted to keep the old logic.
In my understanding client txid-s don't have to be cryptographically secure random numbers, so no need to use RandomNumberGenerator
unless DnsClient.NET is expected to be used in a recursive DNS server.
gotcha.
In my understanding client txid-s don't have to be cryptographically secure random numbers, so no need to use RandomNumberGenerator unless DnsClient.NET is expected to be used in a recursive DNS server.
yup that is also my understanding.
Fix #209 ~and use cryptographically strong random numbers~ Edit: removed the latter, the vulnerability doesn't apply to clients..