feat: add checked return values diagnostic

crypto/s2n_ecdsa.c

@@ -41,12 +41,12 @@ EC_KEY *s2n_unsafe_ecdsa_get_non_const(const struct s2n_ecdsa_key *ecdsa_key)
     PTR_ENSURE_REF(ecdsa_key);
 
     /* pragma gcc diagnostic was added in gcc 4.6 */
-#if defined(__clang__) || S2N_GCC_VERSION_AT_LEAST(4, 6, 0)
+#ifdef S2N_DIAGNOSTICS_SUPPORTED
     #pragma GCC diagnostic push
     #pragma GCC diagnostic ignored "-Wcast-qual"
 #endif
     EC_KEY *out_ec_key = (EC_KEY *) ecdsa_key->ec_key;
-#if defined(__clang__) || S2N_GCC_VERSION_AT_LEAST(4, 6, 0)
+#ifdef S2N_DIAGNOSTICS_SUPPORTED
     #pragma GCC diagnostic pop
 #endif
 

crypto/s2n_rsa.c

@@ -37,12 +37,12 @@ RSA *s2n_unsafe_rsa_get_non_const(const struct s2n_rsa_key *rsa_key)
     PTR_ENSURE_REF(rsa_key);
 
     /* pragma gcc diagnostic was added in gcc 4.6 */
-#if defined(__clang__) || S2N_GCC_VERSION_AT_LEAST(4, 6, 0)
+#ifdef S2N_DIAGNOSTICS_SUPPORTED
     #pragma GCC diagnostic push
     #pragma GCC diagnostic ignored "-Wcast-qual"
 #endif
     RSA *out_rsa_key = (RSA *) rsa_key->rsa;
-#if defined(__clang__) || S2N_GCC_VERSION_AT_LEAST(4, 6, 0)
+#ifdef S2N_DIAGNOSTICS_SUPPORTED
     #pragma GCC diagnostic pop
 #endif
 

pq-crypto/kyber_r3/kyber512r3_kem.c

@@ -35,7 +35,7 @@ int s2n_kyber_512_r3_crypto_kem_keypair(uint8_t *pk, uint8_t *sk)
     {
         POSIX_GUARD(indcpa_keypair(pk, sk));
     }
-    
+
     for(size_t i = 0; i < S2N_KYBER_512_R3_INDCPA_PUBLICKEYBYTES; i++) {
         sk[i + S2N_KYBER_512_R3_INDCPA_SECRETKEYBYTES] = pk[i];
     }
@@ -84,7 +84,7 @@ int s2n_kyber_512_r3_crypto_kem_enc(uint8_t *ct, uint8_t *ss, const uint8_t *pk)
     {
         indcpa_enc(ct, buf, pk, kr+S2N_KYBER_512_R3_SYMBYTES);
     }
-    
+
     /* overwrite coins in kr with H(c) */
     sha3_256(kr+S2N_KYBER_512_R3_SYMBYTES, ct, S2N_KYBER_512_R3_CIPHERTEXT_BYTES);
     /* hash concatenation of pre-k and H(c) to k */
