Added cert generation and verification APIs for attested TLS #1761

soccerGB · 2019-05-03T19:09:10Z

This is certification generation (oe_gen_tls_cert) and verification (oe_verify_tls_cert) part of attested tls feature.
#1325

I am hoping to preliminary feedback whether the implementation of those two new APIs impacts other features/refactoring activities under construction.

End-2-end test and samples will come in a separate PR once its depending components (socket) are ready.

3rdparty/mbedtls/mbedtls/library/x509write_crt.c

common/sgx/tlsverifier.c

enclave/cert.c

common/sgx/tlsverifier.c

3rdparty/mbedtls/mbedtls/library/x509write_crt.c

common/sgx/tlsverifier.c

enclave/tls_cert.c

anakrish · 2019-05-06T23:12:04Z

enclave/core/string.c

+    if (n)
+        *dest = '\0';
+
+    return ret;


@mikbras @soccerGB @CodeMonkeyLeet @paulcallen @gupta-ak
The need to implement and review oe_strncpy is one of the reasons why corelibc is more trouble than it is worth.

This implementation of oe_strncpy looks correct - copy at most n characters from src to dst - a bounded string-copy.

However this implementation is incorrect because, counterintuitively, strncpy has different semantics according to the C standard. strncpy requires that the destination be padded with null characters if the src contains less that n characters.

From strncpy C++ reference

Copies the first num characters of source to destination. If the end of the source C string (which is signaled by a null-character) is found before num characters have been copied, destination is padded with zeros until a total of num characters have been written to it.

From Ansi C Rationale

strncpy was initially introduced into the C library to deal with fixed-length name fields in structures such as directory entries. Such fields are not used in the same way as strings: the trailing null is unnecessary for a maximum-length field, and setting trailing bytes for shorter names to null assures efficient field-wise comparisons. strncpy is not by origin a ``bounded strcpy,'' and the Committee has preferred to recognize existing practice rather than alter the function to better suit it to such use.

C libraries take great pains to implement the standard; no matter how counter-intuitive the standard is. Take for example MUSL's implementation of which strncpy forwards to:
https://github.com/microsoft/openenclave/blob/fe0c6af59d1d3ec038c47cda52555cdf351b10f8/3rdparty/musl/musl/src/string/stpncpy.c#L10-L29

MUSL's strncpy follows the C standard by null-padding the destination bytes. Additionally, when used as a glibc replacement, it does glibc compatible optimizations.

Since crypto is a key component in writing secure code, we need to shy away from writing new code in the hot path. Instead we need to rely on battle tested code like MUSL or GLIBC (along with their security fixes).
If we start implementing libc functions, when we are already using a full fledged libc implementation like MUSL, we ought to lock down each C function we implement rigorously with tests and prove to ourselves that we are standards compliant. At this moment I don't see rigorous tests for corelibc.

@anakrish This is a really good observation, can you please spin up an issue to track this? It suggests a broader scope of work we should tackle more comprehensively in terms of the implementation of corelibc which also intersects with @mikbras work.

Agree with Anand here.

+1. If we have a POSIX name we must adhere to the standard. strncpy as specified in the standard is dangerous since it does not guarantee null termination (as explained at the link Anand cites). As such, if we make oe_strncpy be public, then it should probably be marked as deprecated or unsafe or something, so it will generate warnings if an app tries to use it. strncpy_s is the safer API to use.

This is strncpy was added for the purpose getting mbedtls built on top of corelibc, where once we starting building mbedtls on top of the musl, this direct corelibc dependency will be removed thus the concern raised will be gone because this strncpy will become no longer needed.

Can you open an issue to keep track of removing this strncpy implementation or fixing it to be libc compliant before the next release? Otherwise, we would increase the risk of unpredictable behavior/potential security vulnerability.

soccerGB · 2019-05-07T09:53:39Z

bors try

soccerGB · 2019-05-07T21:35:18Z

bors try

oe-bors · 2019-05-07T21:35:19Z

try

Not awaiting review

oe-bors · 2019-05-08T00:07:02Z

try

Build failed

continuous-integration/jenkins/branch

CodeMonkeyLeet

We should probably have a full review on the new additions to the public API surface.

CodeMonkeyLeet · 2019-05-07T23:09:48Z

include/openenclave/internal/report.h

