Changes in stubs and linker.py for Authentic Execution #36

gianlu33 · 2021-08-23T15:16:00Z

fix a bug in linker.py to preserve input callback symbols
support many-to-many relationships
linker.py: add "--num-connections" argument to set the maximum
connections of a module (impacts on the allocated memory)
fix nonce use, prevent replay attacks
add attest() entry point
set_key: remove attestation
handle_input: fix bug when input data size is zero (use sancus_tag
instead of sancus_unwrap)

jovanbulck · 2021-09-24T10:21:09Z

Hi Gianluca, thanks for the PR! 👍

I started a partial code review w some concerns above, let me know if you'd have any questions!

I especially think the pointer sanitation issue would need to be fixed when using the toolchain in bigger projects.

I'll try to continue the review later (didn't yet review the multiple connection code). Feel free to already start looking into the issues above meanwhile :)

jovanbulck · 2021-09-24T10:22:52Z

Also, when merging this code, I think it'd make a lot of sense to have a minimalist (non-contiki) program with SM_INPUT/OUTPUT as part of the sancus-examples continuous integration to make sure everything keeps working?

Do you think such a mini example would be feasible?

https://github.com/sancus-tee/sancus-examples

fritzalder · 2021-09-24T10:27:27Z

fyi: I already had a chat with Gianluca on this and we will review the PR together once we merged the changes for Aion. In #33 , I added a configuration manager that allows to add a .yml file for enclaves and provide some configurations. That would be ideal for the new connections feature that this PR adds, so I suggest to address this PR again once #33 is complete.

jovanbulck · 2021-09-24T10:31:15Z

thanks, makes sense! (although that would only simplify the linker option, so the other issues above will need to be fixed anyway)

jovanbulck

see comments (forgot they're not visible until review is finished)

As @fritzalder said, let's merge this after Aion. I think the most important points from my review are:

security: check untrusted pointers!
security: use constant-time MAC comparison!
performance: I'm not sure why you need to do the O(n) search if you return a "capability" index on set_key and ask that to be passed on subsequent sm_input invocations by untrusted code?

src/stubs/reactive_stubs_support.h

src/stubs/sm_attest.c

src/stubs/sm_input.c

src/stubs/sm_set_key.c

src/stubs/sm_input.c

gianlu33 · 2021-09-27T08:08:46Z

Thanks Jo for the review. It is always good to learn something new :)

I will make the requested changes soon. I'll let you know if something is unclear!

jovanbulck · 2021-09-29T10:43:47Z

Also following up here @gianlu33 regarding your comment:

handle_input: fix bug when input data size is zero (use sancus_tag
instead of sancus_unwrap)

Linking this to sancus-tee/sancus-core#11 It seems this is the issue you ran into? Feel free to follow-up there if there's a bug in the core that needs to be addressed or better documented?

I tried to improve the documentation in 16cc2ba a while ago, not sure if this is sufficiently clear or not? If not, feel free to open a PR with more accurate documentation to avoid this issue in the future!

src/sancus_support/sm_support.h

gianlu33 · 2021-10-01T10:30:40Z

Also, when merging this code, I think it'd make a lot of sense to have a minimalist (non-contiki) program with SM_INPUT/OUTPUT as part of the sancus-examples continuous integration to make sure everything keeps working?

Do you think such a mini example would be feasible?

https://github.com/sancus-tee/sancus-examples

yes it definitely makes sense, I'll keep this in mind!

src/stubs/reactive_stubs_support.h

gianlu33 · 2021-10-06T09:34:26Z

Also following up here @gianlu33 regarding your comment:

handle_input: fix bug when input data size is zero (use sancus_tag
instead of sancus_unwrap)

Linking this to sancus-tee/sancus-core#11 It seems this is the issue you ran into? Feel free to follow-up there if there's a bug in the core that needs to be addressed or better documented?

I tried to improve the documentation in 16cc2ba a while ago, not sure if this is sufficiently clear or not? If not, feel free to open a PR with more accurate documentation to avoid this issue in the future!

