From e7e378fad116c18c78274e9b268acbd8a653268b Mon Sep 17 00:00:00 2001 From: Loup Vaillant Date: Wed, 11 Mar 2020 19:07:54 +0100 Subject: [PATCH] Secretbox: explained non-portable behavior (#936) Addresses #934 Some tools believe that comparing pointers, *even after converting them to integers*, is undefined. A comment acknowledging this (as well as the necessity of the comparison to begin with), can facilitate audits. Co-authored-by: Frank Denis <124872+jedisct1@users.noreply.github.com> --- .../crypto_secretbox/crypto_secretbox_easy.c | 14 ++++++++++++++ .../secretbox_xchacha20poly1305.c | 14 ++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/src/libsodium/crypto_secretbox/crypto_secretbox_easy.c b/src/libsodium/crypto_secretbox/crypto_secretbox_easy.c index 12132a2a..5d58adc7 100644 --- a/src/libsodium/crypto_secretbox/crypto_secretbox_easy.c +++ b/src/libsodium/crypto_secretbox/crypto_secretbox_easy.c @@ -27,6 +27,13 @@ crypto_secretbox_detached(unsigned char *c, unsigned char *mac, crypto_core_hsalsa20(subkey, n, k, NULL); + /* Allow the m and and c buffer to partially overlap, by calling + memmove() if necessary. + + Note that there is no fully portable way to compare pointers. + Some tools even report undefined behavior, despite the conversion. + Nevertheless, this works on all supported platforms. + */ if (((uintptr_t) c > (uintptr_t) m && (uintptr_t) c - (uintptr_t) m < mlen) || ((uintptr_t) m > (uintptr_t) c && @@ -101,6 +108,13 @@ crypto_secretbox_open_detached(unsigned char *m, const unsigned char *c, if (m == NULL) { return 0; } + /* Allow the m and and c buffer to partially overlap, by calling + memmove() if necessary. + + Note that there is no fully portable way to compare pointers. + Some tools even report undefined behavior, despite the conversion. + Nevertheless, this works on all supported platforms. + */ if (((uintptr_t) c > (uintptr_t) m && (uintptr_t) c - (uintptr_t) m < clen) || ((uintptr_t) m > (uintptr_t) c && diff --git a/src/libsodium/crypto_secretbox/xchacha20poly1305/secretbox_xchacha20poly1305.c b/src/libsodium/crypto_secretbox/xchacha20poly1305/secretbox_xchacha20poly1305.c index d012b13b..3459ee5b 100644 --- a/src/libsodium/crypto_secretbox/xchacha20poly1305/secretbox_xchacha20poly1305.c +++ b/src/libsodium/crypto_secretbox/xchacha20poly1305/secretbox_xchacha20poly1305.c @@ -31,6 +31,13 @@ crypto_secretbox_xchacha20poly1305_detached(unsigned char *c, crypto_core_hchacha20(subkey, n, k, NULL); + /* Allow the m and and c buffer to partially overlap, by calling + memmove() if necessary. + + Note that there is no fully portable way to compare pointers. + Some tools even report undefined behavior, despite the conversion. + Nevertheless, this works on all supported platforms. + */ if (((uintptr_t) c > (uintptr_t) m && (uintptr_t) c - (uintptr_t) m < mlen) || ((uintptr_t) m > (uintptr_t) c && @@ -108,6 +115,13 @@ crypto_secretbox_xchacha20poly1305_open_detached(unsigned char *m, if (m == NULL) { return 0; } + /* Allow the m and and c buffer to partially overlap, by calling + memmove() if necessary. + + Note that there is no fully portable way to compare pointers. + Some tools even report undefined behavior, despite the conversion. + Nevertheless, this works on all supported platforms. + */ if (((uintptr_t) c > (uintptr_t) m && (uintptr_t) c - (uintptr_t) m < clen) || ((uintptr_t) m > (uintptr_t) c &&