@@ -161,6 +161,13 @@ OE_STATIC_ASSERT(sizeof(oe_report_header_t) == 16);
 OE_STATIC_ASSERT(
    OE_OFFSETOF(oe_report_header_t, report) == sizeof(oe_report_header_t));

+// ISO(1).ANSI(2).USA(840).Microsoft(113556).ACC(10).Classes(103).Subclass(1)


The comment doesn't match the OID we're using. (Classes is 1, not 103)

Have we registered this with the OID range owners in MS to avoid collisions?

Mismatched class id fixed.
Still figuring out how to register this OID

include/openenclave/host.h

3rdparty/mbedtls/mbedtls/library/x509write_crt.c

CodeMonkeyLeet · 2019-05-09T00:50:19Z

enclave/core/string.c

+    if (n)
+        *dest = '\0';
+
+    return ret;


@anakrish This is a really good observation, can you please spin up an issue to track this? It suggests a broader scope of work we should tackle more comprehensively in terms of the implementation of corelibc which also intersects with @mikbras work.

include/openenclave/enclave.h

include/openenclave/host.h

include/openenclave/internal/cert.h

common/sgx/tlsverifier.c

3rdparty/mbedtls/mbedtls/library/x509write_crt.c

dthaler · 2019-05-14T23:19:07Z

enclave/cert.c

+        OE_RAISE(OE_INVALID_PARAMETER);
+
+    /* Allocate memory for the certificate */
+    if (!(crt = mbedtls_calloc(1, sizeof(mbedtls_x509_crt))))


Personally I think it's bad for a core OE file to directly hard code mbedTLS specific API calls instead of going through an abstraction layer. I thought that was being addressed.

This cert.c is part of the crypto implementation for the enclave, which depends solely on the mbedtls library.
Before that dependency was replaced/removed, we will have to keep it this way.

I can't parse the last sentence in your reply. Do you mean "Until that is done"? meaning it's future work to fix this? Or what? I still maintain its bad to call mbedtls APIs directly from OE core code.

dthaler · 2019-05-14T23:23:40Z

enclave/core/string.c

+    if (n)
+        *dest = '\0';
+
+    return ret;


+1. If we have a POSIX name we must adhere to the standard. strncpy as specified in the standard is dangerous since it does not guarantee null termination (as explained at the link Anand cites). As such, if we make oe_strncpy be public, then it should probably be marked as deprecated or unsafe or something, so it will generate warnings if an app tries to use it. strncpy_s is the safer API to use.

enclave/tls_cert.c

tests/tls_apis/enc/enc.cpp

tests/tls_apis/host/host.cpp

dthaler · 2019-05-24T14:01:05Z

include/openenclave/enclave.h

@@ -724,6 +724,76 @@ oe_enclave_t* oe_get_enclave(void);
 */
 oe_result_t oe_random(void* data, size_t size);