@jovanbulck I opened an issue on sancus-core about this (sancus-tee/sancus-core#26). The problem is not about NULL pointers, but rather that sancus_unwrap always fails if the length of the ciphertext is zero.

I think, if this bug can be fixed in the core, there is no need for the MAC comparison here. I could just always use sancus_unwrap (as it was before) hence removing the insecure code.

About the documentation, looks clear to me!

jovanbulck · 2021-10-07T11:01:59Z

I think, if this bug can be fixed in the core, there is no need for the MAC comparison here. I could just always use sancus_unwrap (as it was before) hence removing the insecure code.

No, this is also tricky with the current core as tag comparison is unfortunately not constant-time in software, I opened a separate issue for that: see sancus-tee/sancus-core#27

jovanbulck · 2021-10-07T11:04:13Z

While sancus-tee/sancus-core#27 can be fixed with a small change, not sure I'll want to fix sancus-tee/sancus-core#26 any time soon.

So I suggest to implement the MAC comparison in constant-time software at least for now, eg using the VulCAN union approach

gianlu33 · 2021-10-08T09:20:38Z

Alright, but I would suggest to open another PR for this.

We could implement sancus_untag and sancus_untag_with_key functions in sm_support.h that would do the job, and maybe also add a check on cipher_len in sancus_unwrap_with_key (if the length is zero, then call the untag function). This way we would not require changes in existing code to fix the bug.

About the union approach: I think I got it, but this would depend on the size of the tag, right? So, if we have 64 bits of security we would need to use an uint64_t type, but for 128 bits of security we don't have a corresponding uint128_t type.. therefore, we would need to do something like in #31 anyway. Am I right or maybe I am missing something?

jovanbulck · 2021-10-08T11:24:10Z

HI @gianlu33,

Yes you're absolutely right on both points!

Hiding the tag comparison in software using a sancus_untag function that can also be called from sancus_unwrap_with_key is indeed the cleanest. And this would allow us to still use the hardware primitives if they would get fixed later.

The union approach is indeed not the cleanest as it'd hard-code the key length (in VulCAN we always used 128-bit so it was no problem there). I agree implementing a small constant-time comparision function is probably best. You might want to base it on the one from NaCL I referenced in #31

Feel free to open a separate PR for this if you'd like (but then it'd need to be merged before this one I guess)

gianlu33 · 2021-10-08T12:26:10Z

Ok, cool!

I don't think that we'd need to merge the two PRs in order, since I changed the code of __sm_handle_input in 47d7e24. Now this function does not do the MAC comparison anymore, it only calls sancus_unwrap_with_key.

So, if there isn't anything else to review, I think that this PR is almost ready to merge. Last thing I need to do is to implement the small change in linker.py as soon as #33 is merged, as @fritzalder suggested earlier.

jovanbulck · 2021-10-08T12:41:26Z

looks good! 👍

Let's indeed merge this after #33

@gianlu33 thanks for all the changes! Feel free to open a new PR later w the needed changes to sm_support.h to handle the cipher_len=0 as you described above, if needed

fritzalder · 2021-10-15T08:40:41Z

Just looked at the PR again and code-wise it looks good (also the new config addition). Let me know if you need another discussion on the linker or so.

Just one request: Can we add some comments and maybe some documentation in a Readme or so on what these new features are doing? I honestly don't really know what the stuff you added can do or what it is for, so it will be difficult to understand this next time we change the linker and/or would need a similar functionality

* moved the check on `io_id` to `__sm_set_key` * add check on `len`: it has to be >= `SANCUS_TAG_SIZE` * remove the condition on `data_len`: the workaround to handle length==0 should not be implemented here, but rather in the core or in sm_support.h

in set_key, io_id might as well be an _output_ identifier, so it was wrong to add that check here.

src/stubs/sm_input.c

src/stubs/sm_attest.c

gianlu33 · 2021-10-21T09:16:15Z

src/stubs/sm_output.c

