.NET Secure Random Class - Updated

[.NET] Secure Random Class - Updated - Printable Version

+- Hack Forums (https://hackforums.net)
+-- Forum: Tech (https://hackforums.net/forumdisplay.php?fid=444)
+--- Forum: Coding (https://hackforums.net/forumdisplay.php?fid=151)
+---- Forum: Software Development (https://hackforums.net/forumdisplay.php?fid=118)
+---- Thread: [.NET] Secure Random Class - Updated (/showthread.php?tid=5169646)

Pages: Pages 1 2 3

Secure Random Class - Updated - debug - Feb 13, 2016

Hey all,

After Aeon berated me for using System.Random, I finally decided to stop using it. If you want a good read on why it's not secure, view his thread here: http://coderail.com/thread-2.html

Now, for general purposes, System.Random isn't horrible to use, but RNGCryptoServiceProvider is much better. Again, for full explanation read aeon's thread above.

Anyway, here is the class.
VB.NET - https://gist.github.com/debug-hf/a08553a0da1f5e1e51b4
( Thanks to Eternal Darkness )
C# - https://gist.github.com/debug-hf/1a932d5d3c7270bd57ac
And thanks to Odus for correcting multiple things <3
EDIT: I have updated the class based on your suggestions. (Although I'm not certain on my implementation of SecureRandom. If it's incorrect let me know.)

Enjoy,
debug

* - Rhez - Feb 14, 2016

Woah. This is new thing for me.
Thanks posting this. 👍

* - Cecilia - Feb 14, 2016

It is a nice little wrapper.
Thanks as always :)

* - _B1N4RY_ - Feb 14, 2016

I always knew entropy was a major concern when using a built in "Random" function for any language. Never realized how actually insecure it really was though, nice read. Thanks for sharing!

* - Niǌa - Feb 14, 2016

Don't declare anything as new outside of the functions.

Code 
Private ReadOnly rng As RandomNumberGenerator = New RNGCryptoServiceProvider()

You should replace that with:

Code 
Private ReadOnly rng As RandomNumberGenerator

And in the constructor assign the value to it:

Code 
RandomNumberGenerator = New RNGCryptoServiceProvider()

* - Temploit - Feb 14, 2016

(Feb 14, 2016 08:49 AM)Tyranitar Wrote: Don't declare anything as new outside of the functions.

Code
Private ReadOnly rng As RandomNumberGenerator = New RNGCryptoServiceProvider()
You should replace that with:

Code
Private ReadOnly rng As RandomNumberGenerator
And in the constructor assign the value to it:

Code
RandomNumberGenerator = New RNGCryptoServiceProvider()

It seems you have no idea what you're talking about. Directly initializing instance variables compiles down to them being assigned before anything else in the constructor(s).

* - +Haze - Feb 14, 2016

Nice one debug, my nigga :D

* - Odus - Feb 14, 2016

Looks good. Only thing I would say is pad out your code (especially the VB version). You also don't have to evaluate min and max, it will still work even if reversed. If you do, supply the offending param name with a message when throwing the range exception.

* - Cecilia - Feb 14, 2016

(Feb 14, 2016 08:49 AM)Tyranitar Wrote: Don't declare anything as new outside of the functions.

Code
Private ReadOnly rng As RandomNumberGenerator = New RNGCryptoServiceProvider()
You should replace that with:

Code
Private ReadOnly rng As RandomNumberGenerator
And in the constructor assign the value to it:

Code
RandomNumberGenerator = New RNGCryptoServiceProvider()

I just laughed more than I should have.
If variables are assigned in a class outside of the constructor, they are added there on compile time without the user knowing that it has been done.
Please, go and open up any project which you have in dnSpy that does a similar thing.

* - 6e756c6c - Feb 14, 2016

Kind of off topic but Tyranitar is right. You should always initialize your default members inside of the constructor, if you let the compiler do it for you then you are forfeiting the ability to define the 'load' order. For instance, if one member relies on another for whatever reason; especially at construction but was automatically initialized first, then you are going to get a null reference exception (if your lucky? It may just silently ignore the error causing some bizarre issues). It's also much more readable (in my opinion) if members are initialized in the constructor as this saves you from searching all over the document. :P

Edit:
Looks awesome debug!

* - +Haze - Feb 14, 2016

Tbh I think the issue is to do with the way Tyranitar worded his reply as to what I think he intended to say. Rather than saying "Don't declare anything as new outside of the functions." he made it seem like a must, when infact it's sometimes down to preference and if you have overload constructor methods.

Personally, I'm with Tyranitar on the basis that I prefer to initialize my variables inside constructors. I find it more cleaner that way, that's just my preference. And also, when I have overload constructors, it helps to control load order like Aeon mentioned.

* - Cecilia - Feb 14, 2016

(Feb 14, 2016 03:15 PM)Aeonhack Wrote: Kind of off topic but Tyranitar is right. You should always initialize your default members inside of the constructor, if you let the compiler do it for you then you are forfeiting the ability to define the 'load' order. For instance, if one member relies on another for whatever reason; especially at construction but was automatically initialized first, then you are going to get a null reference exception (if your lucky? It may just silently ignore the error causing some bizarre issues). It's also much more readable (in my opinion) if members are initialized in the constructor as this saves you from searching all over the document. :P

Edit:
Looks awesome debug!

Yes, correct, but there is only 1 field in this class. There are of course issues in much larger projects.

* - 6e756c6c - Feb 14, 2016

(Feb 14, 2016 03:48 PM)LaPanthere Wrote: Yes, correct, but there is only 1 field in this class. There are of course issues in much larger projects.

In that case it's an issue of consistency which is just as important. o_o

* - Niǌa - Feb 14, 2016

(Feb 14, 2016 03:04 PM)LaPanthere Wrote: I just laughed more than I should have.
If variables are assigned in a class outside of the constructor, they are added there on compile time without the user knowing that it has been done.
Please, go and open up any project which you have in dnSpy that does a similar thing.

(Feb 14, 2016 03:21 PM)+Haze Wrote: Tbh I think the issue is to do with the way Tyranitar worded his reply as to what I think he intended to say. Rather than saying "Don't declare anything as new outside of the functions." he made it seem like a must, when infact it's sometimes down to preference and if you have overload constructor methods.

Personally, I'm with Tyranitar on the basis that I prefer to initialize my variables inside constructors. I find it more cleaner that way, that's just my preference. And also, when I have overload constructors, it helps to control load order like Aeon mentioned.

Its just good practice. For this it surely works but its a bad habit if you keep on doing it like this especially in bigger projects or if you work with someone else on a project. I remember doing it the same way as OP instead of in a constructor which caused a shit-storm.

* - Temploit - Feb 14, 2016

(Feb 14, 2016 04:18 PM)Aeonhack Wrote: In that case it's an issue of consistency which is just as important. o_o

I actually have to disagree on both your statements.

First, if you created a situation where load order becomes an issue, then you did it wrong. Knowing how the compiler does things, you should be able to control the load order without problems. If you really need 'recursive' instance variables (1st one uses 2nd one, 2nd one uses 1st one), then rethink your design.

Second, if your instance variable assignments don't change when you overload your constructor, it's simply bad design to put them in one in my opinion. If the value is always the same, it's just a burden to put them in the constructor, buried between other things, while you can just as easily put them after instance variables, making it clear what value it should have. A constructor should only do things with its arguments. If not, it fails single-responsibility. If overloading requires redundant duplication, it also fails 'as little code repeating' (don't know the actual name). It's actually inconsistent to put it in the constructor.

(It doesn't matter what happens after compilation, since most of c# is syntactic sugar anyways)