Comments (23)
Thanks @LeoneChen. We may need a static code scan in the future.
from ehsm.
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"));
from ehsm.
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)
from ehsm.
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);
...
}
from ehsm.
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
from ehsm.
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
from ehsm.
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
from ehsm.
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 variablekeybloblen
, and at this point,cmk
may be malloc-ed with insufficient heap memory, and thencmk->keybloblen
will 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?
from ehsm.
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.
from ehsm.
I create a PR #267 for above stack OOB
from ehsm.
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
ehsm/core/Enclave/enclave_hsm.cpp
Lines 861 to 878 in 7a54667
from ehsm.
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.
from ehsm.
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
Line 92 in 7a54667
Lines 61 to 144 in 7a54667
However, if not return -1, msgresp
is still null, and will cause crash in memcpy at line 167
ehsm/core/App/untrusted_enclave_msg_exchange.cpp
Lines 159 to 167 in 7a54667
Another example is at line 72, cause free an invalid pointer.
ehsm/core/App/untrusted_enclave_msg_exchange.cpp
Lines 62 to 72 in 7a54667
And msg1_response
is not initialized, better to initialized to null.
from ehsm.
Thanks for pointing the issues. Could you provide a patch to fix them? Thanks
from ehsm.
@yang8621 OK
from ehsm.
from ehsm.
Good! Hope EHSM become more and more popular
from ehsm.
Thanks for your contribution.
from ehsm.
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?
from ehsm.
Hi~
We've developed a fuzzing tool to detect this bugs.
from ehsm.
Thanks~ Is this tool public? If so, could u share a link?
from ehsm.
We will open source after paper is accepted
from ehsm.
ok. Looking forward to it~
from ehsm.
Related Issues (20)
- Bug: RSA key can't work
- Bug: memory leak HOT 1
- support CMK rotation HOT 2
- Support Intel Trust Authority Attestation
- Support SM9 (cryptography standard) HOT 5
- Upgrade to Openssl 3
- Trouble with setting environment of quick start HOT 16
- move the HMAC verification into core enclave
- support restful request with attestation token
- support cmk rotation
- unify the input params of digest for sign operation
- support client sdk for popular programming language(go, rust, python, java, scala, etc)
- Feature Request: Support WebKMS standard HOT 2
- ehsm_ksm_service application running on single mode stop during the generateQuote operation HOT 2
- Is there some bytes limit in Encrypt or AsymmetricEncrypt? HOT 4
- Bug: padding is returned as part of plaintext in the SM4_CBC decryption HOT 2
- Bug: Internal server exception when listing secrets with empty description
- ehsm_base
- [eHSM-KMS] How is the remote attestation realized? HOT 2
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from ehsm.