Fix Enclave Interfaces of WAMR SGX version; Set pointer to NULL after free, otherwise cause UAF/DF #2357

LeoneChen · 2023-07-13T21:54:49Z

Fix Enclave Interfaces of WAMR SGX version
Set pointer to NULL after free, otherwise cause UAF/DF

product-mini/platforms/linux-sgx/enclave-sample/Enclave/Enclave.edl

TianlongLiang · 2023-07-24T05:27:34Z

Hi, I see that there some CI tests are failing, seems not able to call ecall_handle_cmd_exec_app_func()?

BTW, what is the purpose of using idx rather than raw pointer for module_inst and other data? Is it for easier management with the help of the newly introduced pointer manager?

LeoneChen · 2023-07-24T06:00:17Z

BTW, what is the purpose of using idx rather than raw pointer for module_inst and other data? Is it for easier management with the help of the newly introduced pointer manager?

Use raw pointer is very dangerous, since it can point to in-Enclave memory, it's hard to check pointed memory is a valid context content or invalid sensitive in-Enclave data. So use indexes to replace the raw pointers when passing enclave_module and wasm_module_inst across the SGX boundary. This method is decribed in TeeRex, and at the end of section 5.3

The developers of the Rust SGX SDK acknowledged the problem and promptly updated their code. Akin to our suggestions to the developers, the enclave code now utilizes session identifiers instead of pointers to identify TLS sessions; similar to using file descriptors on Unix-like systems. Upon session creation in tls_client_new, the pointer to the TLS session object is now inserted into a hashmap, which is then used to map the identifier in subsequent ECALLs. Hence, no pointers are passed on the host-to-enclave boundary. This drastically reduces the attack surface of the enclave and eradicates both the vulnerability pattern Returning pointers to enclave memory (P2) and Pointers to Overlapping Memory (P3).

TianlongLiang · 2023-07-24T06:07:56Z

Thanks! That makes sense

LeoneChen · 2023-07-24T06:10:24Z

Hi, I see that there some CI tests are failing, seems not able to call ecall_handle_cmd_exec_app_func()?

Do you means CI tests successfully call ecall_handle_cmd_exec_app_func, but crash or return error inside this function? Or CI tests can't successfully call ecall_handle_cmd_exec_app_func？If it's the first situation, maybe the check added by myself in ecall_handle_cmd_exec_app_func result in checking fault

LeoneChen · 2023-07-24T06:16:00Z

Thanks! That makes sense

This part of code maybe can be optimized. I think it's better to avoid using lock

LeoneChen · 2023-07-24T06:17:31Z

Hi, I see that there some CI tests are failing, seems not able to call ecall_handle_cmd_exec_app_func()?

Do you means CI tests successfully call ecall_handle_cmd_exec_app_func, but crash or return error inside this function? Or CI tests can't successfully call ecall_handle_cmd_exec_app_func？If it's the first situation, maybe the check added by myself in ecall_handle_cmd_exec_app_func result in checking fault

Can you provide more detail about it? Then I can figure out why it failed

TianlongLiang · 2023-07-24T06:18:14Z

Do you means CI tests successfully call ecall_handle_cmd_exec_app_func, but crash or return error inside this function? Or CI tests can't successfully call ecall_handle_cmd_exec_app_func？If it's the first situation, maybe the check added by myself in ecall_handle_cmd_exec_app_func result in checking fault

https://github.com/bytecodealliance/wasm-micro-runtime/actions/runs/5548246017/job/15029123379?pr=2357

Some spec tests are failing, output is something like:

Starting interpreter for module '/tmp/tmp9j2olynn.aot'
Running: ../../../product-mini/platforms/linux-sgx/enclave-sample/iwasm --heap-size=0 -v=0 --repl /tmp/tmp9j2olynn.aot
Testing(return) i32_add  = 0x102:i32
THE FINAL EXCEPTION IS out should be in a form likes numbers:type, but Call ecall_handle_cmd_exec_app_func() failed.

You can run a hello world aot file with similar cmd option to reproduce it to double-check whether it's due to the newly added check

TianlongLiang · 2023-07-24T06:22:47Z

Can you provide more detail about it? Then I can figure out why it failed

