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

Use crypt_ra to allocate scratch area for password hashing #16981

Merged
merged 12 commits into from
Sep 18, 2020

Conversation

keszybz
Copy link
Member

@keszybz keszybz commented Sep 8, 2020

No description provided.

@keszybz keszybz added util-lib firstboot homed homed, homectl, pam_homed labels Sep 8, 2020
Copy link
Member

@poettering poettering left a comment

Choose a reason for hiding this comment

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

I'd really hide the buffer allocation in the inner-most code block. The reusing of the allocated buffers seems a lot complexity in the interface for pretty much no gain, since we typically deal with a single password to hash only, and in the worst case maybe a handful, but this still shouldn't matter at all performance-wise.

Or to say this differently: the actual hashing of the pw is likely a lot more expensive than the malloc() we might do too much, hence it shouldn't matter

src/firstboot/firstboot.c Outdated Show resolved Hide resolved
src/home/user-record-util.c Outdated Show resolved Hide resolved
src/home/user-record-util.c Outdated Show resolved Hide resolved
src/home/homework.c Outdated Show resolved Hide resolved
@poettering poettering added the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Sep 8, 2020
@keszybz keszybz force-pushed the use-crypt_ra branch 2 times, most recently from 132e357 to 7b884b5 Compare September 8, 2020 16:11
@keszybz
Copy link
Member Author

keszybz commented Sep 8, 2020

I made the cached allocation mostly hidden, but I left it in places where we loop over passwords, e.g. in user_record_quality_check_password(). PTAL.

@keszybz keszybz removed the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Sep 8, 2020
@keszybz
Copy link
Member Author

keszybz commented Sep 8, 2020

Hmm, the build fails because crypt_ra is not available, strange.

@poettering
Copy link
Member

CI is not happy...

I still wouldn't bother even for the pw quality check stuff. Yeah, it looks like O(n^2), but given that n is almost always == 1, the malloc() shouldn't matter...

@keszybz keszybz added the ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR label Sep 8, 2020
@poettering
Copy link
Member

oh, uh.

i think i remember now: crypt_ra() is libxcrypt-only. glibc didn't have it. our CIs still use glibcs that did not do the libxcrypt split-out.

I guess this means there needs to be some fallback for a while. crypt + crypt_r are available on glibc, and crypt_rn/crypt_ra are openwall/libxcrypt additions.

@keszybz
Copy link
Member Author

keszybz commented Sep 8, 2020

I removed the caching of the buffer now. It is only exposed in the low-level hash_password_full() function and used in tests. "Normal" callers use the wrapper which always allocates.