+/**
+ * oe_generate_attestation_cert.


Don't use abbreviations ("cert") in non-POSIX API names.

thanks for spotting this.
Renamed to
oe_generate_attestation_certificate
oe_free_attestation_certificate
oe_verify_attestation_certificate

dthaler · 2019-05-24T14:01:18Z

include/openenclave/enclave.h

+/**
+ * oe_generate_attestation_cert.
+ *
+ * This function generates a self-signed x509 certificate with an embedded


x509 -> X.509

thanks for spotting this. Fixed.

dthaler · 2019-05-24T14:01:39Z

include/openenclave/enclave.h

+ * This function generates a self-signed x509 certificate with an embedded
+ * quote from the underlying enclave.
+ *
+ * @param[in] subject_name a string contains contain an X.509 distinguished


"contains contain" -> "containing"

thanks for spotting this. Fixed.

dthaler · 2019-05-24T14:02:35Z

include/openenclave/enclave.h

+ * See RFC5280 (https://tools.ietf.org/html/rfc5280) specification for details.
+ * Example value "CN=Open Enclave SDK,O=OESDK TLS,C=US"
+ *
+ * @param[in] private_key a private key used to sign this certificate


nit: some lines end in period (740, 742) and some don't (739, 741). Be consistent throughout this file.

thanks for spotting this. Fixed.

Nit is not yet fixed. See line 742

dthaler · 2019-05-24T14:03:39Z

include/openenclave/enclave.h

+/**
+ * oe_verify_attestation_cert
+ *
+ * This function preform a custom validation on the input certificate. This


"preform" -> "perform"

thanks for spotting this. Fixed.

include/openenclave/enclave.h

dthaler · 2019-05-24T14:04:36Z

include/openenclave/enclave.h

+ * @param[in] cert_in_der_len size of certificate buffer above
+ * @param[in] enclave_identity_callback callback routine for custom identity
+ * checking
+ * @param[in] arg optional argument


what is "arg" used for? "optional argument" is not a helpful description

rephrased to

@param[in] arg an optional context pointer argument specified by the

application when setting callback

dthaler · 2019-05-24T14:05:28Z

include/openenclave/enclave.h

+ * oe_verify_attestation_cert
+ *
+ * This function preform a custom validation on the input certificate. This
+ * validation includes extracting a quote extension from the certificate before


I think the term "quote" is SGX specific and does not belong in the doxygen comments at all, unless used in an example where it's clear it's using SGX as the example

good point.
I replaced "quote" with "attestation evidence".

dthaler · 2019-05-24T14:06:16Z

include/openenclave/host.h

+/**
+ * oe_verify_attestation_cert
+ *
+ * This function preform a custom validation on the input certificate. This


Same comments in host.h as I added into enclave.h

all feedback were addressed

soccerGB · 2019-05-25T01:30:38Z

bors r+

1761: Added cert generation and verification APIs for attested TLS r=soccerGB a=soccerGB This is certification generation (oe_gen_tls_cert) and verification (oe_verify_tls_cert) part of attested tls feature. #1325 I am hoping to preliminary feedback whether the implementation of those two new APIs impacts other features/refactoring activities under construction. End-2-end test and samples will come in a separate PR once its depending components (socket) are ready. Co-authored-by: Cheng-mean Liu <[email protected]>

oe-bors · 2019-05-25T01:54:12Z

Build failed

continuous-integration/jenkins/branch

common/sgx/tlsverifier.c

enclave/cert.c

include/openenclave/internal/cert.h

gupta-ak · 2019-05-25T01:58:59Z

tests/tls_apis/enc/CMakeLists.txt

+
+target_include_directories(tls_enc PRIVATE
+                          ${CMAKE_CURRENT_BINARY_DIR}
+                          ${PROJECT_SOURCE_DIR}/include/openenclave/corelibc)


Why is corelibc part of the includes if you're already linking with oelibc? You should have the headers from musl.

This is an internal test case, where a couple of internal oe_* api, defined in oecorelibc, were used. That's why corelibc path was included.
In the upcoming external same code, it will solely base on the musl headers.

gupta-ak · 2019-05-25T01:59:29Z

tests/tls_apis/enc/enc.cpp

+/* Only map CHAR_BIT out of limits.h to scope potential standard definition
+ * name conflicts in enclave sources.
+ */
+#if !defined(CHAR_BIT)


I don't think you need to do this if you remove the corelibc includes.

tests/tls_apis/enc/enc.cpp

soccerGB · 2019-05-25T22:12:47Z

bors try

oe-bors · 2019-05-25T22:37:36Z

try

Build failed

continuous-integration/jenkins/branch

soccerGB · 2019-05-26T00:17:01Z

bors r+

1761: Added cert generation and verification APIs for attested TLS r=soccerGB a=soccerGB This is certification generation (oe_gen_tls_cert) and verification (oe_verify_tls_cert) part of attested tls feature. #1325 I am hoping to preliminary feedback whether the implementation of those two new APIs impacts other features/refactoring activities under construction. End-2-end test and samples will come in a separate PR once its depending components (socket) are ready. Co-authored-by: Cheng-mean Liu <[email protected]>

oe-bors · 2019-05-26T00:41:21Z

Build succeeded

continuous-integration/jenkins/branch

1818: Remove OE_USE_LIBSGX on verifying report. r=soccerGB a=gupta-ak This allows remote quote verification without `OE_USE_LIBSGX`. It's essentially a piece of PR #1575 to unblock Cheng-mean's PR #1761. A way to test this is to run the report test with a pre-generated report. That test should now pass. Co-authored-by: Akash Gupta <[email protected]>

anakrish · 2019-05-27T02:18:47Z

3rdparty/mbedtls/mbedtls/library/x509write_crt.c

                                        sig_oid, sig_oid_len, sig, sig_len ) );

