Skip to content

Add SystemEntropy and HashDRBG generators. #2208

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

Abscissa
Copy link
Contributor

(This is a fixed and updated version of #2206)

Adds two RNG's suitable for cryptographic purposes.

Unlike the algorithms of Phobos's existing PRNGs, the core nature of both the system RNGs and the general Hash_DRBG algorithm is to generate an arbitrary (user-specified) number of bytes at each request. That leads to this PR's notion of RNG "streams", which the SystemEntropy and HashDRBG ranges are built upon. Since the new redesigned std.stream isn't ready, the docs in this PR specifically note that these optional "stream" interfaces are subject to change. However, I'm not entirely opposed to simply making them private for the time being, if that's deemed more appropriate.

Joseph Rushton Wakeling and I have discussed at length various aspects of this (and future directions of std.random, FWIW) in this NG thread: http://forum.dlang.org/thread/lk476f$159d$1@digitalmars.com

@quickfur
Copy link
Member

Looks like this needs a rebase.

Also, is no one seriously going to review this one? Crypto may be scary, but code is still code, and this one looks like a useful addition to Phobos.

P.S. Not to mention, this one doesn't actually implement any crypto algorithms; it just provides nice wrappers around system-provided ones.

@mihails-strasuns
Copy link

@WebDrake Joseph can you review this PR? It looks like you have taken part in original discussion.

@WebDrake
Copy link
Contributor

I have to say that where crypto is concerned, I currently feel very inadequate to make appropriate judgements. However, I'll try to review it systematically at least from a general Phobos/std.random design point of view and by comparison to the standard this is based on.

@mihails-strasuns
Copy link

ok, in a meanwhile @Abscissa please rebase ;)

@Abscissa
Copy link
Contributor Author

Re: Rebase: In the past I've adjusted PRs by just simply manually re-doing them (I'm not exactly a git master). I assume there's a more proper way to "rebase a PR". What's the standard procedure for that? Assume I'm an idiot.

Re: Review: Couple questions I'd like people to consider when reviewing this:

  1. Should this wait for the proposed redesign of std.random, or should it get pulled regardless and then (I can) submit a corresponding pull to update the proposed redesign?
  2. Should the "stream"-like stuff just be made private until we have the new std.stream?

I've been on the fence about both. Not sure which are the better ways to go.

@WebDrake
Copy link
Contributor

Should this wait for the proposed redesign of std.random, or should it get pulled regardless and then (I can) submit a corresponding pull to update the proposed redesign?

If the design itself is good, then I don't see why Phobos users should be denied access to it. The more important thing I think is how confident you/we are that this API is the right one for the problem at hand.

Should the "stream"-like stuff just be made private until we have the new std.stream?

I'll think about this carefully in my review. You could also touch base with Steve and ask for his feedback.

@mihails-strasuns
Copy link

Re: Rebase: In the past I've adjusted PRs by just simply manually re-doing them (I'm not exactly a git master). I assume there's a more proper way to "rebase a PR". What's the standard procedure for that? Assume I'm an idiot.

git checkout hash_drbg2
git fetch upstream
git rebase upstream/master

at this point it will likely to tell you about conflicts, git status will tell which files have those. You need to open those files and find <<<< lines - one block will show upstream changes and another yours. You will need to remove extra stuff and leave the sources in a way you expect those to be.

Once conflicts has been resolved, do git add filename. Once all conflicting files has been updated and added do git rebase --continue. It may possibly conflict again on next patch.

Once all patches has been applied successfully (it will print relevant message to console), you will need to force push updated branch : git push -f origin hash_drbg2. Reload GitHub PR page to check that everything has finished as expected.

@quickfur
Copy link
Member

On Sun, Jul 13, 2014 at 12:32:42PM -0700, Nick Sabalausky wrote:

