Page MenuHomeFreeBSD

improve PBKDF2 performance
ClosedPublic

Authored by allanjude on Oct 13 2016, 1:59 AM.

Details

Summary

The PBKDF2 in sys/geom/eli/pkcs5v2.c is around half the speed than it could be.
https://jbp.io/2015/08/11/pbkdf2-performance-matters/

Submitted by: Joe Pixton
Original Patch by: jmg

I cleaned up the patch to solve issues from files that have moved in the mean time

PR: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=202365

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

allanjude updated this revision to Diff 21336.Oct 13 2016, 1:59 AM
allanjude retitled this revision from to improve PBKDF2 performance.
allanjude updated this object.
allanjude edited the test plan for this revision. (Show Details)
allanjude added reviewers: secteam, jmg, pjd, delphij, emaste, ed, oshogbo.
allanjude updated this object.Oct 13 2016, 2:00 AM
bcr added a subscriber: bcr.Oct 13 2016, 9:08 AM

I'm just picky today. ;-)

tests/sys/geom/eli/pbkdf2/gentestvect.py
8 ↗(On Diff #21336)

Do we need this comment? I know what you do there and I leave breadcrumbs in my code for myself sometimes, too. ;-)

ed accepted this revision.Oct 13 2016, 9:26 AM
ed edited edge metadata.

Looks good.

Quick question: the tests that are added by this commit are actually independent of the change to GELI itself, right? If so, could we please have them committed separately? Thanks!

tests/sys/geom/eli/pbkdf2/gentestvect.py
3 ↗(On Diff #21336)

As pbkdf2_hmac is only used once in this Python script, you can also just remove the import and use hashlib.pbkdf2_hmac instead.

20 ↗(On Diff #21336)

Rule of thumb: when concatenating more than two strings, just use % or .format():

return '"%s"' % result

As Python is an interpreted language, performance is typically paid by the operation. String formatting is therefore cheaper than concatenating many strings.

26 ↗(On Diff #21336)

What's wrong with Python's own RNG functions? :-)

This revision is now accepted and ready to land.Oct 13 2016, 9:26 AM
allanjude added inline comments.Oct 18 2016, 2:26 AM
tests/sys/geom/eli/pbkdf2/gentestvect.py
8 ↗(On Diff #21336)

this bit of code is not mine, it was written by jmg@

26 ↗(On Diff #21336)

I am just integrating the code from the PR. We'd need to query jmg@ about the specifics.

ed added inline comments.Oct 18 2016, 6:42 AM
tests/sys/geom/eli/pbkdf2/gentestvect.py
1 ↗(On Diff #21336)

By the way, I've noticed that this Python script is formatted like style(9). I think that's not something we should try to do for non-C languages. Could we please follow the PEP-8 style?

https://www.python.org/dev/peps/pep-0008/

The https://pypi.python.org/pypi/autopep8 tool can help you to do the formatting automatically.

delphij requested changes to this revision.Oct 18 2016, 6:54 AM
delphij edited edge metadata.

Looks otherwise good to me. Could you please fix or comment with the reasoning for the comment inline?

sys/geom/eli/pkcs5v2.c
85 ↗(On Diff #21336)

Since we already have explicit_bzero now and the intention is to wipe sensitive data here, it should be used (unless you have really compelling reasons not to do so).

This revision now requires changes to proceed.Oct 18 2016, 6:54 AM
pjd accepted this revision.Feb 3 2017, 9:36 PM
pjd edited edge metadata.
allanjude updated this revision to Diff 25372.Feb 19 2017, 5:54 AM
allanjude edited edge metadata.

Switch the bzero's in pkcs5v2_genkey to explicit_bzero because they are wiping sensitive data

As requested by delphij

allanjude marked 5 inline comments as done.Feb 19 2017, 5:56 AM
delphij accepted this revision.Feb 19 2017, 6:15 AM
delphij edited edge metadata.

Thanks!

This revision is now accepted and ready to land.Feb 19 2017, 6:15 AM
This revision was automatically updated to reflect the committed changes.