-    if( len > (size_t)( c2 - buf ) )
-        return( MBEDTLS_ERR_ASN1_BUF_TOO_SMALL );
+    /*


@soccerGB The previous version returned MBEDTLS_ERR_ASN1_BUF_TOO_SMALL.
I cannot spot where the new version returns the same error code.

anakrish · 2019-05-27T02:24:52Z

enclave/cert.c

+    int rc = 0;
+
+    /* Clear the implementation */
+    if (impl)


If an impl already exists, then shouldn't we be cleaning up contents of the impl rather than just doing memset?

anakrish · 2019-05-27T02:33:14Z

host/crypto/openssl/cert.c

@@ -665,7 +707,7 @@ oe_result_t oe_cert_verify(
    }

    /* Check for invalid chain parameter */
-    if (!_cert_chain_is_valid(chain_impl))
+    if (chain && !_cert_chain_is_valid(chain_impl))


Should this be chain_impl && ?

dthaler · 2019-05-28T20:31:42Z

3rdparty/mbedtls/mbedtls/library/x509write_crt.c

     */
-    c = tmp_buf + sizeof( tmp_buf );
+    c = buf + size;


These look like changes to mbedtls itself. Are they being contributed back upstream to the master mbedtls? If not, why not? If so, what's the link to the pull request or commit?

dthaler · 2019-05-28T20:32:55Z

enclave/cert.c

+        OE_RAISE(OE_INVALID_PARAMETER);
+
+    /* Allocate memory for the certificate */
+    if (!(crt = mbedtls_calloc(1, sizeof(mbedtls_x509_crt))))


I can't parse the last sentence in your reply. Do you mean "Until that is done"? meaning it's future work to fix this? Or what? I still maintain its bad to call mbedtls APIs directly from OE core code.

dthaler · 2019-05-28T20:36:05Z

enclave/tls_cert.c

+#define ISSUER_NAME "CN=Open Enclave SDK,O=OESDK TLS,C=UK"
+#define SUBJECT_NAME ISSUER_NAME
+#define DATE_NOT_VALID_BEFORE "20190501000000"
+#define DATE_NOT_VALID_AFTER "20501231235959"


I think it's bad to hard code any timestamps in code, as opposed to in admin-configurable policy.

dthaler · 2019-05-28T20:37:08Z

include/openenclave/enclave.h

+ * See RFC5280 (https://tools.ietf.org/html/rfc5280) specification for details.
+ * Example value "CN=Open Enclave SDK,O=OESDK TLS,C=US"
+ *
+ * @param[in] private_key a private key used to sign this certificate


Nit is not yet fixed. See line 742

dthaler · 2019-05-28T20:38:51Z

include/openenclave/enclave.h

+ * Free the given cert
+ * @param[in] cert If not NULL, the buffer to free.
+ */
+void oe_free_attestation_certificate(uint8_t* cert);


style comment: In Microsoft public headers and sample code, we try to avoid abbreviations not just in method names, but also variable/parameter names. "cert" should be expanded to "certificate" in 755, 756, 760, 762, etc., for consistency with the method name. (Acronyms on the other hand are fine, so "der" is perfectly fine in 794.) There are a very small number of exceptions where abbreviations are ok. Those include "app" and "arg" (which is used in this file and is ok.)

dthaler · 2019-05-28T20:43:25Z

tests/attestation_cert_apis/README.md

+- **Host Side**
+  1. Create an enclave
+  2. Issue an ecall (get_tls_cert) into enclave for getting a self-signed certificate embedded with an quote of the enclave
+  3. Once the certificate is received, call oe_verify_attestation_cert to verify the certificate and quote


cert -> certificate (and same in oe_generate_attestation_cert on line 12)

dthaler · 2019-05-28T20:46:03Z

tests/attestation_cert_apis/host/host.cpp

+#define TEST_RSA_KEY 1
+#define SKIP_RETURN_CODE 2
+
+// This is the identity validation callback. An TLS connecting party (client or


grammar nit: "An TLS" -> "A TLS"

dthaler · 2019-05-28T20:47:21Z

tests/attestation_cert_apis/host/host.cpp

+            "./cert_%s.der",
+            test_type == TEST_RSA_KEY ? "rsa" : "ec");
+        OE_TRACE_INFO(
+            "Host: Log quote embedded certificate to file: [%s]\n", filename);


I believe the term "quote" is SGX specific. Use platform agnostic terms in trace output from non-SGX-specific code

soccerGB added the do not merge Indicates a PR should not be merged until removed. label May 3, 2019

