diff --git a/include/ap_mmn.h b/include/ap_mmn.h index 09770fa1a57..ccd504d8e82 100644 --- a/include/ap_mmn.h +++ b/include/ap_mmn.h @@ -736,6 +736,7 @@ * 20211221.28 (2.5.1-dev) Add dav_get_base_path() to mod_dav * 20211221.29 (2.5.1-dev) Add ap_set_time_process_request() to scoreboard.h * 20211221.30 (2.5.1-dev) Add ap_stat_check() to httpd.h + * 20211221.31 (2.5.1-dev) Add ap_*_timingsafe() to httpd.h */ #define MODULE_MAGIC_COOKIE 0x41503235UL /* "AP25" */ @@ -743,7 +744,7 @@ #ifndef MODULE_MAGIC_NUMBER_MAJOR #define MODULE_MAGIC_NUMBER_MAJOR 20211221 #endif -#define MODULE_MAGIC_NUMBER_MINOR 30 /* 0...n */ +#define MODULE_MAGIC_NUMBER_MINOR 31 /* 0...n */ /** * Determine if the server's current MODULE_MAGIC_NUMBER is at least a diff --git a/include/httpd.h b/include/httpd.h index b12b7e4e21b..03376db9958 100644 --- a/include/httpd.h +++ b/include/httpd.h @@ -2261,6 +2261,54 @@ AP_DECLARE(int) ap_ind(const char *str, char c); /* Sigh... */ */ AP_DECLARE(int) ap_rind(const char *str, char c); +/** + * Check whether two buffers of equal size have the same content, using a + * constant time algorithm (branch-less with regard to the content of the + * buffers and an execution time solely dependent on the number of bytes + * compared, not the bytes themselves). + * + * @param buf1 first buffer to compare + * @param buf2 second buffer to compare + * @param n number of bytes to compare + * @return 1 if equal, 0 otherwise + */ +AP_DECLARE(int) ap_memeq_timingsafe(const void *buf1, const void *buf2, + apr_size_t n); + +/** + * Check whether two NUL-terminated strings have the same content, using a + * constant time algorithm (branch-less with regard to the content of the + * secret string and an execution time solely dependent on the length of + * the non-secret string). The secret string of the two should be set in + * the first parameter \c sec1 to avoid leaking its length. + * + * @param sec1 first string to compare (the secret one) + * @param str2 second string to compare + * @return 1 if equal, 0 otherwise + * @remark The function will compare as much characters as there are in + * \c str2, so the length of \c str2 might leak through side channel, + * while the length of \c sec1 does not. + */ +AP_DECLARE(int) ap_streq_timingsafe(const char *sec1, const char *str2); + +/** + * Check whether two NUL-terminated strings have the same content, up to \c n + * characters, using a constant time algorithm (branch-less with regard to the + * content of the secret string and an execution time solely dependent on the + * length of the non-secret string or \c n). The secret string of the two + * should be set in the first parameter \c sec1 to avoid leaking its length. + * + * @param sec1 secret string to compare + * @param str2 string to compare with + * @param n max number of characters to compare + * @return 1 if equal, 0 otherwise + * @remark The function will compare as much characters as there are in + * \c str2 if it's less than \c n, so the length of \c str2 might + * leak through side channel, while the length of \c sec1 does not. + */ +AP_DECLARE(int) ap_strneq_timingsafe(const char *sec1, const char *str2, + apr_size_t n); + /** * Given a string, replace any bare " with \\" . * @param p The pool to allocate memory from diff --git a/modules/aaa/mod_auth_digest.c b/modules/aaa/mod_auth_digest.c index ac02e6b9673..fb4d26306be 100644 --- a/modules/aaa/mod_auth_digest.c +++ b/modules/aaa/mod_auth_digest.c @@ -73,7 +73,6 @@ #include "apr_shm.h" #include "apr_rmm.h" #include "ap_provider.h" -#include "apr_crypto.h" /* for apr_crypto_equals */ #include "mod_auth.h" @@ -1438,7 +1437,7 @@ static int check_nonce(request_rec *r, digest_header_rec *resp, resp->nonce[NONCE_TIME_LEN] = tmp; resp->nonce_time = nonce_time.time; - if (!apr_crypto_equals(hash, resp->nonce+NONCE_TIME_LEN, NONCE_HASH_LEN)) { + if (!ap_memeq_timingsafe(hash, resp->nonce+NONCE_TIME_LEN, NONCE_HASH_LEN)) { ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01776) "invalid nonce %s received - hash is not %s", resp->nonce, hash); @@ -1769,7 +1768,7 @@ static int authenticate_digest_user(request_rec *r) if (resp->message_qop == NULL) { /* old (rfc-2069) style digest */ - if (!apr_crypto_equals(resp->digest, old_digest(r, resp), MD5_DIGEST_LEN)) { + if (!ap_memeq_timingsafe(old_digest(r, resp), resp->digest, MD5_DIGEST_LEN)) { ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01792) "user %s: password mismatch: %s", r->user, r->uri); @@ -1804,7 +1803,7 @@ static int authenticate_digest_user(request_rec *r) /* we failed to allocate a client struct */ return HTTP_INTERNAL_SERVER_ERROR; } - if (!apr_crypto_equals(resp->digest, exp_digest, MD5_DIGEST_LEN)) { + if (!ap_memeq_timingsafe(exp_digest, resp->digest, MD5_DIGEST_LEN)) { ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01794) "user %s: password mismatch: %s", r->user, r->uri); diff --git a/modules/session/mod_session_crypto.c b/modules/session/mod_session_crypto.c index f072d4c6fbc..67b9e75464f 100644 --- a/modules/session/mod_session_crypto.c +++ b/modules/session/mod_session_crypto.c @@ -69,8 +69,6 @@ typedef struct { #define AP_SIPHASH_KSIZE APR_SIPHASH_KSIZE #define ap_siphash24_auth apr_siphash24_auth -#define ap_crypto_equals apr_crypto_equals - #else #define AP_SIPHASH_DSIZE 8 @@ -165,21 +163,6 @@ static void ap_siphash24_auth(unsigned char out[AP_SIPHASH_DSIZE], U64TO8_LE(out, h); } -static int ap_crypto_equals(const void *buf1, const void *buf2, - apr_size_t size) -{ - const unsigned char *p1 = buf1; - const unsigned char *p2 = buf2; - unsigned char diff = 0; - apr_size_t i; - - for (i = 0; i < size; ++i) { - diff |= p1[i] ^ p2[i]; - } - - return 1 & ((diff - 1) >> 8); -} - #endif static void compute_auth(const void *src, apr_size_t len, @@ -404,7 +387,7 @@ static apr_status_t decrypt_string(request_rec * r, const apr_crypto_t *f, * the MAC and comparing it (timing safe) with the one in the payload. */ compute_auth(slider, len, passphrase, passlen, auth); - if (!ap_crypto_equals(auth, decoded, AP_SIPHASH_DSIZE)) { + if (!ap_memeq_timingsafe(auth, decoded, AP_SIPHASH_DSIZE)) { ap_log_rerror(APLOG_MARK, APLOG_DEBUG, res, r, APLOGNO(10006) "auth does not match, skipping"); continue; diff --git a/server/util.c b/server/util.c index eda0eceaf9c..d1d06fc15b4 100644 --- a/server/util.c +++ b/server/util.c @@ -3934,3 +3934,141 @@ AP_DECLARE(const char *)ap_dir_fnmatch(ap_dir_match_t *w, const char *path, return NULL; } + + +#if APR_VERSION_AT_LEAST(1,8,0) +AP_DECLARE(int) ap_memeq_timingsafe(const void *buf1, const void *buf2, + apr_size_t n) +{ + return apr_memeq_timingsafe(buf1, buf2, n); +} + +AP_DECLARE(int) ap_streq_timingsafe(const char *sec1, const char *str2) +{ + return apr_streq_timingsafe(sec1, str2); +} + +AP_DECLARE(int) ap_strneq_timingsafe(const char *sec1, const char *str2, + apr_size_t n) +{ + return apr_strneq_timingsafe(sec1, str2, n); +} + +#else /* !APR_VERSION_AT_LEAST(1,8,0) */ + +/* A volatile variable which is always zero but allows to block the compiler + * from optimizing or eliding code using it. Volatile forces the compiler to + * emit a memory load for which no value can be assumed, so for instance an + * add/sub/xor/or with "optblocker" is a noop that will hide the result to + * the optimizer. + */ +static volatile const apr_uint32_t optblocker; + +/* Return whether x is not zero, with no branching controlled by x. + * + * Taken from the cryptoint library (public domain) by D. J. Bernstein, + * which provides timing attacks safe integer operations/primitives. + * Code: + * https://lib.mceliece.org/libmceliece-20250507/cryptoint/crypto_uint32.h + * Paper: + * https://cr.yp.to/papers/cryptoint-20250424.pdf + */ +#if __has_attribute(always_inline) +__attribute__((always_inline)) +#endif +static APR_INLINE int test_nonzero_timingsafe(apr_uint32_t x) +{ + x |= -x; /* sets the most significant bit unless x == 0 */ + + /* shift bit 31 (MSB) to bit 0 */ + x >>= 32-6; /* keep 6 bits */ + x += optblocker; /* lose the optimizer */ + x >>= 5; /* keep the (original) MSB only */ + + /* x is now 0 or 1 */ + return x & INT_MAX; +} + +AP_DECLARE(int) ap_memeq_timingsafe(const void *buf1, const void *buf2, + apr_size_t n) +{ + apr_uint32_t diff = 0; + volatile apr_size_t count = n; /* prevent loop unrolling */ + apr_size_t i = 0; + + for (; i < count; ++i) { + const unsigned char c1 = ((volatile const unsigned char *)buf1)[i]; + const unsigned char c2 = ((volatile const unsigned char *)buf2)[i]; + + diff |= c1 ^ c2; /* sets diff to non-zero whenever c1 != c2 */ + } + + /* (diff == 0) <=> (diff != 0) ^ 1 */ + return test_nonzero_timingsafe(diff) ^ 1; +} + +AP_DECLARE(int) ap_streq_timingsafe(const char *sec1, const char *str2) +{ + apr_uint32_t diff = 0; + apr_size_t i1 = 0, i2 = 0; + + for (;; ++i2) { + const unsigned char c1 = ((volatile const unsigned char *)sec1)[i1]; + const unsigned char c2 = ((volatile const unsigned char *)str2)[i2]; + + diff |= c1 ^ c2; /* sets diff to non-zero whenever c1 != c2 */ + + /* Not a shortest/longest match because an attacker would usually know + * one of the strings and could then determine the length of the other. + * So assume only sec1 and its length are secret and stop the loop at + * the end of str2. If sec1 is shorter than str2 the loop will continue + * by comparing the rest of str2 with the trailing NUL byte of sec1. + * In any case since the diff above is computed up to and including a + * NUL byte, only the same content and length will raise match. + */ + if (!c2) { + break; + } + + /* Don't go above sec1's NUL byte */ + i1 += test_nonzero_timingsafe(c1); + } + + /* (diff == 0) <=> (diff != 0) ^ 1 */ + return test_nonzero_timingsafe(diff) ^ 1; +} + +AP_DECLARE(int) ap_strneq_timingsafe(const char *sec1, const char *str2, + apr_size_t n) +{ + apr_uint32_t diff = 0; + volatile apr_size_t count = n; /* prevent loop unrolling */ + apr_size_t i1 = 0, i2 = 0; + + for (; i2 < count; ++i2) { + const unsigned char c1 = ((volatile const unsigned char *)sec1)[i1]; + const unsigned char c2 = ((volatile const unsigned char *)str2)[i2]; + + diff |= c1 ^ c2; /* sets diff to non-zero whenever c1 != c2 */ + + /* Not a shortest/longest match because an attacker would usually know + * one of the strings and could then determine the length of the other. + * So assume only sec1 and its length are secret and stop the loop at + * the end of str2. If sec1 is shorter than str2 the loop will continue + * by comparing the rest of str2 with the trailing NUL byte of sec1. + * In any case since the diff above is computed up to and including a + * NUL byte, only the same content and length will raise match. + */ + if (!c2) { + break; + } + + /* Don't go above sec1's NUL byte */ + i1 += test_nonzero_timingsafe(c1); + } + + /* (diff == 0) <=> (diff != 0) ^ 1 */ + return test_nonzero_timingsafe(diff) ^ 1; +} + +#endif /* !APR_VERSION_AT_LEAST(1,8,0) */