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

cryptenroll --recovery-key crashes #19203

Closed
grawity opened this issue Apr 3, 2021 · 10 comments · Fixed by #19653
Closed

cryptenroll --recovery-key crashes #19203

grawity opened this issue Apr 3, 2021 · 10 comments · Fixed by #19653

Comments

@grawity
Copy link
Contributor

grawity commented Apr 3, 2021

systemd version the issue has been seen with

v248 (Arch Linux build)

Used distribution

Arch

Linux kernel version used (uname -a)

5.11.8-arch1-1

CPU architecture issue was seen on

amd64

Expected behaviour you didn't see

It doesn't crash.

Unexpected behaviour you saw

It crashes.

Steps to reproduce the problem

systemd-cryptenroll --recovery-key

Additional program output to the terminal or log subsystem illustrating the issue

# SYSTEMD_LOG_LEVEL=debug systemd-cryptenroll --recovery-key /dev/nvme0n1p2
Trying to load LUKS2 crypt type from device /dev/nvme0n1p2.
Crypto backend (OpenSSL 1.1.1k  25 Mar 2021) initialized in cryptsetup library version 2.3.5.
Detected kernel Linux 5.11.8-arch1-1 x86_64.
Loading LUKS2 header (repair disabled).
Acquiring read lock for device /dev/nvme0n1p2.
Opening lock resource file /run/cryptsetup/L_259:2
Verifying lock handle for /dev/nvme0n1p2.
Device /dev/nvme0n1p2 READ lock taken.
Trying to read primary LUKS2 header at offset 0x0.
Opening locked device /dev/nvme0n1p2
Veryfing locked device handle (bdev)
LUKS2 header version 2 of size 16384 bytes, checksum sha256.
Checksum:a8b702198d6c275f8a05700c82d96b262d6a04ad21783ad61cf13ac613705737 (on-disk)
Checksum:a8b702198d6c275f8a05700c82d96b262d6a04ad21783ad61cf13ac613705737 (in-memory)
Trying to read secondary LUKS2 header at offset 0x4000.
Reusing open ro fd on device /dev/nvme0n1p2
LUKS2 header version 2 of size 16384 bytes, checksum sha256.
Checksum:4cf340067f1f510071ddf0db08e5c03466d95f230fa412576e52a1612b2a58d2 (on-disk)
Checksum:4cf340067f1f510071ddf0db08e5c03466d95f230fa412576e52a1612b2a58d2 (in-memory)
Device size 549755813888, offset 2097152.
Device /dev/nvme0n1p2 READ lock released.
PBKDF argon2i, time_ms 2000 (iterations 0), max_memory_kb 1048576, parallel_threads 4.
🔐 Please enter current passphrase for disk /dev/nvme0n1p2: *******************Failed to adjust kernel keyring key timeout: Permission denied
Added key to kernel keyring as 823387308.

Keyslot 0 priority 1 != 2 (required), skipped.
Keyslot 1 priority 1 != 2 (required), skipped.
Trying to open LUKS2 keyslot 0.
Reading keyslot area [0x8000].
Acquiring read lock for device /dev/nvme0n1p2.
Opening lock resource file /run/cryptsetup/L_259:2
Verifying lock handle for /dev/nvme0n1p2.
Device /dev/nvme0n1p2 READ lock taken.
Reusing open ro fd on device /dev/nvme0n1p2
Device /dev/nvme0n1p2 READ lock released.
Verifying key from keyslot 0, digest 0.
Digest 0 (pbkdf2) verify failed with -1.
Trying to open LUKS2 keyslot 1.
Reading keyslot area [0x28000].
Acquiring read lock for device /dev/nvme0n1p2.
Opening lock resource file /run/cryptsetup/L_259:2
Verifying lock handle for /dev/nvme0n1p2.
Device /dev/nvme0n1p2 READ lock taken.
Reusing open ro fd on device /dev/nvme0n1p2
Device /dev/nvme0n1p2 READ lock released.
Verifying key from keyslot 1, digest 0.
*** buffer overflow detected ***: terminated
Aborted (core dumped)
(returned 134 or SIGABRT)

Backtrace:

#0  0x00007f1143046ef5 in raise () from /usr/lib/libc.so.6
No symbol table info available.
#1  0x00007f1143030862 in abort () from /usr/lib/libc.so.6
No symbol table info available.
#2  0x00007f1143088f38 in __libc_message () from /usr/lib/libc.so.6
No symbol table info available.
#3  0x00007f11431188ba in __fortify_fail () from /usr/lib/libc.so.6
No symbol table info available.
#4  0x00007f1143117154 in __chk_fail () from /usr/lib/libc.so.6
No symbol table info available.
#5  0x00007f1143118865 in __explicit_bzero_chk () from /usr/lib/libc.so.6
No symbol table info available.
#6  0x00007f11436af519 in explicit_bzero (__len=<optimized out>, __dest=0x558626f77520) at /usr/include/bits/string_fortified.h:72
No locals.
#7  explicit_bzero_safe (l=<optimized out>, p=0x558626f77520) at ../systemd-stable/src/basic/memory-util.h:77
No locals.
#8  erase_and_free (p=0x558626f77520) at ../systemd-stable/src/basic/memory-util.h:92
        l = <optimized out>
        l = <optimized out>
#9  erase_and_freep (p=<synthetic pointer>) at ../systemd-stable/src/basic/memory-util.h:97
No locals.
#10 make_recovery_key (ret=ret@entry=0x7ffe6c332478) at ../systemd-stable/src/basic/recovery-key.c:76
        formatted = 0x0
        key = 0x558626f77520 ":\242\023\353\232\t5\376J0S\351f\221\231\265\027\206\260\320\342F\243's>\331R\266\377z\237"
        r = <optimized out>
        __PRETTY_FUNCTION__ = "make_recovery_key"
        _ptr_ = <optimized out>
#11 0x0000558626738f7d in enroll_recovery (volume_key_size=32, volume_key=0x558626f7f520, cd=0x558626f6d3e0) at ../systemd-stable/src/cryptenroll/cryptenroll-recovery.c:28
        keyslot = <optimized out>
        q = <optimized out>
        node = 0x558626f6d8e0 "/dev/nvme0n1p2"
        v = 0x0
        password = 0x558626f7c700 "elldbeun-klckegvu-flecgeuk-hhkbkkng-bijhnctc-udfhledi-ieeutkgd-nhvvilkv"
        keyslot_as_string = 0x0
        r = <optimized out>
        v = <optimized out>
        password = <optimized out>
        keyslot_as_string = <optimized out>
        keyslot = <optimized out>
        r = <optimized out>
        q = <optimized out>
        node = <optimized out>
        __PRETTY_FUNCTION__ = {<optimized out> <repeats 16 times>}
        __func__ = {<optimized out> <repeats 16 times>}
        rollback = <optimized out>
        _level = <optimized out>
        _e = <optimized out>
        _level = <optimized out>
        _e = <optimized out>
        _level = <optimized out>
        _e = <optimized out>
        _x = <optimized out>
        _x = <optimized out>
        _x = <optimized out>
        _x = <optimized out>
        _level = <optimized out>
        _e = <optimized out>
        _level = <optimized out>
        _e = <optimized out>
        _level = <optimized out>
        _e = <optimized out>
        _level = <optimized out>
        _e = <optimized out>
#12 run (argv=<optimized out>, argc=<optimized out>) at ../systemd-stable/src/cryptenroll/cryptenroll.c:481
        slot = <optimized out>
        cd = 0x558626f6d3e0
        vk = 0x558626f7f520
        vks = 32
        r = 0
        cd = <optimized out>
        vk = <optimized out>
        vks = <optimized out>
        slot = <optimized out>
        r = <optimized out>
        __func__ = {<optimized out>, <optimized out>, <optimized out>, <optimized out>}
        _level = <optimized out>
        _e = <optimized out>
#13 main (argc=<optimized out>, argv=<optimized out>) at ../systemd-stable/src/cryptenroll/cryptenroll.c:518

Annoyingly, if I just compile the same v248 from git it seems to work fine.

@poettering
Copy link
Member

this is weird, really weird. must be some memory corruption somewhere. But I can't see it. Maybe the memory is corrupted elsewhere, and it just becomes visible in make_recovery_key(). Any chance you can reproduce this in valgrind with debug symbols?

@poettering poettering added cryptsetup needs-reporter-feedback ❓ There's an unanswered question, the reporter needs to answer labels Apr 6, 2021
@grawity
Copy link
Contributor Author

grawity commented Apr 6, 2021

Works perfectly fine under valgrind ¯\_(ツ)_/¯

@poettering
Copy link
Member

worked fine here too... weird weird weird.

@poettering
Copy link
Member

no idea what to propose next

poettering added a commit to poettering/systemd that referenced this issue Apr 6, 2021
Let's ensure our key sizes calculations are correct.

This doesn't actually change anything, just adds more safety checks.
Inspired by systemd#19203, but not a fix.
@grawity
Copy link
Contributor Author

grawity commented Apr 6, 2021

Figured out why binaries from Arch's packaging crash and my manual git builds don't. Arch packages are built with CPPFLAGS="-D_FORTIFY_SOURCE=2" – that seems to be the main difference.

for the record, Arch's standard cflags up until now were:

CPPFLAGS="-D_FORTIFY_SOURCE=2"
CFLAGS="-march=x86-64 -mtune=generic -O2 -pipe -fno-plt"
CXXFLAGS="-march=x86-64 -mtune=generic -O2 -pipe -fno-plt"
LDFLAGS="-Wl,-O1,--sort-common,--as-needed,-z,relro,-z,now"

DEBUG_CFLAGS="-g -fvar-tracking-assignments"
DEBUG_CXXFLAGS="-g -fvar-tracking-assignments"

(They changed recently, but I still have the old configuration on this machine.)

@grawity
Copy link
Contributor Author

grawity commented Apr 6, 2021

So it crashes inside erase_and_freep(&key). Nothing wrong with formatted it seems.

I noticed that even though only 32 bytes were allocated, malloc_usable_size(key) actually returns 40.

So explicit_bzero() gets called with len=40, which is compared against __glibc_objsize0(key) which is of course 32...

@poettering
Copy link
Member

so some time back we ran into this already with valgrind, and after discussing with glibc upstream and people we agreed that our logic was right and that valgrind should be fixed to honour malloc_usable_size(key)... i have the suspicion that gcc's fortify should be fixed the same way.

that said, maybe we can do something on our side too, and before relying on malloc_usable_size() call realloc() first with that size, which should then update the size in all static checkers too.

@poettering
Copy link
Member

See #12965

@poettering
Copy link
Member

In particular this: https://sourceware.org/bugzilla/show_bug.cgi?id=24775#c6

Where Florian Weimer makes clear that msan was borked, because it didn't fix up malloc_usable_size() to return what was usable...

hence I#d argue the fortify stuff in gcc is similarly broken and should be fixed.

I am a bit curious why this never has shown up on Fedora like this, we use the same fortify settings after all

poettering added a commit that referenced this issue Apr 6, 2021
Let's ensure our key sizes calculations are correct.

This doesn't actually change anything, just adds more safety checks.
Inspired by #19203, but not a fix.
@ghost
Copy link

ghost commented Apr 30, 2021

Hi,

any news on this issue ?

poettering added a commit to poettering/systemd that referenced this issue May 18, 2021
It's a wrapper around malloc_usable_size() that is supposed to be
compatible with _FORTIFY_SOURCES=1, by taking the
__builtin_object_size() data into account, the same way as the
_FORTIFY_SOURCES=1 logic does.

Fixes: systemd#19203
poettering added a commit to poettering/systemd that referenced this issue May 19, 2021
It's a wrapper around malloc_usable_size() that is supposed to be
compatible with _FORTIFY_SOURCES=1, by taking the
__builtin_object_size() data into account, the same way as the
_FORTIFY_SOURCES=1 logic does.

Fixes: systemd#19203
poettering added a commit to poettering/systemd that referenced this issue May 19, 2021
It's a wrapper around malloc_usable_size() that is supposed to be
compatible with _FORTIFY_SOURCES=1, by taking the
__builtin_object_size() data into account, the same way as the
_FORTIFY_SOURCES=1 logic does.

Fixes: systemd#19203
poettering added a commit to poettering/systemd that referenced this issue May 19, 2021
It's a wrapper around malloc_usable_size() that is supposed to be
compatible with _FORTIFY_SOURCES=1, by taking the
__builtin_object_size() data into account, the same way as the
_FORTIFY_SOURCES=1 logic does.

Fixes: systemd#19203
poettering added a commit to poettering/systemd that referenced this issue May 19, 2021
It's a wrapper around malloc_usable_size() that is supposed to be
compatible with _FORTIFY_SOURCES=1, by taking the
__builtin_object_size() data into account, the same way as the
_FORTIFY_SOURCES=1 logic does.

Fixes: systemd#19203
dakr pushed a commit to dakr/systemd that referenced this issue Jun 14, 2021
It's a wrapper around malloc_usable_size() that is supposed to be
compatible with _FORTIFY_SOURCES=1, by taking the
__builtin_object_size() data into account, the same way as the
_FORTIFY_SOURCES=1 logic does.

Fixes: systemd#19203
@keszybz keszybz removed the needs-reporter-feedback ❓ There's an unanswered question, the reporter needs to answer label Jun 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

3 participants