soccerGB requested review from radhikaj, gupta-ak, anakrish, CodeMonkeyLeet and johnkord May 3, 2019 19:09

johnkord reviewed May 3, 2019

View reviewed changes

3rdparty/mbedtls/mbedtls/library/x509write_crt.c Outdated Show resolved Hide resolved

radhikaj reviewed May 4, 2019

View reviewed changes

common/sgx/tlsverifier.c Outdated Show resolved Hide resolved

enclave/cert.c Show resolved Hide resolved

enclave/cert.c Outdated Show resolved Hide resolved

radhikaj reviewed May 4, 2019

View reviewed changes

common/sgx/tlsverifier.c Outdated Show resolved Hide resolved

soccerGB requested a review from anitagov May 4, 2019 00:30

anakrish previously requested changes May 4, 2019

View reviewed changes

soccerGB force-pushed the soccerl/tls_cert_api branch from e029a6f to b8424b2 Compare May 6, 2019 20:30

anakrish reviewed May 6, 2019

View reviewed changes

andyleejordan added this to the v0.6 milestone May 7, 2019

andyleejordan added the functionality Issue describes an enhancement or addition of functionality to Open Enclave SDK label May 7, 2019

oe-bors bot pushed a commit that referenced this pull request May 7, 2019

Try #1761:

ffa0dab

CodeMonkeyLeet previously requested changes May 11, 2019

View reviewed changes

radhikaj reviewed May 14, 2019

View reviewed changes

common/sgx/tlsverifier.c Outdated Show resolved Hide resolved

dthaler requested changes May 14, 2019

View reviewed changes

soccerGB force-pushed the soccerl/tls_cert_api branch 2 times, most recently from ef7d6e0 to 960a17b Compare May 22, 2019 01:07

hanno-becker mentioned this pull request May 22, 2019

Size limit is too small for x509 extension size(2KB) and x509 certificate size(4KB) in x509write_crt.c Mbed-TLS/mbedtls#2631

Closed

soccerGB removed the do not merge Indicates a PR should not be merged until removed. label May 22, 2019

gupta-ak mentioned this pull request May 23, 2019

Remove OE_USE_LIBSGX on verifying report. #1818

Merged

dthaler previously requested changes May 24, 2019

View reviewed changes

Added cert generation and verification APIs for attested TLS

feecfd9

radhikaj approved these changes May 24, 2019

View reviewed changes

Updated CHANGELOG and fixed a merge error

043eede

soccerGB force-pushed the soccerl/tls_cert_api branch from 4313fed to 043eede Compare May 25, 2019 00:11

gupta-ak reviewed May 25, 2019

View reviewed changes

tests/tls_apis/enc/enc.cpp Outdated Show resolved Hide resolved

Addressed Akash feedback

473c6d3

oe-bors bot pushed a commit that referenced this pull request May 25, 2019

Try #1761:

7294961

removed uncessary error message

6b1cf36

Skipped attestation_cert_test from running in the simulation mode

a66362f

soccerGB force-pushed the soccerl/tls_cert_api branch from 2ebf7eb to a66362f Compare May 25, 2019 23:54

oe-bors bot merged commit a66362f into master May 26, 2019

oe-bors bot deleted the soccerl/tls_cert_api branch May 26, 2019 00:41

anakrish reviewed May 27, 2019

View reviewed changes

dthaler reviewed May 28, 2019

View reviewed changes

CodeMonkeyLeet mentioned this pull request Jun 4, 2019

Need to add support for establishing secure channel between enclaves : TLS with remote attestation #1325

Closed

CodeMonkeyLeet mentioned this pull request Sep 13, 2019

Remove customization of mbedTLS x509write_crt.c #2179

Open

Added cert generation and verification APIs for attested TLS #1761

Added cert generation and verification APIs for attested TLS #1761

Conversation

soccerGB commented May 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soccerGB commented May 7, 2019

soccerGB commented May 7, 2019

oe-bors bot commented May 7, 2019

try

oe-bors bot commented May 8, 2019

try

Build failed

CodeMonkeyLeet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soccerGB commented May 25, 2019

oe-bors bot commented May 25, 2019

Build failed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soccerGB commented May 25, 2019

oe-bors bot commented May 25, 2019

try

Build failed

soccerGB commented May 26, 2019

oe-bors bot commented May 26, 2019

Build succeeded

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soccerGB commented May 3, 2019 •

edited

Loading