On your local machine, you can also run spec tests to check:

# assume you install the SDK in /opt/intel/sgxsdk/
source /opt/intel/sgxsdk/environment
# in WAMR root directory
cd tests/wamr-test-suites
# the shell script will automatically compile needed component and run spec test on sgx iwasm
./test_wamr.sh -b -x -t aot -s spec -p -P

LeoneChen · 2023-07-24T06:22:56Z

Do you means CI tests successfully call ecall_handle_cmd_exec_app_func, but crash or return error inside this function? Or CI tests can't successfully call ecall_handle_cmd_exec_app_func？If it's the first situation, maybe the check added by myself in ecall_handle_cmd_exec_app_func result in checking fault

https://github.com/bytecodealliance/wasm-micro-runtime/actions/runs/5548246017/job/15029123379?pr=2357

Some spec tests are failing, output is something like:
Starting interpreter for module '/tmp/tmp9j2olynn.aot'
Running: ../../../product-mini/platforms/linux-sgx/enclave-sample/iwasm --heap-size=0 -v=0 --repl /tmp/tmp9j2olynn.aot
Testing(return) i32_add  = 0x102:i32
THE FINAL EXCEPTION IS out should be in a form likes numbers:type, but Call ecall_handle_cmd_exec_app_func() failed.
You can run a hello world aot file with similar cmd option to reproduce it to double-check whether it's due to the newly added check

Ok, I'll try aot mode. I currently is preparing thesis proposal, so maybe late to fix it.

LeoneChen · 2023-07-24T06:23:25Z

Can you provide more detail about it? Then I can figure out why it failed

On your local machine, you can also run spec tests to check:
# assume you install the SDK in /opt/intel/sgxsdk/
source /opt/intel/sgxsdk/environment
# in WAMR root directory
cd tests/wamr-test-suites
# the shell script will automatically compile needed component and run spec test on sgx iwasm
./test_wamr.sh -b -x -t aot -s spec -p -P

Thanks

TianlongLiang · 2023-07-24T06:26:00Z

No hurry, good luck with your thesis!

LeoneChen · 2023-07-24T06:26:57Z

No hurry, good luck with your thesis!

Thanks!

LeoneChen · 2023-07-28T20:30:45Z

@TianlongLiang Hello, I fixed the parameters check in 1306d68, and all CI tests passed

TianlongLiang · 2023-07-31T06:16:32Z

LGTM

product-mini/platforms/linux-sgx/enclave-sample/Enclave/Enclave.cpp

wenyongh · 2023-08-01T07:28:24Z

product-mini/platforms/linux-sgx/enclave-sample/Enclave/Enclave.cpp

    }

    wasm_application_execute_main(module_inst, app_argc - 1, app_argv + 1);

