| Message ID | 20230719142500.13623-4-jo.vanbulck@cs.kuleuven.be (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | selftests/sgx: Harden test enclave | expand |
On Wed Jul 19, 2023 at 5:24 PM EEST, Jo Van Bulck wrote: > Adhere to enclave programming best practices and prevent confused-deputy > attacks on the test enclave by validating that untrusted pointer arguments > do not fall inside the protected enclave range. > > Note that the test enclave deliberately allows arbitrary reads/writes in > enclave memory through the get_from_addr/put_to_addr operations for > explicit testing purposes. Hence, only allow remaining unchecked pointer > dereferences in these functions. > > Signed-off-by: Jo Van Bulck <jo.vanbulck@cs.kuleuven.be> > --- > tools/testing/selftests/sgx/main.c | 5 + > tools/testing/selftests/sgx/test_encl.c | 161 ++++++++++++++++++------ > 2 files changed, 128 insertions(+), 38 deletions(-) > > diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c > index bad963c79..8d60f8dcd 100644 > --- a/tools/testing/selftests/sgx/main.c > +++ b/tools/testing/selftests/sgx/main.c > @@ -355,6 +355,11 @@ TEST_F(enclave, poison_args) > : "=m"(flags) : : ); > EXPECT_EEXIT(&self->run); > EXPECT_EQ(flags & 0x40400, 0); > + > + /* attempt API pointer poisoning */ > + EXPECT_EQ(ENCL_CALL(self->encl.encl_base + self->encl.encl_size - 1, &self->run, false), 0); > + EXPECT_EQ((&self->run)->function, ERESUME); > + EXPECT_EQ((&self->run)->exception_vector, 6 /* expect ud2 */); > } > > /* > diff --git a/tools/testing/selftests/sgx/test_encl.c b/tools/testing/selftests/sgx/test_encl.c > index c0d639729..5531f5d48 100644 > --- a/tools/testing/selftests/sgx/test_encl.c > +++ b/tools/testing/selftests/sgx/test_encl.c > @@ -16,37 +16,55 @@ enum sgx_enclu_function { > EMODPE = 0x6, > }; > > -static void do_encl_emodpe(void *_op) > +uint64_t get_enclave_base(void); > +uint64_t get_enclave_size(void); > + > +static int is_outside_enclave(void *addr, size_t len) > { > - struct sgx_secinfo secinfo __aligned(sizeof(struct sgx_secinfo)) = {0}; > - struct encl_op_emodpe *op = _op; > + /* need cast since void pointer arithmetics are undefined in C */ > + size_t start = (size_t) addr; > + size_t end = start + len - 1; > + size_t enclave_end = get_enclave_base() + get_enclave_size(); > > - secinfo.flags = op->flags; > + /* check for integer overflow with untrusted length */ > + if (start > end) > + return 0; > > - asm volatile(".byte 0x0f, 0x01, 0xd7" > - : > - : "a" (EMODPE), > - "b" (&secinfo), > - "c" (op->epc_addr)); > + return (start > enclave_end || end < get_enclave_base()); > } > > -static void do_encl_eaccept(void *_op) > +static int is_inside_enclave(void *addr, size_t len) > { > - struct sgx_secinfo secinfo __aligned(sizeof(struct sgx_secinfo)) = {0}; > - struct encl_op_eaccept *op = _op; > - int rax; > + /* need cast since void pointer arithmetics are undefined in C */ > + size_t start = (size_t) addr; > + size_t end = start + len - 1; > + size_t enclave_end = get_enclave_base() + get_enclave_size(); > > - secinfo.flags = op->flags; > + /* check for integer overflow with untrusted length */ > + if (start > end) > + return 0; > > - asm volatile(".byte 0x0f, 0x01, 0xd7" > - : "=a" (rax) > - : "a" (EACCEPT), > - "b" (&secinfo), > - "c" (op->epc_addr)); > - > - op->ret = rax; > + return (start >= get_enclave_base() && end <= enclave_end); > } > > +#define PANIC() \ > + asm("ud2\n\t") > + > +#define SAFE_COPY_STRUCT(u_arg, t_cp) \ any reason not to use static inline function? > + do { \ > + /* 1. check if the argument lies entirely outside */ \ > + if (!is_outside_enclave((void *)u_arg, sizeof(*t_cp))) \ > + PANIC(); \ > + /* 2. copy the argument inside to prevent TOCTOU */ \ > + memcpy(t_cp, u_arg, sizeof(*t_cp)); \ > + } while (0) > + > +#define ASSERT_INSIDE_ENCLAVE(u_arg, size) \ " > + do { \ > + if (!is_inside_enclave(((void *)(u_arg)), size)) \ > + PANIC(); \ > + } while (0) > + > static void *memcpy(void *dest, const void *src, size_t n) > { > size_t i; > @@ -67,18 +85,62 @@ static void *memset(void *dest, int c, size_t n) > return dest; > } > > +static void do_encl_emodpe(void *_op) > +{ > + struct encl_op_emodpe op; > + struct sgx_secinfo secinfo __aligned(sizeof(struct sgx_secinfo)) = {0}; > + > + SAFE_COPY_STRUCT(_op, &op); > + ASSERT_INSIDE_ENCLAVE(op.epc_addr, PAGE_SIZE); > + > + secinfo.flags = op.flags; > + > + asm volatile(".byte 0x0f, 0x01, 0xd7" > + : > + : "a" (EMODPE), > + "b" (&secinfo), > + "c" (op.epc_addr)); > +} > + > +static void do_encl_eaccept(void *_op) > +{ > + struct encl_op_eaccept op; > + struct sgx_secinfo secinfo __aligned(sizeof(struct sgx_secinfo)) = {0}; > + int rax; > + > + SAFE_COPY_STRUCT(_op, &op); > + ASSERT_INSIDE_ENCLAVE(op.epc_addr, PAGE_SIZE); > + > + secinfo.flags = op.flags; > + > + asm volatile(".byte 0x0f, 0x01, 0xd7" > + : "=a" (rax) > + : "a" (EACCEPT), > + "b" (&secinfo), > + "c" (op.epc_addr)); > + > + op.ret = rax; > + memcpy(_op, &op, sizeof(op)); > +} > + > static void do_encl_init_tcs_page(void *_op) > { > - struct encl_op_init_tcs_page *op = _op; > - void *tcs = (void *)op->tcs_page; > + struct encl_op_init_tcs_page op; > + void *tcs; > uint32_t val_32; > > + SAFE_COPY_STRUCT(_op, &op); > + tcs = (void *)op.tcs_page; > + ASSERT_INSIDE_ENCLAVE(tcs, PAGE_SIZE); > + ASSERT_INSIDE_ENCLAVE(get_enclave_base() + op.ssa, PAGE_SIZE); > + ASSERT_INSIDE_ENCLAVE(get_enclave_base() + op.entry, 1); > + > memset(tcs, 0, 16); /* STATE and FLAGS */ > - memcpy(tcs + 16, &op->ssa, 8); /* OSSA */ > + memcpy(tcs + 16, &op.ssa, 8); /* OSSA */ > memset(tcs + 24, 0, 4); /* CSSA */ > val_32 = 1; > memcpy(tcs + 28, &val_32, 4); /* NSSA */ > - memcpy(tcs + 32, &op->entry, 8); /* OENTRY */ > + memcpy(tcs + 32, &op.entry, 8); /* OENTRY */ > memset(tcs + 40, 0, 24); /* AEP, OFSBASE, OGSBASE */ > val_32 = 0xFFFFFFFF; > memcpy(tcs + 64, &val_32, 4); /* FSLIMIT */ > @@ -86,32 +148,54 @@ static void do_encl_init_tcs_page(void *_op) > memset(tcs + 72, 0, 4024); /* Reserved */ > } > > -static void do_encl_op_put_to_buf(void *op) > +static void do_encl_op_put_to_buf(void *_op) > { > - struct encl_op_put_to_buf *op2 = op; > + struct encl_op_get_from_buf op; > + > + SAFE_COPY_STRUCT(_op, &op); > > - memcpy(&encl_buffer[0], &op2->value, 8); > + memcpy(&encl_buffer[0], &op.value, 8); > + memcpy(_op, &op, sizeof(op)); > } > > -static void do_encl_op_get_from_buf(void *op) > +static void do_encl_op_get_from_buf(void *_op) > { > - struct encl_op_get_from_buf *op2 = op; > + struct encl_op_get_from_buf op; > > - memcpy(&op2->value, &encl_buffer[0], 8); > + SAFE_COPY_STRUCT(_op, &op); > + > + memcpy(&op.value, &encl_buffer[0], 8); > + memcpy(_op, &op, sizeof(op)); > } > > static void do_encl_op_put_to_addr(void *_op) > { > - struct encl_op_put_to_addr *op = _op; > + struct encl_op_put_to_addr op; > + > + SAFE_COPY_STRUCT(_op, &op); > > - memcpy((void *)op->addr, &op->value, 8); > + /* > + * NOTE: not checking is_outside_enclave(op.addr, 8) here > + * deliberately allows arbitrary writes to enclave memory for > + * testing purposes. > + */ > + memcpy((void *)op.addr, &op.value, 8); > + memcpy(_op, &op, sizeof(op)); > } > > static void do_encl_op_get_from_addr(void *_op) > { > - struct encl_op_get_from_addr *op = _op; > + struct encl_op_get_from_addr op; > + > + SAFE_COPY_STRUCT(_op, &op); > > - memcpy(&op->value, (void *)op->addr, 8); > + /* > + * NOTE: not checking is_outside_enclave(op.addr, 8) here > + * deliberately allows arbitrary reads from enclave memory for > + * testing purposes. > + */ > + memcpy(&op.value, (void *)op.addr, 8); > + memcpy(_op, &op, sizeof(op)); > } > > static void do_encl_op_nop(void *_op) > @@ -131,9 +215,10 @@ void encl_body(void *rdi, void *rsi) > do_encl_emodpe, > do_encl_init_tcs_page, > }; > + struct encl_op_header op; > > - struct encl_op_header *op = (struct encl_op_header *)rdi; > + SAFE_COPY_STRUCT(rdi, &op); > > - if (op->type < ENCL_OP_MAX) > - (*encl_op_array[op->type])(op); > + if (op.type < ENCL_OP_MAX) > + (*encl_op_array[op.type])(rdi); > } > -- > 2.34.1 BR, Jarkko
On 20.07.23 19:32, Jarkko Sakkinen wrote: >> +#define PANIC() \ >> + asm("ud2\n\t") > > any reason not to use static inline function? > Thanks for the suggestion, no reason in this case. Apart perhaps that it's only 1 line of code and an inline function may seem a bit like a waste (given that gcc does not inline until certain optimization levels). I can surely change it to static inline void panic(void) if you prefer? >> +#define SAFE_COPY_STRUCT(u_arg, t_cp) \ >> + do { \ >> + /* 1. check if the argument lies entirely outside */ \ >> + if (!is_outside_enclave((void *)u_arg, sizeof(*t_cp))) \ >> + PANIC(); \ >> + /* 2. copy the argument inside to prevent TOCTOU */ \ >> + memcpy(t_cp, u_arg, sizeof(*t_cp)); \ >> + } while (0) >> + This could be made into a static inline function, but then t_cp would have to be type void* and sizeof(*t_cp) won't work anymore and a third parameter to pass the sizeof would be needed, which would require the caller to pass it correctly. Hence, a macro seems "safer" to me here in this instance as it requires only 2 arguments. Agreed? >> +#define ASSERT_INSIDE_ENCLAVE(u_arg, size) \ >> + do { \ >> + if (!is_inside_enclave(((void *)(u_arg)), size)) \ >> + PANIC(); \ >> + } while (0) This macro could certainly be turned into a static inline void function if preferred. Best, Jo
diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c index bad963c79..8d60f8dcd 100644 --- a/tools/testing/selftests/sgx/main.c +++ b/tools/testing/selftests/sgx/main.c @@ -355,6 +355,11 @@ TEST_F(enclave, poison_args) : "=m"(flags) : : ); EXPECT_EEXIT(&self->run); EXPECT_EQ(flags & 0x40400, 0); + + /* attempt API pointer poisoning */ + EXPECT_EQ(ENCL_CALL(self->encl.encl_base + self->encl.encl_size - 1, &self->run, false), 0); + EXPECT_EQ((&self->run)->function, ERESUME); + EXPECT_EQ((&self->run)->exception_vector, 6 /* expect ud2 */); } /* diff --git a/tools/testing/selftests/sgx/test_encl.c b/tools/testing/selftests/sgx/test_encl.c index c0d639729..5531f5d48 100644 --- a/tools/testing/selftests/sgx/test_encl.c +++ b/tools/testing/selftests/sgx/test_encl.c @@ -16,37 +16,55 @@ enum sgx_enclu_function { EMODPE = 0x6, }; -static void do_encl_emodpe(void *_op) +uint64_t get_enclave_base(void); +uint64_t get_enclave_size(void); + +static int is_outside_enclave(void *addr, size_t len) { - struct sgx_secinfo secinfo __aligned(sizeof(struct sgx_secinfo)) = {0}; - struct encl_op_emodpe *op = _op; + /* need cast since void pointer arithmetics are undefined in C */ + size_t start = (size_t) addr; + size_t end = start + len - 1; + size_t enclave_end = get_enclave_base() + get_enclave_size(); - secinfo.flags = op->flags; + /* check for integer overflow with untrusted length */ + if (start > end) + return 0; - asm volatile(".byte 0x0f, 0x01, 0xd7" - : - : "a" (EMODPE), - "b" (&secinfo), - "c" (op->epc_addr)); + return (start > enclave_end || end < get_enclave_base()); } -static void do_encl_eaccept(void *_op) +static int is_inside_enclave(void *addr, size_t len) { - struct sgx_secinfo secinfo __aligned(sizeof(struct sgx_secinfo)) = {0}; - struct encl_op_eaccept *op = _op; - int rax; + /* need cast since void pointer arithmetics are undefined in C */ + size_t start = (size_t) addr; + size_t end = start + len - 1; + size_t enclave_end = get_enclave_base() + get_enclave_size(); - secinfo.flags = op->flags; + /* check for integer overflow with untrusted length */ + if (start > end) + return 0; - asm volatile(".byte 0x0f, 0x01, 0xd7" - : "=a" (rax) - : "a" (EACCEPT), - "b" (&secinfo), - "c" (op->epc_addr)); - - op->ret = rax; + return (start >= get_enclave_base() && end <= enclave_end); } +#define PANIC() \ + asm("ud2\n\t") + +#define SAFE_COPY_STRUCT(u_arg, t_cp) \ + do { \ + /* 1. check if the argument lies entirely outside */ \ + if (!is_outside_enclave((void *)u_arg, sizeof(*t_cp))) \ + PANIC(); \ + /* 2. copy the argument inside to prevent TOCTOU */ \ + memcpy(t_cp, u_arg, sizeof(*t_cp)); \ + } while (0) + +#define ASSERT_INSIDE_ENCLAVE(u_arg, size) \ + do { \ + if (!is_inside_enclave(((void *)(u_arg)), size)) \ + PANIC(); \ + } while (0) + static void *memcpy(void *dest, const void *src, size_t n) { size_t i; @@ -67,18 +85,62 @@ static void *memset(void *dest, int c, size_t n) return dest; } +static void do_encl_emodpe(void *_op) +{ + struct encl_op_emodpe op; + struct sgx_secinfo secinfo __aligned(sizeof(struct sgx_secinfo)) = {0}; + + SAFE_COPY_STRUCT(_op, &op); + ASSERT_INSIDE_ENCLAVE(op.epc_addr, PAGE_SIZE); + + secinfo.flags = op.flags; + + asm volatile(".byte 0x0f, 0x01, 0xd7" + : + : "a" (EMODPE), + "b" (&secinfo), + "c" (op.epc_addr)); +} + +static void do_encl_eaccept(void *_op) +{ + struct encl_op_eaccept op; + struct sgx_secinfo secinfo __aligned(sizeof(struct sgx_secinfo)) = {0}; + int rax; + + SAFE_COPY_STRUCT(_op, &op); + ASSERT_INSIDE_ENCLAVE(op.epc_addr, PAGE_SIZE); + + secinfo.flags = op.flags; + + asm volatile(".byte 0x0f, 0x01, 0xd7" + : "=a" (rax) + : "a" (EACCEPT), + "b" (&secinfo), + "c" (op.epc_addr)); + + op.ret = rax; + memcpy(_op, &op, sizeof(op)); +} + static void do_encl_init_tcs_page(void *_op) { - struct encl_op_init_tcs_page *op = _op; - void *tcs = (void *)op->tcs_page; + struct encl_op_init_tcs_page op; + void *tcs; uint32_t val_32; + SAFE_COPY_STRUCT(_op, &op); + tcs = (void *)op.tcs_page; + ASSERT_INSIDE_ENCLAVE(tcs, PAGE_SIZE); + ASSERT_INSIDE_ENCLAVE(get_enclave_base() + op.ssa, PAGE_SIZE); + ASSERT_INSIDE_ENCLAVE(get_enclave_base() + op.entry, 1); + memset(tcs, 0, 16); /* STATE and FLAGS */ - memcpy(tcs + 16, &op->ssa, 8); /* OSSA */ + memcpy(tcs + 16, &op.ssa, 8); /* OSSA */ memset(tcs + 24, 0, 4); /* CSSA */ val_32 = 1; memcpy(tcs + 28, &val_32, 4); /* NSSA */ - memcpy(tcs + 32, &op->entry, 8); /* OENTRY */ + memcpy(tcs + 32, &op.entry, 8); /* OENTRY */ memset(tcs + 40, 0, 24); /* AEP, OFSBASE, OGSBASE */ val_32 = 0xFFFFFFFF; memcpy(tcs + 64, &val_32, 4); /* FSLIMIT */ @@ -86,32 +148,54 @@ static void do_encl_init_tcs_page(void *_op) memset(tcs + 72, 0, 4024); /* Reserved */ } -static void do_encl_op_put_to_buf(void *op) +static void do_encl_op_put_to_buf(void *_op) { - struct encl_op_put_to_buf *op2 = op; + struct encl_op_get_from_buf op; + + SAFE_COPY_STRUCT(_op, &op); - memcpy(&encl_buffer[0], &op2->value, 8); + memcpy(&encl_buffer[0], &op.value, 8); + memcpy(_op, &op, sizeof(op)); } -static void do_encl_op_get_from_buf(void *op) +static void do_encl_op_get_from_buf(void *_op) { - struct encl_op_get_from_buf *op2 = op; + struct encl_op_get_from_buf op; - memcpy(&op2->value, &encl_buffer[0], 8); + SAFE_COPY_STRUCT(_op, &op); + + memcpy(&op.value, &encl_buffer[0], 8); + memcpy(_op, &op, sizeof(op)); } static void do_encl_op_put_to_addr(void *_op) { - struct encl_op_put_to_addr *op = _op; + struct encl_op_put_to_addr op; + + SAFE_COPY_STRUCT(_op, &op); - memcpy((void *)op->addr, &op->value, 8); + /* + * NOTE: not checking is_outside_enclave(op.addr, 8) here + * deliberately allows arbitrary writes to enclave memory for + * testing purposes. + */ + memcpy((void *)op.addr, &op.value, 8); + memcpy(_op, &op, sizeof(op)); } static void do_encl_op_get_from_addr(void *_op) { - struct encl_op_get_from_addr *op = _op; + struct encl_op_get_from_addr op; + + SAFE_COPY_STRUCT(_op, &op); - memcpy(&op->value, (void *)op->addr, 8); + /* + * NOTE: not checking is_outside_enclave(op.addr, 8) here + * deliberately allows arbitrary reads from enclave memory for + * testing purposes. + */ + memcpy(&op.value, (void *)op.addr, 8); + memcpy(_op, &op, sizeof(op)); } static void do_encl_op_nop(void *_op) @@ -131,9 +215,10 @@ void encl_body(void *rdi, void *rsi) do_encl_emodpe, do_encl_init_tcs_page, }; + struct encl_op_header op; - struct encl_op_header *op = (struct encl_op_header *)rdi; + SAFE_COPY_STRUCT(rdi, &op); - if (op->type < ENCL_OP_MAX) - (*encl_op_array[op->type])(op); + if (op.type < ENCL_OP_MAX) + (*encl_op_array[op.type])(rdi); }
Adhere to enclave programming best practices and prevent confused-deputy attacks on the test enclave by validating that untrusted pointer arguments do not fall inside the protected enclave range. Note that the test enclave deliberately allows arbitrary reads/writes in enclave memory through the get_from_addr/put_to_addr operations for explicit testing purposes. Hence, only allow remaining unchecked pointer dereferences in these functions. Signed-off-by: Jo Van Bulck <jo.vanbulck@cs.kuleuven.be> --- tools/testing/selftests/sgx/main.c | 5 + tools/testing/selftests/sgx/test_encl.c | 161 ++++++++++++++++++------ 2 files changed, 128 insertions(+), 38 deletions(-)