framework-pkcs15: Fix use after free issue

[MkfsSion]

Newly created pkcs15 secret key object is freed upon success, which maybe used if user calls other PKCS11 functions (like C_DestroyObject) and causes use after free issue.

Signed-off-by: MkfsSion [email protected]

Card:

Using reader with a card: Yubico YubiKey OTP+FIDO+CCID 00 00
Personal Identity Verification Card

Checklist

• PKCS#11 module is tested

[Jakuje]

Can you clarify what variable was used after free in your case. I am afraid this way you will leak some memory.

[MkfsSion]

Can you clarify what variable was used after free in your case. I am afraid this way you will leak some memory.

I entercountered a strange bug when using C_DeriveKey to derive ECDH shared secret. Here is my code, After calling C_DestroyObject, it returns CKR_SESSION_READ_ONLY error. After debuging, I found the value of secret key property session_object is zeroed by a sc_mem_clear call since the adajacent memory region is reallocated by a malloc call in pcsc_transmit().

Here is the backtrace:

(gdb) p &(key_obj->session_object)
$8 = (int *) 0x5555556405a0
(gdb) info b
Num     Type           Disp Enb Address            What
2       breakpoint     keep y   <MULTIPLE>         
        breakpoint already hit 1 time
2.1                         y   0x00007ffff734e1c2 in C_DeriveKey at pkcs11-object.c:1213
2.2                         y   0x00007ffff78835c0 in C_DeriveKey(CK_SESSION_HANDLE, CK_MECHANISM_PTR, CK_OBJECT_HANDLE, CK_ATTRIBUTE_PTR, CK_ULONG, CK_OBJECT_HANDLE_PTR) at main.cpp:1100
2.3                         y   0x00007ffff78c18f0 in SoftHSM::C_DeriveKey(unsigned long, _CK_MECHANISM*, unsigned long, _CK_ATTRIBUTE*, unsigned long, unsigned long*) at SoftHSM.cpp:6992
3       breakpoint     keep y   0x00007ffff734b27c in sc_create_object_int at pkcs11-object.c:96
        breakpoint already hit 1 time
4       breakpoint     keep y   0x00007ffff736100a in pkcs15_create_object at framework-pkcs15.c:2899
        breakpoint already hit 1 time
5       breakpoint     keep y   0x00007ffff735f775 in pkcs15_create_secret_key at framework-pkcs15.c:2409
        breakpoint already hit 1 time
(gdb) watch *0x5555556405a0
Hardware watchpoint 8: *0x5555556405a0
(gdb) c
Continuing.

Thread 1 "decrypt" hit Hardware watchpoint 8: *0x5555556405a0

Old value = 1
New value = 0
__memset_avx2_unaligned_erms () at ../sysdeps/x86_64/multiarch/memset-vec-unaligned-erms.S:422
422             rep     stosb
(gdb) bt
#0  __memset_avx2_unaligned_erms () at ../sysdeps/x86_64/multiarch/memset-vec-unaligned-erms.S:422
#1  0x00007ffff7a090d3 in explicit_bzero (s=<optimized out>, len=<optimized out>) at explicit_bzero.c:35
#2  0x00007ffff70be605 in sc_mem_clear (ptr=0x55555563fad0, len=4098) at sc.c:957
#3  0x00007ffff710f083 in pcsc_transmit (reader=0x555555620e70, apdu=0x7fffffffa7c0) at reader-pcsc.c:346
#4  0x00007ffff70df111 in sc_single_transmit (card=0x555555621b30, apdu=0x7fffffffa7c0) at apdu.c:379
#5  0x00007ffff70df9d5 in sc_transmit (card=0x555555621b30, apdu=0x7fffffffa7c0) at apdu.c:517
#6  0x00007ffff70dff4c in sc_transmit_apdu (card=0x555555621b30, apdu=0x7fffffffa890) at apdu.c:608
#7  0x00007ffff717e281 in piv_general_io
    (card=0x555555621b30, ins=135, p1=20, p2=157, sendbuf=0x7fffffffa9c0 "|e\202", sendbuflen=103, recvbuf=0x7fffffffb9c0 "|2\202\060\003p\330\021\036\301ΎʓH\205\035\367+{\213\026\300_\372\327y\323\006\201\263\335%\245f\265\311\317qC\317G)\027\273", <incomplete sequence \367>, recvbuflen=4096) at card-piv.c:526
#8  0x00007ffff7186851 in piv_validate_general_authentication
    (card=0x555555621b30, data=0x55555563fa60 "\004\067\203\321\062\005\300\r\377\016\362\356z\022\354=J\214q\322\300\347\361\234}\255;\216W\367i䫹", datalen=97, out=0x55555563d360 "", outlen=48) at card-piv.c:2366
#9  0x00007ffff71872d3 in piv_decipher
    (card=0x555555621b30, data=0x55555563fa60 "\004\067\203\321\062\005\300\r\377\016\362\356z\022\354=J\214q\322\300\347\361\234}\255;\216W\367i䫹", datalen=97, out=0x55555563d360 "", outlen=48) at card-piv.c:2475
#10 0x00007ffff70c98ad in sc_decipher
    (card=0x555555621b30, crgram=0x55555563fa60 "\004\067\203\321\062\005\300\r\377\016\362\356z\022\354=J\214q\322\300\347\361\234}\255;\216W\367i䫹", crgram_len=97, out=0x55555563d360 "", outlen=48) at sec.c:49
#11 0x00007ffff70fd77f in use_key
     (p15card=0x555555623190, obj=0x555555632d80, senv=0x7fffffffcbe0, card_command=0x7ffff70c968f <sc_decipher>, in=0x55555563fa60 "\004\067\203\321\062\005\300\r\377\016\362\356z\022\354=J\214q\322\300\347\361\234}\255;\216W\367i䫹", inlen=97, out=0x55555563d360 "", outlen=48) at pkcs15-sec.c:154
#12 0x00007ffff70feee2 in sc_pkcs15_derive
    (p15card=0x555555623190, obj=0x555555632d80, flags=2097152, in=0x55555563fa60 "\004\067\203\321\062\005\300\r\377\016\362\356z\022\354=J\214q\322\300\347\361\234}\255;\216W\367i䫹", inlen=97, out=0x55555563d360 "", poutlen=0x7fffffffd6f0--Type <RET> for more, q to quit, c to continue without paging--c
) at pkcs15-sec.c:383
#13 0x00007ffff7366325 in pkcs15_prkey_derive (session=0x55555563d170, obj=0x5555556348a0, pMechanism=0x55555563d738, pParameters=0x7fffffffda08 "\001", ulParametersLen=40, pData=0x55555563d360 "", pulDataLen=0x7fffffffd7e8) at framework-pkcs15.c:4549
#14 0x00007ffff7356a2d in sc_pkcs11_derive (operation=0x55555563d730, basekey=0x5555556348a0, pmechParam=0x7fffffffda08 "\001", ulmechParamLen=40, pData=0x55555563d360 "", pulDataLen=0x7fffffffd7e8) at mechanism.c:1223
#15 0x00007ffff7356738 in sc_pkcs11_deri (session=0x55555563d170, pMechanism=0x7fffffffd9c8, basekey=0x5555556348a0, key_type=3, hSession=93824993186160, hdkey=93824993188528, dkey=0x55555563dab0) at mechanism.c:1138
#16 0x00007ffff734e3d9 in C_DeriveKey (hSession=93824993186160, pMechanism=0x7fffffffd9c8, hBaseKey=93824993151136, pTemplate=0x7fffffffda50, ulAttributeCount=8, phKey=0x7fffffffd9f8) at pkcs11-object.c:1275
#17 0x0000555555555cf7 in pkcs11_derive_shared_secret_malloc (pubkey1=0x555555573420, out_len=0x7fffffffdc00) at /home/mkfssion/develop/ecdh/decrypt.c:141
#18 0x000055555555626c in main () at /home/mkfssion/develop/ecdh/decrypt.c:214

And the logic of determining whether the secret key object can be destroy in a RO session is to check if property session_object is set or not.

By the way, I also noticed that the value of args.key.data is assigned skey_info->data.value, but we free the pointer at last, which makes skey_info->data.value a dangling pointer.

Reference:
1: https://github.com/OpenSC/OpenSC/blob/master/src/pkcs11/framework-pkcs15.c#L5276

[Jakuje]

can you try to run your code with opensc under valgrind to see if there are some invalid memory access?

Do you use latest upstream or v0.22.0?

Do you have a backtrace from the use-after free crash? I still do not understand what is going on there and how the proposed patch is solving that.

[MkfsSion]

can you try to run your code with opensc under valgrind to see if there are some invalid memory access?

Do you use latest upstream or v0.22.0?

Do you have a backtrace from the use-after free crash? I still do not understand what is going on there and how the proposed patch is solving that.

Here is valgrind result:

➜ valgrind --leak-check=full ./build/decrypt
==90532== Memcheck, a memory error detector
==90532== Copyright (C) 2002-2022, and GNU GPL'd, by Julian Seward et al.
==90532== Using Valgrind-3.19.0 and LibVEX; rerun with -h for copyright info
==90532== Command: ./build/decrypt
==90532== 
Found module p11-kit-trust
Found module opensc
Slot 0 info:
Description: Yubico YubiKey OTP+FIDO+CCID 00 00                              Yubico                          
manufactureId: Yubico                          
==90532== Invalid read of size 4
==90532==    at 0x58BCD94: pkcs15_skey_get_attribute (framework-pkcs15.c:5276)
==90532==    by 0x589F591: C_DestroyObject (pkcs11-object.c:189)
==90532==    by 0x10AE98: pkcs11_derive_shared_secret_malloc (decrypt.c:173)
==90532==    by 0x10B30B: main (decrypt.c:236)
==90532==  Address 0x506f800 is 2,768 bytes inside a block of size 2,776 free'd
==90532==    at 0x484826F: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==90532==    by 0x58B400E: pkcs15_create_secret_key (framework-pkcs15.c:2580)
==90532==    by 0x58B53B7: pkcs15_create_object (framework-pkcs15.c:2982)
==90532==    by 0x589F432: sc_create_object_int (pkcs11-object.c:140)
==90532==    by 0x58A2331: C_DeriveKey (pkcs11-object.c:1262)
==90532==    by 0x10AD39: pkcs11_derive_shared_secret_malloc (decrypt.c:159)
==90532==    by 0x10B30B: main (decrypt.c:236)
==90532==  Block was alloc'd at
==90532==    at 0x484AA73: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==90532==    by 0x58B3DB0: pkcs15_create_secret_key (framework-pkcs15.c:2530)
==90532==    by 0x58B53B7: pkcs15_create_object (framework-pkcs15.c:2982)
==90532==    by 0x589F432: sc_create_object_int (pkcs11-object.c:140)
==90532==    by 0x58A2331: C_DeriveKey (pkcs11-object.c:1262)
==90532==    by 0x10AD39: pkcs11_derive_shared_secret_malloc (decrypt.c:159)
==90532==    by 0x10B30B: main (decrypt.c:236)
==90532== 
hsecret length is 72
HKDF secret is same==90532==

