Skip to content
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

Scheduler-friendly bcrypt NIF #12

Closed
wants to merge 5 commits into from
Closed

Conversation

jazzyb
Copy link
Contributor

@jazzyb jazzyb commented Apr 14, 2015

I have reworked the bcrypt C code to fix the issue seen in #8. These changes break up the longest running parts of the bcrypt algorithm in order to yield execution back to the Erlang VM every millisecond or so. I have verified the fix manually using the bitwise method.

There are a couple of concerns with these changes:

  • I have done my best to maintain the security of the original implementation, and there are no obvious bugs or vulnerabilities. However, writing C code is difficult, and writing secure C code is doubly so. If you know of anyone who is familiar with cryptography or writing complex NIFs in general, I would love for them to take a close look at my implementation.
  • In order to make bcrypt regularly relinquish control back to Erlang, the code was modified to the point that keeping the C source up-to-date with the OpenSSL implementation will be more difficult. I don't know if this fix is worth the extra work for maintainers going forward.

I see that you have started your own pure Elixir implementation of bcrypt. In the long run, that may be the best solution. In which case this code may be used as a temporary fix for #8, or you might want to pull it into a separate branch to compare the efficiency of the native code with a pure Elixir implementation when it is completed.

Thanks!

This commit splits the original bcrypt() fucntion into three separate NIF
functions:
	bcrypt() - initializes the state for the Blowfish algorithm
	bcrypt_expandstate() - Blowfish_expand0state loop
	bcrypt_fini() - performs final encryption and returns hash
There should be no change to the underlying algorithm.
Verified manually with the following method:

	iex(1)> :erlang.system_monitor(self(), [{:long_schedule, 5}])
	:undefined
	iex(2)> spawn(fn -> Comeonin.Bcrypt.hashpwsalt("difficult2guess") end)
	#PID<0.118.0>
	iex(3)> flush()
	:ok
	iex(4)>

The number in the :erlang.system_monitor() call represents milliseconds.  If an
erlang process ever uses up more than those milliseconds, a warning message
will be seen after the flush() call.

See:  https://github.com/vinoski/bitwise#long-scheduling
@jazzyb
Copy link
Contributor Author

jazzyb commented Apr 14, 2015

Apparently, enif_schedule_nif is only implemented for 17.3+. None of this code will work for older versions :(

@riverrun
Copy link
Owner

Great stuff!
I had started work on a naive implementation that uses Elixir as mush as possible, but uses NIFs to call the blowfish expand keys operations. I might still continue with this and see how it goes, but I'm happy that your solution solves the main issue.
I'm going to initially merge this into a 'erlschedulenif' branch, but I'm almost certain that I'll merge it into master within a few days.
Thanks again!

@riverrun riverrun closed this Apr 14, 2015
@jazzyb
Copy link
Contributor Author

jazzyb commented Apr 14, 2015

Please keep up the work on the Elixir implementation. If it performs efficiently, it will certainly be easier for yourself and others to maintain.

There is a odd bug in 17.3 I had discovered thanks to Travis CI. I'm working on a fix.

Edit: Filed a bug for this issue: #14.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants