Univention Bugzilla – Bug 52693
Support bcrypt in UCS/openldap
Last modified: 2021-02-03 15:04:04 CET
* OpenLDAP patchen - * bcrypt Unterstütung in lock_password in univention-directory-manager-modules einbauen ** python-bcrypt maintained machen ** lock_password in univention-python anpassen, sodass eine UCR-Variable ausgewertet wird ** Optional den bcrypt cost factor ebenfalls per UCR konfigurierbar machen. * ucs-test Integrationstest schreiben
* python-bcrypt.yaml - copied to errata4.4-7 * openldap.yaml - bcrypt support - patches 98_bcrypt.quilt and 98_bcrypt_rules.patch - based on https://github.com/wclarie/openldap-bcrypt.git * univention-ldap.yaml 61c885ce42b13cd8d83b36517c3ed5bf83320a15 - added ldap/pw-bcrypt for bcrypt module * ucs-test 94053a5cc9e8aab2913932d1f1639e29b7c85b33 - simple bcrypt test (no udm) TODO * the UDM stuff * caculateMaintained for python-bcrypt * merge to 5.0-0
* ucs-test - added 61_udm-users/53_udm_users_user_bcrypt_password for users/user and users/ldap bcrypt handling tests 11fdd959c8261951db0963732520db35618eafbf 45fd406ebfd76a0e10373ad778e23abeea5b5d2c * univention-directory-manager-modules - bcrypt support ced4e2f820191d67bf72ca3ff6038a085e1c92fb 3c39b05f79a604170e48f7d795c8663fe122a8ef 81792d134432ae3080cae74302af663c8c300461 d50e767300d4b825c4b942939c008dd86b932315 * univention-directory-manager-modules.yaml * python-bcrypt is maintained (via dependency in univention-directory-manager-modules) -> apt-cache policy python-bcrypt python-bcrypt: Installiert: 3.1.2-1 500 http://updates-test.software-univention.de/4.4/maintained/component 4.4-7-errata-test/amd64/ Packages TODO * check jenkins test * merge to 5.0
Tests looking good, i have added extra bugs for documentation (bug #52713) and for bcrypt support for computer objects (bug #52714). please QA the current state, i will merge these changes to 5.0-0 after the qa, so please re-open
Some security related questions: 1. Why is this new cipher necessary? Where does the requirement come from? This Bug does not link a any ticket or task providing that information. 2. Who has done a security audit for this upstream patch? We're patching our core component OpenLDAP. Why do we merge this patch and why is it not yet integrated upstream into OpenLDAP itself?
(In reply to Philipp Hahn from comment #4) > Some security related questions: > > 1. Why is this new cipher necessary? Where does the requirement come from? > This Bug does not link a any ticket or task providing that information. DL 2020062921001161 > 2. Who has done a security audit for this upstream patch? We're patching our > core component OpenLDAP. Why do we merge this patch and why is it not yet > integrated upstream into OpenLDAP itself? That is a legitimate concern. I had a quick look at the patch (and i think Arvid), but i'm not a security expert ... and i double checked the hashing functionality with a different tool, if that is not enough please re-open/create new bug/have a look at the patch One more note, this is disabled by default
Hi Everyone, Monika asked me to see if I can say something to this patch from security perspective, so here is a quick one. The basic bcrypt implementation code comes from the Openwall security-enhanced Linux distribution. Which "takes the inspiration" it from the even more security conscious OpenBSD project. https://www.openwall.com/ https://en.wikipedia.org/wiki/Openwall_Project Openwall performs their own security audits, in fact providing security audits is Openwall's main source of income: https://www.openwall.com/services/ So regarding the question of "2. Who has done a security audit for this". I think it's safe to say that for this part Openwall did. The code we will be looking at here states: "No copyright is claimed" and placed under public domain. On top of that codebase, to make it usable with OpenLDAP Wouter Clarie, security & privacy consultant made a wrapper. His profile is here https://www.linkedin.com/in/wclarie with some recommendations from big names in the software industry. He also had some contributions to OpenBSD and LibreSSL, where the whole bcrypt initiative comes from. So he also does security audits for a living, so to the question of "2. Who has done a security audit for this" I think the answer is, that he did it himself. On his code the same thing applies, "No copyright is claimed" and placed under public domain. But let's see what the https://github.com/wclarie/openldap-bcrypt code really is: crypt_blowfish.h --> Exactly the same as the openwall 1.3 implementation no difference. crypt_blowfish.c --> The only difference to the openwall 1.3 implementation, is that the openwall implementation has some Assembly optimisation embedded into some C macro. This C macro is only enabling this optimisation for i386, and for x86_64 it is always off. Since we are only targeting x86_64 essentially there is no difference for us whatsoever. Makefile --> Just a harmless Makefile, I don't see any magic backdoor being opened here. pw-bcrypt.c --> This is 200 lines of C code, uses the symbols exposed by crypt_blowfish.h for implementing 2 static and 1 global C functions for OpenLDAP, namely hash_bcrypt, chk_bcrypt and init_module. Let's see those three functions in details: ___ hash_bcrypt ___ It takes two input arguments, a Scheme and a Password in form of OpenLDAP BER Values, and it returns the apropriate hash, in one output argument. It does it by using OpenLDAP's standard `lutil_entropy` generator as `salt`. And using the basline bcrypt functions _crypt_gensalt_blowfish_rn and _crypt_blowfish_rn, the rest is error handling, memory copying and other boilerplate. It is worth mentioning that this function takes an unused parameter here, but that is inevitable as OpenLDAP expects a function pointer to a function with 4 arguments. ___ chk_bcrypt ___ It takes two input arguments, an existing hash of Password, and a user-supplied password, which needs to be checked. All in form of OpenLDAP BER Values. It uses the same baseline _crypt_blowfish_rn to get a new hash and compare it to the previously supplied one. This function takes two unused arguments, which are again inevitable to fulfill the OpenLDAP requirements. __ init_module __ This is the usual OpenLDAP module initialisation boilerplate, this takes variable number of arguments. If there is no, then a default bcrypt work factor is used, otherwise first argument is interpreted as a number and that will be the work factor. So all in all, there is no big magic here, if the original Openwall Project implementation of bcrypt is to be trusted, then this OpenLDAP integration is also OK in my opinion. The one very important thing to keep in mind is that we should by no means enable SLAPD_BCRYPT_DEBUG by accident, as it will start printing out unencrypted passwords. Should we go on and compare the Openwall codebase to the OpenBSD implementation? Will then OpenBSD trusted enough? Those are my questions. Regarding the question "Why do we merge this patch and why is it not yet integrated upstream into OpenLDAP itself?". Let's see this issue: https://github.com/wclarie/openldap-bcrypt/issues/1 It appears that the author of the patch doesn't have time and capacity to deal with upstreaming this, and the OpenLDAP people are blaming tha author for that, and think that only the author can be intrested in upstreaming something. But of course there can be other interpretations as well.
(In reply to Ferenc Géczi from comment #6) ... Thank you for your very details analysis, this is enough for me. I just wanted to assert that at least someone from the security community has audited this piece of code before we integrate and ship it.
python-bcrypt is maintained: OK Authentication/password-change works with bcrypt: OK Authentication/password-change works with 2a/2b mode: OK Authentication/password-change with S4: OK Authentication/password-change with AD: OK Password history works with bcrypt hashes(2a, 2b): OK Tests: OK Code review: OK YAM: OK REOPEN: Please merge to ucs5!
402df7fc7d218d3c1f09b44086c9953d968dcdf7 - removed salt parameter
openldap: Added (slightly modified) 98_bcrypt.quilt and 98_bcrypt_rules.patch to svn/patches/openldap/5.0-0-0-ucs/2.4.47+dfsg-3+deb10u4 and re-built the openldap packages, short test with UCS 5 OK (as the other changes are not yet available, everything by hand) add "moduleload pw-bcrypt.so" to /etc/ldap/slapd.conf create a test user change userPassword to bcrypt hash (e.g. {BCRYPT}$2b$08$s8k7t6cvdHLLMz75emGZMeP.d0c3Xl/.to3FntmYUxabPlpEa/bjW -> randompassword) ldap authentication should work: univention-ldapsearch -D uid=test1,dc=five,dc=test -x -w randompassword univention-ldap, univention-directory-manager-modules, ucs-test: https://git.knut.univention.de/univention/ucs/-/merge_requests/64
openLDAP built in UCS5: OK mr: OK short manual tests on UCS5: OK Verified
<https://errata.software-univention.de/#/?erratum=4.4x884> <https://errata.software-univention.de/#/?erratum=4.4x885> <https://errata.software-univention.de/#/?erratum=4.4x886> <https://errata.software-univention.de/#/?erratum=4.4x887>