meson.build Outdated
crypt_header = conf.get('HAVE_CRYPT_H') == 1 ? \
'''#include <crypt.h>''' : '''#include <unistd.h>'''
foreach ident : [
['crypt_r', crypt_header],
Copy link
Member

Choose a reason for hiding this comment

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

crypt_r is glibc already, we should not need to test for it.

Copy link
Member Author

Choose a reason for hiding this comment

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

The impetus for this PR came from centos ci which reports:

20:36:36     archlinux_systemd_ci: Checking for function "crypt_r" with dependency -lcrypt: YES 
20:36:36     archlinux_systemd_ci: Checking for function "crypt_rn" with dependency -lcrypt: NO 
20:36:36     archlinux_systemd_ci: Checking for function "crypt_ra" with dependency -lcrypt: NO 

If we had crypt_rn, there'd be hope to improve things. But since there is neither _ra nor _rn, most likely this will not resolve #16965, unless the glibc version there is updated. I think it's still worthwhile to pursue this PR, since the _ra variant seems to be a much better API. WDYT?

(Am I missing some magic option to "enable" xcrypt? At least in Fedora libxcrypt.rpm only provides libcrypt.so.2, so using -lcrypt is the only way to consume xcrypt.)

Copy link
Member

@mrc0mmand mrc0mmand Sep 10, 2020

Choose a reason for hiding this comment

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

Hah, now I see why downgrading glibc on the current Arch Linux fails:

error: failed to commit transaction (conflicting files)
libxcrypt: /usr/include/crypt.h exists in filesystem (owned by glibc)
libxcrypt: /usr/lib/libcrypt.so exists in filesystem (owned by glibc)

The thing is that in recent glibc updates Arch transitioned from libcrypt in glibc to libcrypt in libxcrypt, see [0][1]. And, of course, the libxcrypt contains the functions you seek:

libxcrypt /usr/share/man/man3/crypt_r.3.gz
libxcrypt /usr/share/man/man3/crypt_ra.3.gz
libxcrypt /usr/share/man/man3/crypt_rn.3.gz

But right now the upgrade is blocked by the sanitizer issue, but to make it go away we need to upgrade the image, so we're in a chicken-egg situation. I guess I could temporarily disable the test which triggers the issue (TEST-46) and go ahead with the upgrade.

Copy link
Member

Choose a reason for hiding this comment

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

all i wanted to say: there's no point in checking for crypt_r, since our baseline, i.e. old glibc had that since time began.

checking for crypt_ra() definitely makes sense since glibc never had that, and onl libxcrypt has.

crypt_rn is a call we probably don#t care about, we shouldn't test for it hence

@keszybz keszybz marked this pull request as draft September 8, 2020 20:44
@keszybz keszybz marked this pull request as ready for review September 10, 2020 16:40
@keszybz keszybz removed the ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR label Sep 10, 2020
@keszybz
Copy link
Member Author

keszybz commented Sep 10, 2020

As discussed above, I think this is still worth pursuing even if it doesn't fix the immediate issue when libxcrypt is not available. Hopefully the tests will pass now.

@poettering
Copy link
Member

maybe add a minimal local fallback implementation for crypt_ra() here, that just wraps crypt_r() and does malloc() itself. i.e. call it missing_crypt_ra() and #define it to crypt_ra() if thta's available and otherwise just malloc() locally, and call crypt_r().

(I guess the whole issue here is just fix what this the comment in crypt(3) suggests: "struct crypt_data may be quite large (32kB in this implementation of libcrypt; over 128kB in some other implementations). This is large enough that it may be unwise to allocate it on the stack.")

@poettering
Copy link
Member

using crypt_ra() is probably more than just an option, given this further comment from the man page:

"Some recently designed hashing methods need even more scratch memory, but the crypt_r interface makes it impossible to change the size of struct crypt_data without breaking binary compatibility. The crypt_rn interface could
accommodate larger allocations for specific hashing methods, but the caller of crypt_rn has no way of knowing how much memory to allocate. crypt_ra does the allocation itself, but can only make a single call to malloc(3)."

So yeah, unless we use crypt_ra() if it is available we would not be compatible with contemporary hashes, and I think we should

@poettering
Copy link
Member

ah, now i see you just pushed such a fallback...

@@ -75,21 +75,23 @@ int make_salt(char **ret) {
#endif
}

int hash_password(const char *password, char **ret) {
int hash_password_full(const char *password, void **cd_data, int *cd_size, char **ret) {
Copy link
Member

Choose a reason for hiding this comment

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

i still wouldn't expose the cd_data, sounds like needless optimization. i.e it's the kind of optimization you might want to do ina hash breaker or so, but otherwise I really wouldn't bother...

Copy link
Member Author

Choose a reason for hiding this comment

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

This is exposed here, but is only used in the test. I think it is worthwhile to show that in tests: it should make it much easier to debug issues like #16965.

meson.build Outdated
crypt_header = conf.get('HAVE_CRYPT_H') == 1 ? \
'''#include <crypt.h>''' : '''#include <unistd.h>'''
foreach ident : [
['crypt_r', crypt_header],
Copy link
Member

Choose a reason for hiding this comment

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

all i wanted to say: there's no point in checking for crypt_r, since our baseline, i.e. old glibc had that since time began.

checking for crypt_ra() definitely makes sense since glibc never had that, and onl libxcrypt has.

crypt_rn is a call we probably don#t care about, we shouldn't test for it hence

/* Provide a poor man's fallback that uses a fixed size buffer. */

static char* systemd_crypt_ra(const char *phrase, const char *setting, void **data, int *size) {
struct crypt_data cd = {};
Copy link
Member

Choose a reason for hiding this comment

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

i think we should malloc() this, given that the structure might be too large for the stack according to the man page:

"struct crypt_data may be quite large (32kB in this implementation of libcrypt; over 128kB in some other implementations). This is large enough that it may be unwise to allocate it on the stack".

@poettering poettering added the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Sep 10, 2020
@mrc0mmand
Copy link
Member

mrc0mmand commented Sep 10, 2020

As discussed above, I think this is still worth pursuing even if it doesn't fix the immediate issue when libxcrypt is not available. Hopefully the tests will pass now.

As the issue is reproducible only with libxcrypt (which is not in the current images), the tests should definitely pass. However, once the outstanding reviews in this PR are resolved, I'll run it manually against the upgraded image to see if the issue is indeed gone.

src/home/homework.c Outdated Show resolved Hide resolved
@keszybz
Copy link
Member Author

keszybz commented Sep 15, 2020

centos7:

10:38:23 --- test-libcrypt-util begin ---
10:38:23 Found container virtualization none.
10:38:23 /* test_hash_password */
10:38:23 ew3bU1.hoKk4o: yes
10:38:23 $1$gc5rWpTB$wK1aul1PyBn9AX1z93stk1: no
10:38:23 $2b$12$BlqcGkB/7BFvNMXKGxDea.5/8D6FTny.cbNcHW/tqcrcyo6ZJd8u2: no
10:38:23 $5$lGhDrcrao9zb5oIK$05KlOVG3ocknx/ThreqXE/gk.XzFFBMTksc4t2CPDUD: no
10:38:23 $6$c7wB/3GiRk0VHf7e$zXJ7hN0aLZapE.iO4mn/oHu6.prsXTUG/5k1AxpgR85ELolyAcaIGRgzfwJs3isTChMDBjnthZyaMCfCNxo9I.: no
10:38:23 $y$j9T$$9cKOWsAm4m97WiYk61lPPibZpy3oaGPIbsL4koRe/XD: no

That machine only supports original descrypt hash, how is that even possible? @mrc0mmand maybe it's missing some library.

They are only used under src/home/, but I want to add tests in test-libcrypt-util.c.
And the functions are almost trivial, so I think it is OK to move them to shared.
Now that we wrap crypt_r/ra uses, we can include the header only in libcrypt-util.c.
We always seem to have either libcrypt_r and not the other two, or all
three. So the fallback for libcrypt_ra needs to be based on libcrypt_r.
Since the loop to check various xcrypt functions is already in place,
adding one more is cheap. And it is nicer to check for the function
directly. People like to backport things, so we might get lucky even
without having libxcrypt.
Following the style in missing_syscall.h, we use a non-conflicting name
for the function and use a macro to map to the real name to the replacement.
I'm tired of trying to figure this out.
…lable

On centos7 ci:

--- test-libcrypt-util begin ---
Found container virtualization none.
/* test_hash_password */
ew3bU1.hoKk4o: yes
$1$gc5rWpTB$wK1aul1PyBn9AX1z93stk1: no
$2b$12$BlqcGkB/7BFvNMXKGxDea.5/8D6FTny.cbNcHW/tqcrcyo6ZJd8u2: no
$5$lGhDrcrao9zb5oIK$05KlOVG3ocknx/ThreqXE/gk.XzFFBMTksc4t2CPDUD: no
$6$c7wB/3GiRk0VHf7e$zXJ7hN0aLZapE.iO4mn/oHu6.prsXTUG/5k1AxpgR85ELolyAcaIGRgzfwJs3isTChMDBjnthZyaMCfCNxo9I.: no
$y$j9T$$9cKOWsAm4m97WiYk61lPPibZpy3oaGPIbsL4koRe/XD: no
@mrc0mmand
Copy link
Member

centos7:

10:38:23 --- test-libcrypt-util begin ---
10:38:23 Found container virtualization none.
10:38:23 /* test_hash_password */
10:38:23 ew3bU1.hoKk4o: yes
10:38:23 $1$gc5rWpTB$wK1aul1PyBn9AX1z93stk1: no
10:38:23 $2b$12$BlqcGkB/7BFvNMXKGxDea.5/8D6FTny.cbNcHW/tqcrcyo6ZJd8u2: no
10:38:23 $5$lGhDrcrao9zb5oIK$05KlOVG3ocknx/ThreqXE/gk.XzFFBMTksc4t2CPDUD: no
10:38:23 $6$c7wB/3GiRk0VHf7e$zXJ7hN0aLZapE.iO4mn/oHu6.prsXTUG/5k1AxpgR85ELolyAcaIGRgzfwJs3isTChMDBjnthZyaMCfCNxo9I.: no
10:38:23 $y$j9T$$9cKOWsAm4m97WiYk61lPPibZpy3oaGPIbsL4koRe/XD: no

That machine only supports original descrypt hash, how is that even possible? @mrc0mmand maybe it's missing some library.

I suspect this might be caused by non-descrypt methods being available only in the glibc extension or something like that?

From crypt(3) on CentOS 7:

NOTES
   Glibc notes
       The glibc2 version of this function supports additional encryption algorithms.

       If salt is a character string starting with the characters "$id$" followed by a string terminated by "$":

              $id$salt$encrypted

       then instead of using the DES machine, id identifies the encryption method used and this then determines how the rest of the password string is interpreted.  The following values of id are supported:

              ID  | Method
              ─────────────────────────────────────────────────────────
              1   | MD5
              2a  | Blowfish (not in mainline glibc; added in some
                  | Linux distributions)
              5   | SHA-256 (since glibc 2.7)
              6   | SHA-512 (since glibc 2.7)

When I tried $6$ manually on the CI machine, it worked as expected:

#define _GNU_SOURCE
#include <crypt.h>
#include <stdio.h>
#include <string.h>

int main(int argc, char *argv[])
{
	struct crypt_data cc = {};
	const char *hash = "$6$c7wB/3GiRk0VHf7e$zXJ7hN0aLZapE.iO4mn/oHu6.prsXTUG/5k1AxpgR85ELolyAcaIGRgzfwJs3isTChMDBjnthZyaMCfCNxo9I.";
	const char *k;

	k = crypt_r("ppp", hash, &cc);
	if (!k) {
		perror("crypt_r()");
		return 1;
	}

	printf("%s\n", (strcmp(k, hash) == 0) ? "pass" : "fail");

	return 0;
}
# gcc -o main main.c -lcrypt
# ./main 
pass

@keszybz
Copy link
Member Author

keszybz commented Sep 15, 2020

When I tried $6$ manually on the CI machine, it worked as expected:

I assume that this is on the host itself. The failure happens in nested image in TEST-24-UNITTESTS.

@mrc0mmand
Copy link
Member

When I tried $6$ manually on the CI machine, it worked as expected:

I assume that this is on the host itself. The failure happens in nested image in TEST-24-UNITTESTS.

Ah, my bad. Will look into that.

@mrc0mmand
Copy link
Member

@keszybz so, after comparing straces from inside/outside of the VM and playing around with installing various missing libraries, installing /lib64/libfreeblpriv3.so (along with its dependencies) into the VM image does the trick.

E.g.:

diff --git a/test/test-functions b/test/test-functions
index 9893864..c3204a8 100644
--- a/test/test-functions
+++ b/test/test-functions
@@ -446,6 +446,10 @@ setup_basic_environment() {
     if [[ "$IS_BUILT_WITH_ASAN" = "yes" ]]; then
         create_asan_wrapper
     fi
+
+    inst_binary /root/systemd/main /main
+    inst_library /usr/lib64/libfreeblpriv3.so
+    inst /lib64/libfreeblpriv3.chk
 }

(ignore the /root/systemd/main binary)

Before:

-bash-4.2# strace -e open,openat /main
open("/etc/ld.so.cache", O_RDONLY|O_CLOEXEC) = 3
open("/lib64/libcrypt.so.1", O_RDONLY|O_CLOEXEC) = 3
open("/lib64/libc.so.6", O_RDONLY|O_CLOEXEC) = 3
open("/lib64/libfreebl3.so", O_RDONLY|O_CLOEXEC) = 3
open("/lib64/libdl.so.2", O_RDONLY|O_CLOEXEC) = 3
open("/lib64/libfreeblpriv3.so", O_RDONLY|O_CLOEXEC) = -1 ENOENT (No such file or directory)
open("/usr/lib64/libfreeblpriv3.so", O_RDONLY|O_CLOEXEC) = -1 ENOENT (No such file or directory)
open("/etc/ld.so.cache", O_RDONLY|O_CLOEXEC) = 3
open("/lib64/tls/x86_64/libfreeblpriv3.so", O_RDONLY|O_CLOEXEC) = -1 ENOENT (No such file or directory)
open("/lib64/tls/libfreeblpriv3.so", O_RDONLY|O_CLOEXEC) = -1 ENOENT (No such file or directory)
open("/lib64/x86_64/libfreeblpriv3.so", O_RDONLY|O_CLOEXEC) = -1 ENOENT (No such file or directory)
open("/lib64/libfreeblpriv3.so", O_RDONLY|O_CLOEXEC) = -1 ENOENT (No such file or directory)
open("/usr/lib64/tls/x86_64/libfreeblpriv3.so", O_RDONLY|O_CLOEXEC) = -1 ENOENT (No such file or directory)
open("/usr/lib64/tls/libfreeblpriv3.so", O_RDONLY|O_CLOEXEC) = -1 ENOENT (No such file or directory)
open("/usr/lib64/x86_64/libfreeblpriv3.so", O_RDONLY|O_CLOEXEC) = -1 ENOENT (No such file or directory)
open("/usr/lib64/libfreeblpriv3.so", O_RDONLY|O_CLOEXEC) = -1 ENOENT (No such file or directory)
crypt_r() returned NULL
+++ exited with 1 +++

After:

-bash-4.2# strace -e open,openat /main
open("/etc/ld.so.cache", O_RDONLY|O_CLOEXEC) = 3
open("/lib64/libcrypt.so.1", O_RDONLY|O_CLOEXEC) = 3
open("/lib64/libc.so.6", O_RDONLY|O_CLOEXEC) = 3
open("/lib64/libfreebl3.so", O_RDONLY|O_CLOEXEC) = 3
open("/lib64/libdl.so.2", O_RDONLY|O_CLOEXEC) = 3
open("/lib64/libfreeblpriv3.so", O_RDONLY|O_CLOEXEC) = 3
open("/etc/ld.so.cache", O_RDONLY|O_CLOEXEC) = 3
open("/lib64/libnspr4.so", O_RDONLY|O_CLOEXEC) = 3
open("/dev/urandom", O_RDONLY)          = 3
open("/lib64/libfreeblpriv3.chk", O_RDONLY) = 3
open("/lib64/libfreeblpriv3.so", O_RDONLY) = 3
open("/etc/ld.so.cache", O_RDONLY|O_CLOEXEC) = 3
open("/lib64/libnspr4.so", O_RDONLY|O_CLOEXEC) = 3
open("/proc/sys/crypto/fips_enabled", O_RDONLY) = 3
hash comparison: pass
+++ exited with 0 +++

@keszybz
Copy link
Member Author

keszybz commented Sep 15, 2020

@mrc0mmand wow, thanks for tracking this down. libfreeblpriv3.so is definitely not the first name that would come to mind.

The failing test was caused by missing deps. But I think we do want to test such systems too: this is not something that we verify during the configuration step, and it is possible that users might build on such systems. Also, in principle in the future systemd might be built on a system which only has newer hashes (or just different, e.g. $yg$), so it is better not to assume any particular hash is available, so I think we're good here.

@poettering
Copy link
Member

Hmm, so what's the state of this? I guess we can just merge this? PR looks good to me. Any PPC work to delay this for? If not, let's merge!

@keszybz
Copy link
Member Author

keszybz commented Sep 18, 2020

I think this is good to go.

@poettering poettering merged commit 3f440b1 into systemd:master Sep 18, 2020
@keszybz keszybz deleted the use-crypt_ra branch September 20, 2020 08:46
#include "missing_stdlib.h"
#include "random-util.h"
#include "string-util.h"
#include "strv.h"

int make_salt(char **ret) {

#ifdef XCRYPT_VERSION_MAJOR
#if HAVE_CRYPT_GENSALT_RA
Copy link
Member

Choose a reason for hiding this comment

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

@keszybz this caused a regression for me - I have crypt_gensalt_ra but not crypt_preferred_method which is used in the same ifdef. I'll send a PR to check for both in meson.build and here.

Copy link
Member

Choose a reason for hiding this comment

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

bluca added a commit to bluca/systemd that referenced this pull request Sep 25, 2020
After systemd#16981 only the presence of crypt_gensalt_ra
is checked, but there are cases where that function is available but crypt_preferred_method
is not, and they are used in the same ifdef.
Add a check for the latter as well.
bluca added a commit to bluca/systemd that referenced this pull request Sep 25, 2020
After systemd#16981 only the presence of crypt_gensalt_ra
is checked, but there are cases where that function is available but crypt_preferred_method
is not, and they are used in the same ifdef.
Add a check for the latter as well.
poettering pushed a commit that referenced this pull request Sep 28, 2020
After #16981 only the presence of crypt_gensalt_ra
is checked, but there are cases where that function is available but crypt_preferred_method
is not, and they are used in the same ifdef.
Add a check for the latter as well.
snarkmaster pushed a commit to snarkmaster/systemd that referenced this pull request Sep 30, 2020
After systemd#16981 only the presence of crypt_gensalt_ra
is checked, but there are cases where that function is available but crypt_preferred_method
is not, and they are used in the same ifdef.
Add a check for the latter as well.
ssahani pushed a commit to ssahani/systemd that referenced this pull request Oct 5, 2020
After systemd#16981 only the presence of crypt_gensalt_ra
is checked, but there are cases where that function is available but crypt_preferred_method
is not, and they are used in the same ifdef.
Add a check for the latter as well.
Yamakuzure pushed a commit to elogind/elogind that referenced this pull request Aug 10, 2021
After systemd/systemd#16981 only the presence of crypt_gensalt_ra
is checked, but there are cases where that function is available but crypt_preferred_method
is not, and they are used in the same ifdef.
Add a check for the latter as well.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
firstboot homed homed, homectl, pam_homed util-lib
Development

Successfully merging this pull request may close these issues.

Possible stack buffer overflow in user_record_make_hashed_password()
5 participants