@@ -126,7 +126,7 @@ int s2n_kyber_512_r3_crypto_kem_dec(uint8_t *ss, const uint8_t *ct, const uint8_
     {
         indcpa_dec(buf, ct, sk);
     }
-    
+
     /* Multitarget countermeasure for coins + contributory KEM */
     for(size_t i = 0; i < S2N_KYBER_512_R3_SYMBYTES; i++) {
         buf[S2N_KYBER_512_R3_SYMBYTES + i] = sk[S2N_KYBER_512_R3_SECRET_KEY_BYTES - 2 * S2N_KYBER_512_R3_SYMBYTES + i];
@@ -142,7 +142,7 @@ int s2n_kyber_512_r3_crypto_kem_dec(uint8_t *ss, const uint8_t *ct, const uint8_
     {
         indcpa_enc(cmp, buf, pk, kr+S2N_KYBER_512_R3_SYMBYTES);
     }
-    
+
     /* If ct and cmp are equal (dont_copy = 1), decryption has succeeded and we do NOT overwrite pre-k below.
      * If ct and cmp are not equal (dont_copy = 0), decryption fails and we do overwrite pre-k. */
     int dont_copy = s2n_constant_time_equals(ct, cmp, S2N_KYBER_512_R3_CIPHERTEXT_BYTES);
@@ -151,8 +151,8 @@ int s2n_kyber_512_r3_crypto_kem_dec(uint8_t *ss, const uint8_t *ct, const uint8_
     sha3_256(kr+S2N_KYBER_512_R3_SYMBYTES, ct, S2N_KYBER_512_R3_CIPHERTEXT_BYTES);
 
     /* Overwrite pre-k with z on re-encryption failure */
-    POSIX_GUARD(s2n_constant_time_copy_or_dont(kr, sk+S2N_KYBER_512_R3_SECRET_KEY_BYTES-S2N_KYBER_512_R3_SYMBYTES,
-            S2N_KYBER_512_R3_SYMBYTES, dont_copy));
+    s2n_constant_time_copy_or_dont(kr, sk+S2N_KYBER_512_R3_SECRET_KEY_BYTES-S2N_KYBER_512_R3_SYMBYTES,
+        S2N_KYBER_512_R3_SYMBYTES, dont_copy);
 
     /* hash concatenation of pre-k and H(c) to k */
     shake256(ss, S2N_KYBER_512_R3_SSBYTES, kr, 2*S2N_KYBER_512_R3_SYMBYTES);

scripts/s2n_safety_macros.py

@@ -107,7 +107,7 @@ def cmp_check(op):
 MACROS = {
     'BAIL(error)': dict(
         doc  = 'Sets the global `s2n_errno` to `error` and returns with an `{error}`',
-        impl = 'do {{ _S2N_ERROR((error)); return {error}; }} while (0)',
+        impl = 'do {{ _S2N_ERROR((error)); __S2N_ENSURE_CHECKED_RETURN({error}); }} while (0)',
         harness = '''
         static {ret} {bail}_harness()
         {{
@@ -588,7 +588,7 @@ def cmp_check(op):
     ),
     'GUARD(result)': dict(
         doc  = 'Ensures `{is_ok}`, otherwise the function will return `{error}`',
-        impl = '__S2N_ENSURE({is_ok}, return {error})',
+        impl = '__S2N_ENSURE({is_ok}, __S2N_ENSURE_CHECKED_RETURN({error}))',
         harness = '''
         static {ret} {prefix}GUARD_harness({ret} result)
         {{
@@ -714,7 +714,7 @@ def push_doc(args):
             if other == context:
                 continue;
 
-            impl = '__S2N_ENSURE({is_ok}, return {error})'
+            impl = '__S2N_ENSURE({is_ok}, __S2N_ENSURE_CHECKED_RETURN({error}))'
             args = {
                 'prefix': context['prefix'],
                 'suffix': other['suffix'],

tests/features/S2N_DIAGNOSTICS_SUPPORTED.c

@@ -0,0 +1,30 @@
+/*
+ * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License").
+ * You may not use this file except in compliance with the License.
+ * A copy of the License is located at
+ *
+ *  http://aws.amazon.com/apache2.0
+ *
+ * or in the "license" file accompanying this file. This file is distributed
+ * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
+ * express or implied. See the License for the specific language governing
+ * permissions and limitations under the License.
+ */
+
+#define MACRO_CHECK \
+    do { \
+        _Pragma("GCC diagnostic push") \
+        _Pragma("GCC diagnostic error \"-Wconversion\"") \
+        return -1; \
+        _Pragma("GCC diagnostic pop") \
+    } while (0)
+
+int main() {
+    #pragma GCC diagnostic push
+    #pragma GCC diagnostic ignored "-Wcast-qual"
+    #pragma GCC diagnostic pop
+
+    MACRO_CHECK;
+}

tests/features/S2N_DIAGNOSTICS_SUPPORTED.flags

@@ -0,0 +1 @@
+-Werror

tests/sidetrail/working/stubs/s2n_ensure.h

@@ -43,6 +43,7 @@ void *s2n_sidetrail_memset(void * ptr, int value, size_t num);
     }                                                      \
   } while(0)
 
+#define __S2N_ENSURE_CHECKED_RETURN(v)                     do { return v; } while(0)
 
 /**
  * The C runtime does not give a way to check these properties,

tests/unit/s2n_client_key_share_extension_test.c

@@ -42,6 +42,26 @@ static int s2n_write_named_curve(struct s2n_stuffer *out, const struct s2n_ecc_n
 static int s2n_write_key_share(struct s2n_stuffer *out, uint16_t iana_value, uint16_t share_size,
         const struct s2n_ecc_named_curve *existing_curve);
 
+S2N_RESULT s2n_extensions_client_key_share_size(struct s2n_connection *conn, uint32_t *size)
+{
+    RESULT_ENSURE_REF(conn);
+
+    const struct s2n_ecc_preferences *ecc_pref = NULL;
+    RESULT_GUARD_POSIX(s2n_connection_get_ecc_preferences(conn, &ecc_pref));
+    RESULT_ENSURE_REF(ecc_pref);
+
+    uint32_t s2n_client_key_share_extension_size = S2N_SIZE_OF_EXTENSION_TYPE
+            + S2N_SIZE_OF_EXTENSION_DATA_SIZE
+            + S2N_SIZE_OF_CLIENT_SHARES_SIZE;
+
+    s2n_client_key_share_extension_size += S2N_SIZE_OF_KEY_SHARE_SIZE + S2N_SIZE_OF_NAMED_GROUP;
+    s2n_client_key_share_extension_size += ecc_pref->ecc_curves[0]->share_size;
+
+    *size = s2n_client_key_share_extension_size;
+
+    return S2N_RESULT_OK;
+}
+
 int main(int argc, char **argv)
 {
     BEGIN_TEST();
@@ -53,12 +73,12 @@ int main(int argc, char **argv)
         struct s2n_connection *conn;
         EXPECT_NOT_NULL(conn = s2n_connection_new(S2N_CLIENT));
 
-        int key_share_size;
-        EXPECT_SUCCESS(key_share_size = s2n_extensions_client_key_share_size(conn));
+        uint32_t key_share_size = 0;
+        EXPECT_OK(s2n_extensions_client_key_share_size(conn, &key_share_size));
 
         /* should produce the same result if called twice */
-        int key_share_size_again;
-        EXPECT_SUCCESS(key_share_size_again = s2n_extensions_client_key_share_size(conn));
+        uint32_t key_share_size_again = 0;
+        EXPECT_OK(s2n_extensions_client_key_share_size(conn, &key_share_size_again));
         EXPECT_EQUAL(key_share_size, key_share_size_again);
 
         /* should equal the size of the data written on send */

tls/extensions/s2n_client_key_share.c

@@ -451,24 +451,6 @@ static int s2n_client_key_share_recv(struct s2n_connection *conn, struct s2n_stu
 
 /* Old-style extension functions -- remove after extensions refactor is complete */
 
-uint32_t s2n_extensions_client_key_share_size(struct s2n_connection *conn)
-{
-    POSIX_ENSURE_REF(conn);
-
-    const struct s2n_ecc_preferences *ecc_pref = NULL;
-    POSIX_GUARD(s2n_connection_get_ecc_preferences(conn, &ecc_pref));
-    POSIX_ENSURE_REF(ecc_pref);
-
-    uint32_t s2n_client_key_share_extension_size = S2N_SIZE_OF_EXTENSION_TYPE
-            + S2N_SIZE_OF_EXTENSION_DATA_SIZE
-            + S2N_SIZE_OF_CLIENT_SHARES_SIZE;
-
-    s2n_client_key_share_extension_size += S2N_SIZE_OF_KEY_SHARE_SIZE + S2N_SIZE_OF_NAMED_GROUP;
-    s2n_client_key_share_extension_size += ecc_pref->ecc_curves[0]->share_size;
-
-    return s2n_client_key_share_extension_size;
-}
-
 int s2n_extensions_client_key_share_recv(struct s2n_connection *conn, struct s2n_stuffer *extension)
 {
     return s2n_extension_recv(&s2n_client_key_share_extension, conn, extension);

tls/extensions/s2n_client_key_share.h

@@ -22,4 +22,3 @@ extern const s2n_extension_type s2n_client_key_share_extension;
 
 /* Old-style extension functions -- remove after extensions refactor is complete */
 int s2n_extensions_client_key_share_recv(struct s2n_connection *conn, struct s2n_stuffer *extension);
-uint32_t s2n_extensions_client_key_share_size(struct s2n_connection *conn);

tls/s2n_connection.c

@@ -537,7 +537,7 @@ int s2n_connection_wipe(struct s2n_connection *conn)
     /* Clone the stuffers */
     /* ignore gcc 4.7 address warnings because dest is allocated on the stack */
     /* pragma gcc diagnostic was added in gcc 4.6 */
-#if S2N_GCC_VERSION_AT_LEAST(4, 6, 0)
+#ifdef S2N_DIAGNOSTICS_SUPPORTED
     #pragma GCC diagnostic push
     #pragma GCC diagnostic ignored "-Waddress"
 #endif
@@ -549,7 +549,7 @@ int s2n_connection_wipe(struct s2n_connection *conn)
     POSIX_CHECKED_MEMCPY(&header_in, &conn->header_in, sizeof(struct s2n_stuffer));
     POSIX_CHECKED_MEMCPY(&in, &conn->in, sizeof(struct s2n_stuffer));
     POSIX_CHECKED_MEMCPY(&out, &conn->out, sizeof(struct s2n_stuffer));
-#if S2N_GCC_VERSION_AT_LEAST(4, 6, 0)
+#ifdef S2N_DIAGNOSTICS_SUPPORTED
     #pragma GCC diagnostic pop
 #endif
 
@@ -1076,21 +1076,37 @@ int s2n_connection_set_blinding(struct s2n_connection *conn, s2n_blinding blindi
 #define ONE_S INT64_C(1000000000)
 #define TEN_S INT64_C(10000000000)
 
-uint64_t s2n_connection_get_delay(struct s2n_connection *conn)
+static S2N_RESULT s2n_connection_get_delay_impl(struct s2n_connection *conn, uint64_t *delay)
 {
+    RESULT_ENSURE_REF(conn);
+    RESULT_ENSURE_REF(delay);
+
     if (!conn->delay) {
-        return 0;
+        *delay = 0;
+        return S2N_RESULT_OK;
     }
 
-    uint64_t elapsed;
-    /* This will cast -1 to max uint64_t */
-    POSIX_GUARD_RESULT(s2n_timer_elapsed(conn->config, &conn->write_timer, &elapsed));
+    uint64_t elapsed = 0;
+    RESULT_GUARD(s2n_timer_elapsed(conn->config, &conn->write_timer, &elapsed));
 
     if (elapsed > conn->delay) {
-        return 0;
+        *delay = 0;
+        return S2N_RESULT_OK;
     }
 
-    return conn->delay - elapsed;
+    *delay = conn->delay - elapsed;
+
+    return S2N_RESULT_OK;
+}
+
+uint64_t s2n_connection_get_delay(struct s2n_connection *conn)
+{
+    uint64_t delay = 0;
+    if (s2n_result_is_ok(s2n_connection_get_delay_impl(conn, &delay))) {
+        return delay;
+    } else {
+        return UINT64_MAX;
+    }
 }
 
 static S2N_RESULT s2n_connection_kill(struct s2n_connection *conn)

utils/s2n_ensure.h

@@ -79,6 +79,17 @@
         }                                        \
     } while (0)
 
+#if defined(S2N_DIAGNOSTICS_SUPPORTED)
+    #define __S2N_ENSURE_CHECKED_RETURN(v)                                     \
+        do {                                                                   \
+            _Pragma("GCC diagnostic push")                                     \
+                    _Pragma("GCC diagnostic error \"-Wconversion\"") return v; \
+            _Pragma("GCC diagnostic pop")                                      \
+        } while (0)
+#else
+    #define __S2N_ENSURE_CHECKED_RETURN(v) return v
+#endif
+
 /**
  * `restrict` is a part of the c99 standard and will work with any C compiler. If you're trying to
  * compile with a C++ compiler `restrict` is invalid. However some C++ compilers support the behavior