Chris Rapier
2022-Oct-24 21:08 UTC
[PATCH] Use EVP_MAC interface for Poly1305 if supported.
On 10/24/22 4:23 PM, Darren Tucker wrote:> On Tue, 25 Oct 2022 at 06:23, Chris Rapier <rapier at psc.edu> wrote: > >> +#if OPENSSL_VERSION_NUMBER >= 0x30000000UL > > As mentioned by Dmitry Belyavskiy upthread, since this depends on > EVP_MAC_fetch() this should probably be checked by configure instead > and put inside an ifdef HAVE_EVP_MAC_FETCH. I'm also wondering if the > additional OpenSSL specific code belongs in the poly1305_auth function > in cipher-chachapoly-libcrypto.c.Okay, I think I'm understanding. We'd still have the #ifdefs in the code but we'd be moving away from actually checking a specific version string to seeing if the function itself exists. I'll work on that tomorrow. As for putting it in the poly_auth function itself. I'm concerned that making the parameters work would be difficult and possible confusing if we maintained the current ones for poly1305_auth(). As far as I can figure we'd need 5 parameters and to set ctx->poly_ctx explictly to null in is HAVE_EVP_MAC_FETCH is false. Thoughts?>> + size_t poly_out_len; >> +#endif > > Since poly_out_len is only ever used inside the "if (!do_encrypt)" > block below, you could move this declaration inside the existing ifdef > inside that block and reduce this diff by one hunk.Good point. I've made that change. I'm going to think about a few more things and work out the configure before I submit a new patch. Chris
Chris Rapier
2022-Oct-25 18:20 UTC
[PATCH] Use EVP_MAC interface for Poly1305 if supported.
Newest version of the patch. This includes creating a
OPENSSL_HAVE_EVP_MAC test in configure and moving the EVP_MAC_functions
to poly1305.c. There are still 3 ifdefs in cipher-chachapoly_libcrypto.c
but none in chachapoly_crypt(). I did have to extend the parameters on
poly1305_auth to include the poly evp context.
diff --git a/cipher-chachapoly-libcrypto.c b/cipher-chachapoly-libcrypto.c
index 719f9c843..52decd427 100644
--- a/cipher-chachapoly-libcrypto.c
+++ b/cipher-chachapoly-libcrypto.c
@@ -35,8 +35,18 @@
#include "ssherr.h"
#include "cipher-chachapoly.h"
+
+/* using the EVP_MAC interface for poly1305 is significantly
+ * faster than the version bundled with OpenSSH. However,
+ * this interface is only available in OpenSSL 3.0+
+ * -cjr 10/21/2022 */
struct chachapoly_ctx {
EVP_CIPHER_CTX *main_evp, *header_evp;
+#ifdef OPENSSL_HAVE_EVP_MAC
+ EVP_MAC_CTX *poly_ctx;
+#else
+ char *poly_ctx;
+#endif
};
struct chachapoly_ctx *
@@ -57,6 +67,15 @@ chachapoly_new(const u_char *key, u_int keylen)
goto fail;
if (EVP_CIPHER_CTX_iv_length(ctx->header_evp) != 16)
goto fail;
+#ifdef OPENSSL_HAVE_EVP_MAC
+ EVP_MAC *mac = NULL;
+ if ((mac = EVP_MAC_fetch(NULL, "POLY1305", NULL)) == NULL)
+ goto fail;
+ if ((ctx->poly_ctx = EVP_MAC_CTX_new(mac)) == NULL)
+ goto fail;
+#else
+ ctx->poly_ctx = NULL;
+#endif
return ctx;
fail:
chachapoly_free(ctx);
@@ -70,6 +89,9 @@ chachapoly_free(struct chachapoly_ctx *cpctx)
return;
EVP_CIPHER_CTX_free(cpctx->main_evp);
EVP_CIPHER_CTX_free(cpctx->header_evp);
+#ifdef OPENSSL_HAVE_EVP_MAC
+ EVP_MAC_CTX_free(cpctx->poly_ctx);
+#endif
freezero(cpctx, sizeof(*cpctx));
}
@@ -107,8 +129,7 @@ chachapoly_crypt(struct chachapoly_ctx *ctx, u_int
seqnr, u_char *dest,
/* If decrypting, check tag before anything else */
if (!do_encrypt) {
const u_char *tag = src + aadlen + len;
-
- poly1305_auth(expected_tag, src, aadlen + len, poly_key);
+ poly1305_auth(ctx->poly_ctx, expected_tag, src, aadlen +
len, poly_key);
if (timingsafe_bcmp(expected_tag, tag, POLY1305_TAGLEN)
!= 0) {
r = SSH_ERR_MAC_INVALID;
goto out;
@@ -134,8 +155,8 @@ chachapoly_crypt(struct chachapoly_ctx *ctx, u_int
seqnr, u_char *dest,
/* If encrypting, calculate and append tag */
if (do_encrypt) {
- poly1305_auth(dest + aadlen + len, dest, aadlen + len,
- poly_key);
+ poly1305_auth(ctx->poly_ctx, dest + aadlen + len, dest, aadlen
+ len,
+ poly_key);
}
r = 0;
out:
diff --git a/configure.ac b/configure.ac
index 8a18f8381..8493f4bc3 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2932,6 +2932,11 @@ if test "x$openssl" = "xyes" ; then
EVP_chacha20 \
])
+ # OpenSSL 3.0 API
+ # Does OpenSSL support the EVP_MAC functions?
+ AC_CHECK_FUNCS(EVP_MAC_fetch,
+ [AC_DEFINE([OPENSSL_HAVE_EVP_MAC], [1], [EVP_MAC
Functions])])
+
if test "x$openssl_engine" = "xyes" ; then
AC_MSG_CHECKING([for OpenSSL ENGINE support])
AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[
diff --git a/poly1305.c b/poly1305.c
index 6fd1fc8cd..edc2002eb 100644
--- a/poly1305.c
+++ b/poly1305.c
@@ -14,6 +14,16 @@
#include "poly1305.h"
+#ifdef OPENSSL_HAVE_EVP_MAC
+void
+poly1305_auth(EVP_MAC_CTX *poly_ctx, unsigned char
out[POLY1305_TAGLEN], const unsigned char *m, size_t inlen, const
unsigned char key[POLY1305_KEYLEN]) {
+ size_t poly_out_len;
+ EVP_MAC_init(poly_ctx, (const u_char *)key, POLY1305_KEYLEN, NULL);
+ EVP_MAC_update(poly_ctx, m, inlen);
+ EVP_MAC_final(poly_ctx, out, &poly_out_len,
(size_t)POLY1305_TAGLEN);
+}
+#else
+
#define mul32x32_64(a,b) ((uint64_t)(a) * (b))
#define U8TO32_LE(p) \
@@ -31,7 +41,7 @@
} while (0)
void
-poly1305_auth(unsigned char out[POLY1305_TAGLEN], const unsigned char
*m, size_t inlen, const unsigned char key[POLY1305_KEYLEN]) {
+poly1305_auth(char *unused, unsigned char out[POLY1305_TAGLEN], const
unsigned char *m, size_t inlen, const unsigned char key[POLY1305_KEYLEN]) {
uint32_t t0,t1,t2,t3;
uint32_t h0,h1,h2,h3,h4;
uint32_t r0,r1,r2,r3,r4;
@@ -158,3 +168,4 @@ poly1305_donna_finish:
U32TO8_LE(&out[ 8], f2); f3 += (f2 >> 32);
U32TO8_LE(&out[12], f3);
}
+#endif /* OPENSSL_HAVE_EVP_MAC */
diff --git a/poly1305.h b/poly1305.h
index f7db5f8d7..b4146f92d 100644
--- a/poly1305.h
+++ b/poly1305.h
@@ -13,8 +13,15 @@
#define POLY1305_KEYLEN 32
#define POLY1305_TAGLEN 16
-void poly1305_auth(u_char out[POLY1305_TAGLEN], const u_char *m, size_t
inlen,
+#ifdef OPENSSL_HAVE_EVP_MAC
+#include <openssl/evp.h>
+
+void poly1305_auth(EVP_MAC_CTX *poly_key, u_char out[POLY1305_TAGLEN],
const u_char *m, size_t inlen,
+ const u_char key[POLY1305_KEYLEN])
+#else
+void poly1305_auth(char *unused, u_char out[POLY1305_TAGLEN], const
u_char *m, size_t inlen,
const u_char key[POLY1305_KEYLEN])
+#endif
__attribute__((__bounded__(__minbytes__, 1, POLY1305_TAGLEN)))
__attribute__((__bounded__(__buffer__, 2, 3)))
__attribute__((__bounded__(__minbytes__, 4, POLY1305_KEYLEN)));
On 10/24/22 5:08 PM, Chris Rapier wrote:>
>
> On 10/24/22 4:23 PM, Darren Tucker wrote:
>> On Tue, 25 Oct 2022 at 06:23, Chris Rapier <rapier at psc.edu>
wrote:
>>
>>> +#if OPENSSL_VERSION_NUMBER >= 0x30000000UL
>>
>> As mentioned by Dmitry Belyavskiy upthread, since this depends on
>> EVP_MAC_fetch() this should probably be checked by configure instead
>> and put inside an ifdef HAVE_EVP_MAC_FETCH.? I'm also wondering if
the
>> additional OpenSSL specific code belongs in the poly1305_auth function
>> in cipher-chachapoly-libcrypto.c.
>
> Okay, I think I'm understanding. We'd still have the #ifdefs in the
code
> but we'd be moving away from actually checking a specific version
string
> to seeing if the function itself exists. I'll work on that tomorrow.
>
> As for putting it in the poly_auth function itself. I'm concerned that
> making the parameters work would be difficult and possible confusing if
> we maintained the current ones for poly1305_auth(). As far as I can
> figure we'd need 5 parameters and to set ctx->poly_ctx explictly to
null
> in is HAVE_EVP_MAC_FETCH is false. Thoughts?
>
>>> +?????? size_t poly_out_len;
>>> +#endif
>>
>> Since poly_out_len is only ever used inside the? "if
(!do_encrypt)"
>> block below, you could move this declaration inside the existing ifdef
>> inside that block and reduce this diff by one hunk.
>
> Good point. I've made that change. I'm going to think about a few
more
> things and work out the configure before I submit a new patch.
>
> Chris