I'm using OpenSC master branch.
The issue I trying to fix is session secret key object (not in token, so CKA_TOKEN is false) can not be destroyed in read only session by C_DestroyObject call since the flag session_object is cleared by a memset call.
The memory is freed and reallocated to buffer used in pcsc_transmit function.

[Jakuje]

Thank you.

I went through the code and through the #1393 and it still looks like there is some problem with memory management. I think the key_obj should NOT be freed in either of the cases and you will probably hit very similar issue when the card will support the on-card session objects (myeid cards).

But your change skips the free(args.key.data); part on success. This function (pkcs15_create_secret_key) is also used when creating normal skey objects and it will leak the data allocated in this function in args.key.data. Most probably because it is not handled in when the card supports on-card session objects.

But let me check with @hhonkanen if it makes sense, as he is the author of this code.

[hhonkanen]

Looked at the code and history.

Let's see two possible cases:

1. if we create an object into the card with pkcs15init, temp_object is FALSE, so key_obj is not freed. This is ok with or without framework-pkcs15: Fix use after free issue #2558. It doesn't matter whether it is a on-card session object or a permanent (CKA_TOKEN=TRUE) object. CKA_VALUE can be NULL, if the object was created for key unwrapping. In this case, args.key.data will be NULL in the end. If key data was provided directly, pkcs15init creates the object into the card. I think it makes sense to free the key data in args.key.data from memory. We don't want to keep it in memory as it is now intended to be used on card. It also seems to me that it would leak, if we don't free it here. We got key_obj from pkcs15init, and args.key.data is not assigned to the pkcs15 object that will remain alive.

2. _token == FALSE, card does not support oncard session objects:
   args.key.data is assigned to skey_info->data.value, then args.key.data is set to NULL. -> nothing happens on free(args.key.data);
   temp_obj is set to TRUE -> key_obj is freed in the end. This logic was there even before 1393. We call __pkcs15_create_secret_key_object with key_obj, and it calls __pkcs15_create_object. It allocates space for a pkcs15_any_object struct, and assigns the key_obj to its member. I think it goes wrong here. We shouldn't free key_object if rv == CKR_OK.

Would it work better like this:

	rv = CKR_OK;

out:
	free(args.key.data); /* if allocated */
	
	if (rv != CKR_OK && temp_object) 	/* on error, free key_obj, unless it's created by pkcs15init. if OK, let it live as part of key_any_obj. */ 			
		free(key_obj); /* do not free if the object was created by pkcs15init. It will be freed in C_Finalize */
	return rv;

We might not need the temp_object flag at all though. I may have added it because of thinking that we should free key_obj also on rv == CKR_OK or it would leak, not understanding that it must remain in memory as part of key_any_obj.

// Edit Jakuje: Added formatting of the code block to improve readability.

[MkfsSion]

We might not need the temp_object flag at all though. I may have added it because of thinking that we should free key_obj also on rv == CKR_OK or it would leak, not understanding that it must remain in memory as part of key_any_obj.

Thanks for your clarification. Since I am not familiar to the code base, I have a question, should we free the key_obj created by pkcs15init if rv is not CKR_OK?
And would you mind me update the commit with changes and add Co-authored-by line in commit or you will create a new PR and then close this one?

[Jakuje]

Thank you for having a look into this. I came to similar conclusion as @hhonkanen . I think the key_obj should be freed only on error, because otherwise it is assigned in __pkcs15_create_secret_key_object to &key_any_obj and that is stored in the object list and should be freed on cleanup.

[hhonkanen]

@MkfsSion I looked at sc_pkcs15init_store_secret_key. If an error occurs there, it does cleanup there. If it returns rv other than CKR_OK, it frees the object if it was already created, and key_obj is NULL when it returns.

Please feel free to make the changes and commit. I don't currently have an OpenSC development environment ready to work with, so I'm happy if you can do it.

[Jakuje]

Thank you for the contribution, investigation and patience with review!