intel
Bugs found in ehsm
Hello~
Heap OOB
enclave_decrypt defined in EDL set cmk_size as cmk's size, but attacker can feed cmk_size smaller then sizeof(ehsm_keyblob_t), and TBridge only malloc cmk_size for it.
public sgx_status_t enclave_decrypt([in, size=cmk_size] ehsm_keyblob_t* cmk, size_t cmk_size,
[in, size=aad_size] ehsm_data_t *aad, size_t aad_size,
[in, size=ciphertext_size] ehsm_data_t *ciphertext, size_t ciphertext_size,
[in, out, size=plaintext_size] ehsm_data_t *plaintext, size_t plaintext_size);
Then in function enclave_decrypt, cmk->keybloblen will out-of-bound access invalid memory, e.g. cmk->keybloblen is at offset 0x30, but cmk_size is fed with 0x20.
sgx_status_t enclave_decrypt(ehsm_keyblob_t *cmk, size_t cmk_size,
ehsm_data_t *aad, size_t aad_size,
ehsm_data_t *ciphertext, size_t ciphertext_size,
ehsm_data_t *plaintext, size_t plaintext_size)
{
sgx_status_t ret = SGX_ERROR_UNEXPECTED;
if (cmk == NULL ||
cmk_size != APPEND_SIZE_TO_KEYBLOB_T(cmk->keybloblen) ||
cmk->keybloblen == 0 ||
cmk->metadata.origin != EH_INTERNAL_KEY)
return SGX_ERROR_INVALID_PARAMETER;
...
}
Stack OOB
static bool rsa_decryption_with_pkcs_oaep(map<string, string> test_vector)
{
uint8_t _plaintext[VECTOR_LENGTH("plaintext")] = {0};
RSA_private_decrypt(RSA_size(key), &*ciphertext, _plaintext, key, 4);
}
_plaintext is a stack buffer with size VECTOR_LENGTH("plaintext"), however openssl ask at least RSA_size(rsa) - 42 (RSA_PKCS1_OAEP_PADDING mode) for to buffer(i.e. _plaintext). (See doc.)
RSA_private_decrypt() decrypts the flen bytes at from using the private key rsa and stores the plaintext in to. flen should be equal to RSA_size(rsa) but may be smaller, when leading zero bytes are in the ciphertext. Those are not important and may be removed, but RSA_public_encrypt() does not do that. to must point to a memory section large enough to hold the maximal possible decrypted data (which is equal to RSA_size(rsa) for RSA_NO_PADDING, RSA_size(rsa) - 11 for the PKCS #1 v1.5 based padding modes and RSA_size(rsa) - 42 for RSA_PKCS1_OAEP_PADDING). padding is the padding mode that was used to encrypt the data. to and from may overlap.
In case 1 in aes_gcm_crypto_test_vectors, VECTOR_LENGTH("plaintext") is 28 while RSA_size(rsa) is 128, then 28 < 128 - 42
Thus, in RSA_private_decrypt, a stack OOB has occurred.
Tips: uint8_t _plaintext[VECTOR_LENGTH("plaintext")] = {0} is unsafe since variable-sized object may not be initialized, better to memset to 0 for _plaintext, like memset(_plaintext, 0, VECTOR_LENGTH("plaintext"));
Thanks for pointing the issues.
for the 2nd one, we will take a look and fix it soon. (also welcome for your contribution if you want to..)
for the 1st one, could you provide more details, since i'm not quite understand your point. Because it will return error directly when it hits the below condition: cmk_size != APPEND_SIZE_TO_KEYBLOB_T(cmk->keybloblen)
Above EDL rule in enclave_hsm.edl for enclave_decrypt will create funtion sgx_enclave_decrypt in enclave_hsm_t.c, sgx_enclave_decrypt is a wrapper in TBridge called by SGXSDK's trts_ecall (with a function lookup operation), and sgx_enclave_decrypt will call real enclave_decrypt.
If you specify size of cmk with [in, size=cmk_size] ehsm_keyblob_t* cmk, TBridge will malloc size of ms->ms_cmk_size for it, and followed by a memory clone operation to ensure [in] pointer is resided in Enclave. Here, _in_cmk is the cmk for real enclave_decrypt, and malloc-ed size is cmk_size. More detail you can refer to SGXSDK's manual.
static sgx_status_t SGX_CDECL sgx_enclave_decrypt(void* pms)
{
...
ehsm_keyblob_t* _tmp_cmk = ms->ms_cmk;
size_t _tmp_cmk_size = ms->ms_cmk_size;
size_t _len_cmk = _tmp_cmk_size;
...
if (_tmp_cmk != NULL && _len_cmk != 0) {
_in_cmk = (ehsm_keyblob_t*)malloc(_len_cmk);
if (_in_cmk == NULL) {
status = SGX_ERROR_OUT_OF_MEMORY;
goto err;
}
if (memcpy_s(_in_cmk, _len_cmk, _tmp_cmk, _len_cmk)) {
status = SGX_ERROR_UNEXPECTED;
goto err;
}
}
...
ms->ms_retval = enclave_decrypt(_in_cmk, _tmp_cmk_size, _in_aad, _tmp_aad_size, _in_ciphertext, _tmp_ciphertext_size, _in_plaintext, _tmp_plaintext_size);
...
}
I think if you want to specify ehsm_keyblob_t *cmk only point to one ehsm_keyblob_t object, you can only set [in] EDL attibute, if you want to transfer more then one element, you can set [in, count=xxx], this will malloc count * sizeof(ehsm_keyblob_t) for cmk
for the 1st one, could you provide more details, since i'm not quite understand your point. Because it will return error directly when it hits the below condition: cmk_size != APPEND_SIZE_TO_KEYBLOB_T(cmk->keybloblen)
About this, before APPEND_SIZE_TO_KEYBLOB_T is executed, cmk will firstly index it's member variable keybloblen, and at this point, cmk may be malloc-ed with insufficient heap memory, and then cmk->keybloblen will cause Out-of-Bound access
for the 2nd one, we will take a look and fix it soon. (also welcome for your contribution if you want to..)
OK, I'll try to fix
for the 1st one, could you provide more details, since i'm not quite understand your point. Because it will return error directly when it hits the below condition: cmk_size != APPEND_SIZE_TO_KEYBLOB_T(cmk->keybloblen)
About this, before
APPEND_SIZE_TO_KEYBLOB_Tis executed,cmkwill firstly index it's member variablekeybloblen, and at this point,cmkmay be malloc-ed with insufficient heap memory, and thencmk->keybloblenwill cause Out-of-Bound access
Yes, Thanks. It seems is a common issue for all of the interfaces defined in the EDL file. Currently, we transferred a data structure but the size is used another variable rather than the size of original structure.
But for your suggestion "you can set [in, count=xxx], this will malloc count * sizeof(ehsm_keyblob_t) for cmk", it may not suitable for this case, because the buffer appended to the structure is not an integer multiple of an existing structure.
How do you think if we add more checks in each interfaces to check whether the in_size is larger than the size of structure before doing the existing checks? or do you have some better idea to handle it?
Yes, I find [in, count=xxx] is not suitable for this case. Since type of cmk that ehsm_keyblob_t has a variable length array member
typedef struct {
ehsm_keymetadata_t metadata;
uint32_t keybloblen;
uint8_t keyblob[0];
} ehsm_keyblob_t;
Maybe we can pass another parameter to specify len of keyblob, and add check at enclave whether "keyblob_len * sizeof(uint8_t) + sizeof(ehsm_keyblob_t) == cmk_size" and then "cmk->keybloblen == keyblob_len", but it seems ugly.
Oops, lot's of interfaces in EDL like enclave_sign has same problem as enclave_decrypt, better to figure out a general way to fix them
- [ ] enclave_create_key
- [ ] enclave_get_public_key
- [ ] enclave_encrypt
- [ ] enclave_decrypt
- [ ] enclave_asymmetric_encrypt
- [ ] enclave_asymmetric_decrypt
- [ ] enclave_sign
- [ ] enclave_verify
- [ ] enclave_generate_datakey
- [ ] enclave_export_datakey
p_sgx_quote->report_body.mr_signer.m[i] and p_sgx_quote->report_body.mr_enclave.m[i] can overflow quote
https://github.com/intel/ehsm/blob/7a54667c3f2ea880faff15a0eb6e9f36a9581bfe/core/Enclave/enclave_hsm.cpp#L861-L878
Yes, it's a common issue now.
A quick fix is to add a check at the beginning of the original checks, to make sure the size is larger than the expected structure size in each interfaces.
Forget to set return value
You seems fortget to set ret to -1 at line 92, it's already failed, and fiforesponse will not set to an valid memroy
https://github.com/intel/ehsm/blob/7a54667c3f2ea880faff15a0eb6e9f36a9581bfe/core/App/fifo.cpp#L92
https://github.com/intel/ehsm/blob/7a54667c3f2ea880faff15a0eb6e9f36a9581bfe/core/App/fifo.cpp#L61-L144
However, if not return -1, msgresp is still null, and will cause crash in memcpy at line 167
https://github.com/intel/ehsm/blob/7a54667c3f2ea880faff15a0eb6e9f36a9581bfe/core/App/untrusted_enclave_msg_exchange.cpp#L159-L167
Another example is at line 72, cause free an invalid pointer. https://github.com/intel/ehsm/blob/7a54667c3f2ea880faff15a0eb6e9f36a9581bfe/core/App/untrusted_enclave_msg_exchange.cpp#L62-L72
And msg1_response is not initialized, better to initialized to null.
https://github.com/intel/ehsm/blob/7a54667c3f2ea880faff15a0eb6e9f36a9581bfe/core/App/untrusted_enclave_msg_exchange.cpp#L55
Thanks for pointing the issues. Could you provide a patch to fix them? Thanks
Hi. I am trying to test this sgx-based project. May I ask how you discovered these bugs mentioned above. Did you use some fuzzing tools or just manually check the code?
