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.

Sign up for GitHub

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

Jump to bottom

Potential use after free in openssl_util.c openssl_fingerprint #1231

Closed
fgirlich opened this issue Aug 16, 2022 · 2 comments
Closed

Potential use after free in openssl_util.c openssl_fingerprint #1231

fgirlich opened this issue Aug 16, 2022 · 2 comments
Labels
bug
Milestone
5.9.8

Comments

@fgirlich
Copy link

System:

  • OS: Debian 11
  • Kernel version): 5.10.0
  • strongSwan version(s): 5.8.2
  • Tested/confirmed with the latest version: no

Describe the bug
Before generating a fingerprint, the code first checks if we already have one in cache:

strongswan/src/libstrongswan/plugins/openssl/openssl_util.c

Line 99 in bcedd65

if (lib->encoding->get_cache(lib->encoding, type, key, fp))

If two threads access this concurrently for the same key, they will both fail to find a cached fingerprint and continue generating one.
The first to finish will add its fingerprint to the cache here:

strongswan/src/libstrongswan/plugins/openssl/openssl_util.c

Line 128 in bcedd65

lib->encoding->cache(lib->encoding, type, key, *fp);

The second thread will later also add its fingerprint to the cache while deleting the previously cached one:

strongswan/src/libstrongswan/credentials/cred_encoding.c

Lines 192 to 196 in bcedd65

/* free an encoding already associated to the cache */
if (chunk)
{
free(chunk->ptr);
free(chunk);

Meanwhile, the first thread may continue using a now invalid fingerprint.

Logs/Backtraces
This use after free was observed with version 5.8.2, built with -fsanitize=address. The offending code used to be in openssl_rsa_public_key.c but has since been moved to openssl_util.c. The bug seems to be still there, as described above.

==40841==ERROR: AddressSanitizer: heap-use-after-free on address 0x60300004bd30 at pc 0x7fb46ab09983 bp 0x7fb4628f5610 sp 0x7fb4628f4dc0
READ of size 20 at 0x60300004bd30 thread T10
    #0 0x7fb46ab09982 in __interceptor_memcpy ../../../../src/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:806
    #1 0x560d5778bd25 in memcpy_noop utils/utils/memory.h:49
    #2 0x560d5778c20c in chunk_create_clone utils/chunk.c:48
    #3 0x560d5779db91 in identification_create_from_encoding utils/identification.c:1739
    #4 0x560d5773e039 in get_private_by_cert credentials/credential_manager.c:1133
    #5 0x560d5773e661 in get_private credentials/credential_manager.c:1193
    #6 0x560d57ae67b3 in build sa/ikev2/authenticators/pubkey_authenticator.c:519
    #7 0x560d57a0ef78 in build_i sa/ikev2/tasks/ike_auth.c:702
    #8 0x560d579dffce in initiate sa/ikev2/task_manager_v2.c:640
    #9 0x560d579e19c9 in process_response sa/ikev2/task_manager_v2.c:803
    #10 0x560d579e83c9 in process_message sa/ikev2/task_manager_v2.c:1728
    #11 0x560d579c122d in process_message sa/ike_sa.c:1597
    #12 0x560d578f3aee in execute processing/jobs/process_message_job.c:74
    #13 0x560d57764b34 in process_job processing/processor.c:235
    #14 0x560d5776577d in process_jobs processing/processor.c:321
    #15 0x560d577a70d9 in thread_main threading/thread.c:331
    #16 0x7fb46a678ea6 in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x8ea6)
    #17 0x7fb46a5a8dee in __clone (/lib/x86_64-linux-gnu/libc.so.6+0xfddee)
0x60300004bd30 is located 0 bytes inside of 20-byte region [0x60300004bd30,0x60300004bd44)
freed by thread T7 here:
    #0 0x7fb46ab79b6f in __interceptor_free ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:123
    #1 0x560d5773520c in cache credentials/cred_encoding.c:194
    #2 0x560d577e0069 in openssl_rsa_fingerprint libstrongswan/plugins/openssl/openssl_rsa_public_key.c:391
    #3 0x560d577e015d in get_fingerprint libstrongswan/plugins/openssl/openssl_rsa_public_key.c:399
    #4 0x560d5773dff7 in get_private_by_cert credentials/credential_manager.c:1131
    #5 0x560d5773e661 in get_private credentials/credential_manager.c:1193
    #6 0x560d57ae67b3 in build sa/ikev2/authenticators/pubkey_authenticator.c:519
    #7 0x560d57a0ef78 in build_i sa/ikev2/tasks/ike_auth.c:702
    #8 0x560d579dffce in initiate sa/ikev2/task_manager_v2.c:640
    #9 0x560d579e19c9 in process_response sa/ikev2/task_manager_v2.c:803
    #10 0x560d579e83c9 in process_message sa/ikev2/task_manager_v2.c:1728
    #11 0x560d579c122d in process_message sa/ike_sa.c:1597
    #12 0x560d578f3aee in execute processing/jobs/process_message_job.c:74
    #13 0x560d57764b34 in process_job processing/processor.c:235
    #14 0x560d5776577d in process_jobs processing/processor.c:321
    #15 0x560d577a70d9 in thread_main threading/thread.c:331
    #16 0x7fb46a678ea6 in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x8ea6)
previously allocated by thread T10 here:
    #0 0x7fb46ab79e8f in __interceptor_malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:145
    #1 0x560d5783cc37 in allocate_hash libstrongswan/plugins/sha1/sha1_hasher.c:211
    #2 0x560d577dfe77 in openssl_rsa_fingerprint libstrongswan/plugins/openssl/openssl_rsa_public_key.c:382
    #3 0x560d577e015d in get_fingerprint libstrongswan/plugins/openssl/openssl_rsa_public_key.c:399
    #4 0x560d5773dff7 in get_private_by_cert credentials/credential_manager.c:1131
    #5 0x560d5773e661 in get_private credentials/credential_manager.c:1193
    #6 0x560d57ae67b3 in build sa/ikev2/authenticators/pubkey_authenticator.c:519
    #7 0x560d57a0ef78 in build_i sa/ikev2/tasks/ike_auth.c:702
    #8 0x560d579dffce in initiate sa/ikev2/task_manager_v2.c:640
    #9 0x560d579e19c9 in process_response sa/ikev2/task_manager_v2.c:803
    #10 0x560d579e83c9 in process_message sa/ikev2/task_manager_v2.c:1728
    #11 0x560d579c122d in process_message sa/ike_sa.c:1597
    #12 0x560d578f3aee in execute processing/jobs/process_message_job.c:74
    #13 0x560d57764b34 in process_job processing/processor.c:235
    #14 0x560d5776577d in process_jobs processing/processor.c:321
    #15 0x560d577a70d9 in thread_main threading/thread.c:331
    #16 0x7fb46a678ea6 in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x8ea6)
@tobiasbrunner tobiasbrunner added this to the 5.9.8 milestone Aug 18, 2022
@tobiasbrunner tobiasbrunner removed the new label Aug 18, 2022
@tobiasbrunner
Copy link
Member

Thanks a lot for the detailed report. It is as you describe, however, it's not limited to the openssl plugin. This exact pattern is used by all private/public key implementations (I counted 11 instances in the current codebase, plus what cred_encoding_t does itself in encode()).

A possible solution (that avoids having to add mutexes to all key implementations) would be to change cache() so it does not replace existing entries (it's not expected that the fingerprint for the same key changes). Which it then would either indicate with a boolean return value, allowing the caller to free its own fingerprint and fetch the value that was cached in the meantime, or we simply pass a pointer to a chunk_t and return the cached value instead, while freeing the passed one.

I've implemented the latter in the 1231-fp-cache branch. Please let me know what you think.

@fgirlich
Copy link
Author

This looks like a clean way to address the issue, thank you for the quick fix. I will backport it to 5.8.2 and test it in our setup. However, the issue is rare enough as it is and I am confident that this will fix it.

tobiasbrunner added a commit that referenced this issue Sep 20, 2022
@tobiasbrunner
cred-encoding: Avoid potential use after free when caching encodings
48e9267 
The pattern currently is to call get_cache(), generate the encoding
if that failed and then store it with cache().  The latter adopts the
passed encoding and frees any stored encoding.  However, the latter means
that if two threads concurrently fail to get a cached encoding and then
both generate and store one, one of the threads might use an encoding
that was freed by the other thread.

Since encodings are not expected to change, we can avoid this issue by
not replacing an existing cache entry and instead return that (while
freeing the passed value instead of the cached one).

Closes #1231
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
5.9.8
Development

When branches are created from issues, their pull requests are automatically linked.

1231-fp-cache strongswan/strongswan
2 participants
@tobiasbrunner @fgirlich