@@ -16,7 +16,7 @@ SM_FUNC(SM_NAME) void __sm_send_output(io_index index,
        continue;

      uint8_t* payload = malloc(payload_len);
-      if (payload == NULL)
+      if (payload == NULL  || !sancus_is_outside_sm(SM_NAME, (void *) payload, payload_len))


It would be wrong to free the memory if the buffer is inside the SM, right?

Also, should we just raise an error if this happens (how?) instead of continuing?

yes shouldn't free (although free is outside the enclave anyway).

the current continue seems fine, but indeed since it's really wrong, I'd simply return here. And maybe add a return value instead of void so you can return an error code as with the other functions?

jovanbulck · 2021-10-21T12:04:07Z

I think this is almost ready for merging!

@gianlu33 maybe add a return value to __sm_send_output if that makes sense? When that's done I think this is ready for merging, unless you want to make more changes to the code/functionality?

gianlu33 · 2021-10-21T13:12:53Z

I am quite happy with the code now, no more changes from my side!

jovanbulck · 2021-10-22T10:27:29Z

src/sancus_support/sm_support.h

+ * Returns true if buf is outside the memory region [start, end)
+ * if start >= end, immediately return false
+ */
+always_inline int is_buffer_outside_region(void *start, void *end,


we still have an issue here with overflow(!)

pointer arithmetic for void* pointers is illegal in C (while silently allowed in gcc)

overflow is only guaranteed to wrap around for unsigned data types in the C standard (even gcc might simply compile/optimize away the check we added as it's undefined behavior)

what we should probably do here is to cast all the void* inputs to uintptr_t first

thanks @mtvec !

ok done, by changing the signature of the function the cast should be implicit right?

thanks! I think it is fine now, but we might get compiler warnings if every argument is not explicitly casted in the function call. Not sure how that is for the sancus_is_outside_sm macro.

Maybe cleaner to keep the void* pointers in the function arguments and cast them as local variables instead if that's possible easily?

sorry for the overhead!

I had the same thought but actually I did not get any warnings when compiling the library

not sure, will existing applications not get a warning? eg

https://github.com/sancus-tee/sancus-examples/blob/master/sensor-reader/reader.c#L15

yes, you are totally right, sorry! I did not make clean before rebuilding the library, so this is why I was not getting any warnings.

I fixed it, now everything should be fine!

jovanbulck · 2021-10-26T13:55:25Z

Merged, thanks for all the changes @gianlu33 !

jovanbulck · 2021-10-26T13:58:12Z

For future reference, zhen you'd find a moment, would be nice to add a small sancus-examples program some time, cf:

sancus-tee/sancus-examples#20

Changes in stubs and linker.py for Authentic Execution

jovanbulck self-requested a review September 24, 2021 09:56

jovanbulck requested changes Sep 24, 2021

View reviewed changes

jovanbulck reviewed Sep 30, 2021

View reviewed changes

src/sancus_support/sm_support.h Outdated Show resolved Hide resolved

jovanbulck reviewed Sep 30, 2021

View reviewed changes

src/sancus_support/sm_support.h Show resolved Hide resolved

gianlu33 commented Oct 1, 2021

View reviewed changes

src/sancus_support/sm_support.h Outdated Show resolved Hide resolved

jovanbulck reviewed Oct 1, 2021

View reviewed changes

src/sancus_support/sm_support.h Outdated Show resolved Hide resolved

jovanbulck mentioned this pull request Oct 1, 2021

Minimal reactive programming example sancus-tee/sancus-examples#20

Open

jovanbulck reviewed Oct 1, 2021

View reviewed changes

src/stubs/reactive_stubs_support.h Outdated Show resolved Hide resolved

src/stubs/reactive_stubs_support.h Outdated Show resolved Hide resolved

gianlu33 commented Oct 1, 2021

View reviewed changes

src/stubs/reactive_stubs_support.h Outdated Show resolved Hide resolved

fritzalder mentioned this pull request Oct 6, 2021

Compatibility with the new SSA convention #33

Merged

jovanbulck approved these changes Oct 8, 2021

View reviewed changes

jovanbulck mentioned this pull request Oct 8, 2021

Check for integer overflow in sancus_is_outside_sm macro #30

Closed

gianlu33 mentioned this pull request Oct 8, 2021

Add sancus_untag functions and fix sancus_unwrap_with_key #37

Merged

gianlu33 force-pushed the rearrange-commits branch from 68cdf39 to 23124d5 Compare October 15, 2021 08:18

fritzalder approved these changes Oct 15, 2021

View reviewed changes

gianlu33 added 9 commits October 20, 2021 14:03

__sm_handle_input: return a value to indicate success/error

09f2bfd

__sm_attest: return a value to indicate success/error

6152f0e

simplify __sm_handle_input

6f8cb87

* moved the check on `io_id` to `__sm_set_key` * add check on `len`: it has to be >= `SANCUS_TAG_SIZE` * remove the condition on `data_len`: the workaround to handle length==0 should not be implemented here, but rather in the core or in sm_support.h

small fix return value in __sm_handle_input

e8f5890

use config file for num_connections parameter

3ae4774

add num_connections property in SmConfig

ce71396

add comments in linker.py to explain the connection array

5621e27

add num_connections parameter in sm-config-example.yaml

16ae05c

move io_id check from set_key to handle_input

b50bb02

in set_key, io_id might as well be an _output_ identifier, so it was wrong to add that check here.

gianlu33 force-pushed the rearrange-commits branch from f3b071c to b50bb02 Compare October 20, 2021 12:04

gianlu33 commented Oct 20, 2021

View reviewed changes

src/stubs/sm_input.c Outdated Show resolved Hide resolved

fix sm_{}_io_connections buffer allocation if num_connections is zero

0a81ca0

gianlu33 commented Oct 20, 2021

View reviewed changes

src/stubs/sm_attest.c Outdated Show resolved Hide resolved

gianlu33 added 3 commits October 21, 2021 10:21

return values of stubs as ResultCode values

7ee6635

handle_output: check if allocated buffer is outside SM

e5cb9ce

remove unused include

e760cea

gianlu33 commented Oct 21, 2021

View reviewed changes

return a value from output functions

537c3a0

jovanbulck reviewed Oct 22, 2021

View reviewed changes

gianlu33 added 2 commits October 25, 2021 14:46

use uintptr_t type instead of void* in is_buffer_outside_region

b27b130

fix: cast from void* to uintptr_t inside is_buffer_outside_region

d57b76d

jovanbulck approved these changes Oct 26, 2021

View reviewed changes

jovanbulck merged commit bb196fb into sancus-tee:master Oct 26, 2021

gianlu33 deleted the rearrange-commits branch October 26, 2021 13:57

gianlu33 added a commit to AuthenticExecution/sancus-compiler that referenced this pull request Nov 5, 2021

Merge pull request sancus-tee#36 from gianlu33/rearrange-commits

7982c96

Changes in stubs and linker.py for Authentic Execution

gianlu33 added a commit to AuthenticExecution/sancus-compiler that referenced this pull request Nov 9, 2021

Merge pull request sancus-tee#36 from gianlu33/rearrange-commits

e132b43

Changes in stubs and linker.py for Authentic Execution

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changes in stubs and linker.py for Authentic Execution #36

Changes in stubs and linker.py for Authentic Execution #36

gianlu33 commented Aug 23, 2021

jovanbulck commented Sep 24, 2021

jovanbulck commented Sep 24, 2021

fritzalder commented Sep 24, 2021

jovanbulck commented Sep 24, 2021

jovanbulck left a comment

gianlu33 commented Sep 27, 2021

jovanbulck commented Sep 29, 2021

gianlu33 commented Oct 1, 2021

gianlu33 commented Oct 6, 2021 •

edited

Loading

jovanbulck commented Oct 7, 2021

jovanbulck commented Oct 7, 2021

gianlu33 commented Oct 8, 2021

jovanbulck commented Oct 8, 2021 •

edited

Loading

gianlu33 commented Oct 8, 2021

jovanbulck commented Oct 8, 2021

fritzalder commented Oct 15, 2021

gianlu33 Oct 21, 2021 •

edited

Loading

jovanbulck Oct 21, 2021

jovanbulck commented Oct 21, 2021

gianlu33 commented Oct 21, 2021

jovanbulck Oct 22, 2021

jovanbulck Oct 22, 2021

gianlu33 Oct 25, 2021

jovanbulck Oct 26, 2021

gianlu33 Oct 26, 2021 •

edited

Loading

jovanbulck Oct 26, 2021

gianlu33 Oct 26, 2021

jovanbulck commented Oct 26, 2021

jovanbulck commented Oct 26, 2021

Changes in stubs and linker.py for Authentic Execution #36

Changes in stubs and linker.py for Authentic Execution #36

Conversation

gianlu33 commented Aug 23, 2021

jovanbulck commented Sep 24, 2021

jovanbulck commented Sep 24, 2021

fritzalder commented Sep 24, 2021

jovanbulck commented Sep 24, 2021

jovanbulck left a comment

Choose a reason for hiding this comment

gianlu33 commented Sep 27, 2021

jovanbulck commented Sep 29, 2021

gianlu33 commented Oct 1, 2021

gianlu33 commented Oct 6, 2021 • edited Loading

jovanbulck commented Oct 7, 2021

jovanbulck commented Oct 7, 2021

gianlu33 commented Oct 8, 2021

jovanbulck commented Oct 8, 2021 • edited Loading

gianlu33 commented Oct 8, 2021

jovanbulck commented Oct 8, 2021

fritzalder commented Oct 15, 2021

gianlu33 Oct 21, 2021 • edited Loading

Choose a reason for hiding this comment

jovanbulck Oct 21, 2021

Choose a reason for hiding this comment

jovanbulck commented Oct 21, 2021

gianlu33 commented Oct 21, 2021

jovanbulck Oct 22, 2021

Choose a reason for hiding this comment

jovanbulck Oct 22, 2021

Choose a reason for hiding this comment

gianlu33 Oct 25, 2021

Choose a reason for hiding this comment

jovanbulck Oct 26, 2021

Choose a reason for hiding this comment

gianlu33 Oct 26, 2021 • edited Loading

Choose a reason for hiding this comment

jovanbulck Oct 26, 2021

Choose a reason for hiding this comment

gianlu33 Oct 26, 2021

Choose a reason for hiding this comment

jovanbulck commented Oct 26, 2021

jovanbulck commented Oct 26, 2021

gianlu33 commented Oct 6, 2021 •

edited

Loading

jovanbulck commented Oct 8, 2021 •

edited

Loading

gianlu33 Oct 21, 2021 •

edited

Loading

gianlu33 Oct 26, 2021 •

edited

Loading