+    for (uint32 i = 0; i < app_argc; i++) {


The return value may be written back into app_argv[0..1], had better copy them back into u_app_argv

It need some time

The return value may be written back into app_argv[0..1], had better copy them back into u_app_argv

Hello I want to confirm, it means 1. put the return value (bool type) into app_argv[0..1] or 2. content in app_argv can be modified by wasm_application_execute_main, and you want this modification propagates to outside of Enclave

Hi, I checked again, wasm_application_execute_main puts the return value into *(int *)argv, here should be *(int *)(app_argv + 1), see:
https://github.com/bytecodealliance/wasm-micro-runtime/blob/main/core/iwasm/include/wasm_export.h#L836-L838

So my understanding is that we had better write*(int *)(app_argv + 1) back into *(int *)u_app_argv?

I got it, so it's case 2, and I have to let the content in *(int*)argv propagates to outside Enclave

LeoneChen · 2023-08-01T09:20:56Z

CI tests failed again. What is nuttx. How can I reproduce this problem in my local machine

wenyongh · 2023-08-01T09:29:55Z

CI tests failed again. What is nuttx. How can I reproduce this problem in my local machine

Nuttx CI issue was fixed with PR #2414, could you rebase again?

LeoneChen · 2023-08-01T09:31:18Z

CI tests failed again. What is nuttx. How can I reproduce this problem in my local machine

Nuttx CI issue was fixed with PR #2414, could you rebase again?

OK, I try it

2. Set pointer to NULL after free, otherwise cause UAF/DF

wenyongh · 2023-08-01T11:24:57Z

@LeoneChen Many thanks for the fixing, it really improves the security of WAMR linux-sgx. We are preparing to release 1.2.3, since the changes of this PR are a little big and complex, we (and some customers) need to more time to test, I think we had better merge this PR after the new release is created, and currently we can create another PR to fix the issue of wait_map = NULL; in wasm_shared_memory_destroy. Is it OK for you?

LeoneChen · 2023-08-01T13:06:34Z

@LeoneChen Many thanks for the fixing, it really improves the security of WAMR linux-sgx. We are preparing to release 1.2.3, since the changes of this PR are a little big and complex, we (and some customers) need to more time to test, I think we had better merge this PR after the new release is created, and currently we can create another PR to fix the issue of wait_map = NULL; in wasm_shared_memory_destroy. Is it OK for you?

Yes, it's OK. Fix wasm_shared_memory_destroy is urgent.

This PR is complex and needs more review, and I also need to think about how to write back content to u_app_argv.

LeoneChen · 2023-08-01T13:09:11Z

Fix it by yourself or I create a new PR?

wenyongh · 2023-08-01T13:19:00Z

Fix it by yourself or I create a new PR?

Let me fix it by myself😊

LeoneChen · 2023-08-01T13:37:42Z

OK

LeoneChen · 2023-08-01T13:50:17Z

Hi, I checked again, wasm_application_execute_main puts the return value into *(int *)argv, here should be *(int )(app_argv + 1), see: https://github.com/bytecodealliance/wasm-micro-runtime/blob/main/core/iwasm/include/wasm_export.h#L836-L838
So my understanding is that we had better write(int *)(app_argv + 1) back into *(int *)u_app_argv?

wasm_application_execute_main call execute_main, which write back int type return value that use char** type argv as int *.

I think it's not a good code practice...

And in current situation, app_argv + 1 is malloc-ed in Enclave with memory clone (for security), when execute_main store int type return value to app_argv + 1, it will cause address of malloc-ed char string be overwrite, and then we can't free this deep copied char string.

Maybe can use stack memory

wenyongh · 2023-08-02T00:59:45Z

Hi, I checked again, wasm_application_execute_main puts the return value into *(int *)argv, here should be *(int )(app_argv + 1), see: https://github.com/bytecodealliance/wasm-micro-runtime/blob/main/core/iwasm/include/wasm_export.h#L836-L838
So my understanding is that we had better write(int *)(app_argv + 1) back into *(int *)u_app_argv?

wasm_application_execute_main call execute_main, which write back int type return value that use char** type argv as int *.

I think it's not a good code practice...

And in current situation, app_argv + 1 is malloc-ed in Enclave with memory clone (for security), when execute_main store int type return value to app_argv + 1, it will cause address of malloc-ed char string be overwrite, and then we can't free this deep copied char string.

Maybe can use stack memory

Yes, how about adding an extra argument in ecall_handle_cmd_exec_app_main to output the result? e.g.

public bool ecall_handle_cmd_exec_app_main(
        uint16_t wasm_module_inst_idx,
        [in, count=app_argc] char **u_app_argv,
        uint32_t app_argc,
        [out, count=1] int *ret_main
      );

wenyongh · 2023-08-02T04:09:05Z

Fix it by yourself or I create a new PR?

@LeoneChen I found more similar issues in Enclave, and uploaded PR #2416 to fix them, could you help review it? Thanks.

LeoneChen · 2023-08-02T10:43:40Z

Fix it by yourself or I create a new PR?

@LeoneChen I found more similar issues in Enclave, and uploaded PR #2416 to fix them, could you help review it? Thanks.

OK

LeoneChen mentioned this pull request Jul 13, 2023

BUGs in WAMR #2252

Open

TianlongLiang reviewed Jul 24, 2023

View reviewed changes

product-mini/platforms/linux-sgx/enclave-sample/Enclave/Enclave.edl Show resolved Hide resolved

wenyongh reviewed Aug 1, 2023

View reviewed changes

LeoneChen added 3 commits August 1, 2023 17:37

1. Fix Enclave Interfaces of WAMR SGX version

b6a3957

2. Set pointer to NULL after free, otherwise cause UAF/DF

Argument count for handle_cmd_exec_app_func can be 0, fix it

f5b9c13

Fix partial BUGs

dd0e776

LeoneChen force-pushed the main branch from ecdbaee to dd0e776 Compare August 1, 2023 09:38

Fix

a7778af

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Enclave Interfaces of WAMR SGX version; Set pointer to NULL after free, otherwise cause UAF/DF #2357

Fix Enclave Interfaces of WAMR SGX version; Set pointer to NULL after free, otherwise cause UAF/DF #2357

LeoneChen commented Jul 13, 2023

TianlongLiang commented Jul 24, 2023

LeoneChen commented Jul 24, 2023

TianlongLiang commented Jul 24, 2023

LeoneChen commented Jul 24, 2023

LeoneChen commented Jul 24, 2023

LeoneChen commented Jul 24, 2023

TianlongLiang commented Jul 24, 2023

TianlongLiang commented Jul 24, 2023

LeoneChen commented Jul 24, 2023

LeoneChen commented Jul 24, 2023

TianlongLiang commented Jul 24, 2023

LeoneChen commented Jul 24, 2023

LeoneChen commented Jul 28, 2023

TianlongLiang commented Jul 31, 2023

wenyongh Aug 1, 2023

LeoneChen Aug 1, 2023

LeoneChen Aug 1, 2023

LeoneChen Aug 1, 2023 •

edited

Loading

wenyongh Aug 1, 2023

LeoneChen Aug 1, 2023

LeoneChen commented Aug 1, 2023

wenyongh commented Aug 1, 2023

LeoneChen commented Aug 1, 2023

wenyongh commented Aug 1, 2023 •

edited

Loading

LeoneChen commented Aug 1, 2023

LeoneChen commented Aug 1, 2023

wenyongh commented Aug 1, 2023

LeoneChen commented Aug 1, 2023

LeoneChen commented Aug 1, 2023

wenyongh commented Aug 2, 2023

wenyongh commented Aug 2, 2023

LeoneChen commented Aug 2, 2023

Fix Enclave Interfaces of WAMR SGX version; Set pointer to NULL after free, otherwise cause UAF/DF #2357

Are you sure you want to change the base?

Fix Enclave Interfaces of WAMR SGX version; Set pointer to NULL after free, otherwise cause UAF/DF #2357

Conversation

LeoneChen commented Jul 13, 2023

TianlongLiang commented Jul 24, 2023

LeoneChen commented Jul 24, 2023

TianlongLiang commented Jul 24, 2023

LeoneChen commented Jul 24, 2023

LeoneChen commented Jul 24, 2023

LeoneChen commented Jul 24, 2023

TianlongLiang commented Jul 24, 2023

TianlongLiang commented Jul 24, 2023

LeoneChen commented Jul 24, 2023

LeoneChen commented Jul 24, 2023

TianlongLiang commented Jul 24, 2023

LeoneChen commented Jul 24, 2023

LeoneChen commented Jul 28, 2023

TianlongLiang commented Jul 31, 2023

wenyongh Aug 1, 2023

Choose a reason for hiding this comment

LeoneChen Aug 1, 2023

Choose a reason for hiding this comment

LeoneChen Aug 1, 2023

Choose a reason for hiding this comment

LeoneChen Aug 1, 2023 • edited Loading

Choose a reason for hiding this comment

wenyongh Aug 1, 2023

Choose a reason for hiding this comment

LeoneChen Aug 1, 2023

Choose a reason for hiding this comment

LeoneChen commented Aug 1, 2023

wenyongh commented Aug 1, 2023

LeoneChen commented Aug 1, 2023

wenyongh commented Aug 1, 2023 • edited Loading

LeoneChen commented Aug 1, 2023

LeoneChen commented Aug 1, 2023

wenyongh commented Aug 1, 2023

LeoneChen commented Aug 1, 2023

LeoneChen commented Aug 1, 2023

wenyongh commented Aug 2, 2023

wenyongh commented Aug 2, 2023

LeoneChen commented Aug 2, 2023

LeoneChen Aug 1, 2023 •

edited

Loading

wenyongh commented Aug 1, 2023 •

edited

Loading