Re: Rebase: In the past I've adjusted PRs by just simply manually
re-doing them (I'm not exactly a git master). I assume there's a more
proper way to "rebase a PR". What's the standard procedure for that?
[...]

My usual approach (don't know if it's the "usual" way):

git checkout master
git pull --ff-only upstream master # prevent major screwup
git checkout my_feature_branch
git rebase master

This will rewind your branch back to when it first diverged from master,
and then replay your changes onto the new master. If you're lucky,
everything will go well, and you can just push the branch again:

git push -f origin my_feature_branch

(The -f is needed because rebase rewrites history so git will normally
refuse to push the branch.)

In this case, however, you have merge conflicts, so probably during the
git rebase it will puase and ask you resolve the conflict. It should
tell you which file(s) are in conflict at this point, so fire up your
editor, search for those <<<< and >>>> conflict markers, and fix
them. Once you're happy with your changes, run git add ... to mark the
file(s) as resolved. Once all conflicts are resolved, continue the
rebase by running git rebase --continue. Repeat this until all commits
have been successfully rebased.

Then, push the branch to github with -f.

@mihails-strasuns
Copy link

Ping @WebDrake
Ping @Abscissa

@quickfur
Copy link
Member

quickfur commented Aug 5, 2014

ping @Abscissa

@Abscissa
Copy link
Contributor Author

Abscissa commented Aug 5, 2014

On 8/5/2014 1:40 PM, H. S. Teoh wrote:

ping @Abscissa

Yea, sorry, suddenly got real busy. I'll get to this this week.

@Abscissa
Copy link
Contributor Author

Thanks all. It's now rebased (easy and painless now that I actually know how :P).

@WebDrake
Copy link
Contributor

So sorry for the delay in reviewing this, it's been a rather intense period work-wise. I'll see what I can do, but if there's any impatience, don't wait for me.

version(Windows)
{
// Reference: http://blogs.msdn.com/b/michael_howard/archive/2005/01/14/353379.aspx
_advapi32 = Runtime.loadLibrary("ADVAPI32.DLL");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this ever suffer from DLL hijacking? E.g. someone injects a fake ADVAPI32.DLL in the directory of the executable, and replaces all random calls to always return the same number.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah doing runtime load instead of dynamic linking does not look as a good idea for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't DLL hijacking already a big security risk regardless? Also, wouldn't there be a similar risk with /dev/random via a chroot (or some such)?

Also, and perhaps more imporant, I don't know how to static link that. :( Anyone know?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you do normal dynamic linking, fully qualified path to dll/so gets bound during compilation/link time. So if it got resolved to system-wide dynamic library, hijacking it will require root access. Runtime loading allows to place a local dynamic library which has higher priority in path resolution over a system one.

chroot makes it possible to fake the system environment, of course, but it won't help to hijack already installed program in the host system (with only user access)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how to static link that. :( Anyone know?

I am not familiar with Windows build process but it should be something in line with supplying ADVAPI32.DLL to linker flags in Phobos makefile.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you do normal dynamic linking, fully qualified path to dll/so gets bound during compilation/link time. So if it got resolved to system-wide dynamic library, hijacking it will require root access. Runtime loading allows to place a local dynamic library which has higher priority in path resolution over a system one.

Uuuh no, at least not on windows. The import table only contains the name of the dll, not the path, and dlls in the exe's directory will be preferred over system ones.

I am not familiar with Windows build process but it should be something in line with supplying ADVAPI32.DLL to linker flags in Phobos makefile.

You need to supply advapi32's import library to the linker. It's in the dmd zip as \windows\lib\advapi32.lib.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, forgive my ignorance about Windows then :(

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to supply advapi32's import library to the linker. It's in the dmd zip as \windows\lib\advapi32.lib.

Is that sufficient to prevent the (real?/theoretical?) DLL hijacking mentioned above?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's more theoretical than anything. I remember reading about some hijacking issue with Windows DLLs (which may have been fixed since then). I think it was this one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that sufficient to prevent the (real?/theoretical?) DLL hijacking mentioned above?

No, it's just how you tell the linker how to resolve the symbols in that dll.

@quickfur
Copy link
Member

quickfur commented Oct 6, 2014

ping

@Abscissa
Copy link
Contributor Author

Abscissa commented Oct 8, 2014

pong!

Am I reading the autotester results right? It looks like the only failure is just that there's a module ctor circular dependency that's unrelated to this PR...?

Also, I'm unclear on what, if anything, should be done about the DLL issue discussed above.

(What is wrong with GitHub lately?? It's taking this site's stupid edit box several seconds to register each keypress. How the hell does GitHub manage to make text entry on modern hardware slower than on an Apple II?!?)

@quickfur
Copy link
Member

quickfur commented Oct 8, 2014

(What is wrong with GitHub lately?? It's taking this site's stupid
edit box several seconds to register each keypress. How the hell does
GitHub manage to make text entry on modern hardware slower than on an
Apple II?!?)

Unfortunately, this time nothing is wrong with the edit box, but
everything is wrong with your browser. My current (very old) browser is
also exhibiting strange behaviour like pausing for 30 seconds to respond
to a keystroke every now and then. And taking forever to load a PR page
of medium length, and forever to close it. And taking 20 seconds just to
display the comment entry box in the PR changed files page.

Sounds like github's fault, but it happens on other sites too. And
switching to Firefox magically makes all the problems go away instantly.

Conclusion: either (1) github is in cahoots with firefox to take over
the world; or (2) the Javascript apocalypse is here, and anything short
of superpowered JIT node.js jquery web2.0 buzzworded optimized
Javascript engines won't be able to survive the load without collapsing
under their own weight; or (3) it's the browser that's making things
ridiculously slow, so the blame belongs to the browser, not the website.
Time to ditch that old crusty browser and move on with the rest of the
world.

I trust you can determine which is the most likely scenario. :-P (If
not, pretend they are a monty hall puzzle and apply the corresponding
solution methods. :-P :-P :-P)

@Abscissa
Copy link
Contributor Author

Abscissa commented Oct 8, 2014

Actually, believe it or not, that was on FF v31 (well, Iceweasel). So just about the latest. (Yea, I know, hell's frozen over, right? :) Got a new machine a few weeks ago and didn't feel like figuring out how to downgrade FF. Besides, some time ago, after a tip from arsd, I'd managed to wrangle FF21(or 23? 24? something like that) into a surprisingly good state. So I knew what add-ons and settings to cram it full of.)

I dunno, maybe it's a some runaway process, or memory leak or something. Everything else seems fine though, even within the browser (so it's shouldn't be an addon's fault, I think). FF's memory usage is only ~366MB ATM which is enormous, but somewhat low for a web browser anymore. Does still seem to be JS-related - If I disable JS on this page then the edit box becomes silky-smooth. Then back to molasses when I switch it back on again. Pfft, I dunno. Oh well, pardon the thread hijacking.

@quickfur
Copy link
Member

quickfur commented Oct 8, 2014

Hmm. I'm using FF (Iceweasel) 32.0. And now that you mention it, my ancient crusty Opera 12 was working just fine until recently. So it looks like scenario (2) might actually be the answer here. :-(

@quickfur
Copy link
Member

ping
Any news on this PR?

@andralex
Copy link
Member

closing for now, please reopen

@andralex andralex closed this Jan 11, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants