Mailing List Archive

[PATCH] Use EVP_MAC interface for Poly1305 if supported.
We've been working on improving poly1305 performance and I came across
the EVP_MAC interface in OpenSSL 3.0 today. I tried swapping it in place
of the poly1305_auth routine and my performance increased notably. On a
Ryzen 7 5800X test to localhost I went from about 1000MB/s to 1450MB/s.
On a more realistic test between two AMD Epyc servers connected via 10G
I went from an average of 604.9MB/s to 718.7MB/s. Another testbed also
got me about a 16 to 17% improvement in throughput.

I normally wouldn't clutter up the code with library version specific
ifdefs but it might be worth considering. This is a first pass so if
anyone sees any glaring problems please let me know.

diff --git a/cipher-chachapoly-libcrypto.c b/cipher-chachapoly-libcrypto.c
index 719f9c843..b2a148696 100644
--- a/cipher-chachapoly-libcrypto.c
+++ b/cipher-chachapoly-libcrypto.c
@@ -90,6 +90,15 @@ chachapoly_crypt(struct chachapoly_ctx *ctx, u_int
seqnr, u_char *dest,
int r = SSH_ERR_INTERNAL_ERROR;
u_char expected_tag[POLY1305_TAGLEN], poly_key[POLY1305_KEYLEN];

+ /* 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 */
+#if OPENSSL_VERSION_NUMBER >= 0x30000000UL
+ EVP_MAC_CTX *poly_ctx = NULL;
+ EVP_MAC *mac = NULL;
+ size_t poly_out_len;
+#endif
/*
* Run ChaCha20 once to generate the Poly1305 key. The IV is the
* packet sequence number.
@@ -104,11 +113,27 @@ chachapoly_crypt(struct chachapoly_ctx *ctx, u_int
seqnr, u_char *dest,
goto out;
}

+#if OPENSSL_VERSION_NUMBER >= 0x30000000UL
+ /* fetch the mac and create and initialize the context */
+ if ((mac = EVP_MAC_fetch(NULL, "POLY1305", NULL)) == NULL ||
+ (poly_ctx = EVP_MAC_CTX_new(mac)) == NULL ||
+ !EVP_MAC_init(poly_ctx, (const u_char *)poly_key,
POLY1305_KEYLEN, NULL)) {
+ r = SSH_ERR_LIBCRYPTO_ERROR;
+ goto out;
+ }
+#endif
+
/* If decrypting, check tag before anything else */
if (!do_encrypt) {
const u_char *tag = src + aadlen + len;
-
+#if OPENSSL_VERSION_NUMBER >= 0x30000000UL
+ /* EVP_MAC_update doesn't put the poly_mac into a buffer
+ * we need EVP_MAC_final for that */
+ EVP_MAC_update(poly_ctx, src, aadlen + len);
+ EVP_MAC_final(poly_ctx, expected_tag, &poly_out_len,
(size_t)POLY1305_TAGLEN);
+#else
poly1305_auth(expected_tag, src, aadlen + len, poly_key);
+#endif
if (timingsafe_bcmp(expected_tag, tag, POLY1305_TAGLEN)
!= 0) {
r = SSH_ERR_MAC_INVALID;
goto out;
@@ -134,8 +159,13 @@ 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);
+#if OPENSSL_VERSION_NUMBER >= 0x30000000UL
+ EVP_MAC_update(poly_ctx, dest, aadlen + len);
+ EVP_MAC_final(poly_ctx, dest + aadlen + len, &poly_out_len,
(size_t)POLY1305_TAGLEN);
+#else
+ poly1305_auth(dest + aadlen + len, dest, aadlen + len,
+ poly_key);
+#endif
}
r = 0;
out:
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
Re: [PATCH] Use EVP_MAC interface for Poly1305 if supported. [ In reply to ]
On Sat, 22 Oct 2022 at 07:53, Chris Rapier <rapier@psc.edu> wrote:
[...]
> I normally wouldn't clutter up the code with library version specific
> ifdefs but it might be worth considering.

Instead of ifdefs, you can check if the MAC init succeeded before
calling the EVP functions, else fall back to the existing code path.

> + /* fetch the mac and create and initialize the context */
> + if ((mac = EVP_MAC_fetch(NULL, "POLY1305", NULL)) == NULL ||
> + (poly_ctx = EVP_MAC_CTX_new(mac)) == NULL ||

You're initializing the MAC context on every call to this function.
If you initialize the context once, cache it (say, as a static) and
reuse it does it go any faster?

[...]
> +#if OPENSSL_VERSION_NUMBER >= 0x30000000UL
> + /* EVP_MAC_update doesn't put the poly_mac into a buffer
> + * we need EVP_MAC_final for that */
> + EVP_MAC_update(poly_ctx, src, aadlen + len);
> + EVP_MAC_final(poly_ctx, expected_tag, &poly_out_len, (size_t)POLY1305_TAGLEN);
> +#else
> poly1305_auth(expected_tag, src, aadlen + len, poly_key);
> +#endif

You'd also want to only try to init the context once instead of every
time in the case where libcrypto did not support it, so something
like:

if (ctx_inited && poly_ctx != NULL) {
EVP_MAC_update(poly_ctx, src, aadlen + len);
EVP_MAC_final(poly_ctx, expected_tag, &poly_out_len,
(size_t)POLY1305_TAGLEN);
} else {
poly1305_auth(expected_tag, src, aadlen + len, poly_key);
}

--
Darren Tucker (dtucker at dtucker.net)
GPG key 11EAA6FA / A86E 3E07 5B19 5880 E860 37F4 9357 ECEF 11EA A6FA (new)
Good judgement comes with experience. Unfortunately, the experience
usually comes from bad judgement.
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
Re: [PATCH] Use EVP_MAC interface for Poly1305 if supported. [ In reply to ]
On Sun, Oct 23, 2022 at 12:51 AM Darren Tucker <dtucker@dtucker.net> wrote:

> On Sat, 22 Oct 2022 at 07:53, Chris Rapier <rapier@psc.edu> wrote:
> [...]
> > I normally wouldn't clutter up the code with library version specific
> > ifdefs but it might be worth considering.
>
> Instead of ifdefs, you can check if the MAC init succeeded before
> calling the EVP functions, else fall back to the existing code path.
>

EVP_MAC_fetch is a new OpenSSL 3.0 API, so to follow this way it will be
necessary to detect it via configure.


--
Dmitry Belyavskiy
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
Re: [PATCH] Use EVP_MAC interface for Poly1305 if supported. [ In reply to ]
On 10/22/22 6:49 PM, Darren Tucker wrote:
> On Sat, 22 Oct 2022 at 07:53, Chris Rapier <rapier@psc.edu> wrote:
> [...]
>> I normally wouldn't clutter up the code with library version specific
>> ifdefs but it might be worth considering.
>
> Instead of ifdefs, you can check if the MAC init succeeded before
> calling the EVP functions, else fall back to the existing code path.

As pointed out, this is only in OSSL3. That said, for hpnssh we've been
looking at extracting the necessary code/assembly from OSSL3 and
incorporating it into our code base to provide this functionality.
Maybe. Depends on the complexity of the task.
>> + /* fetch the mac and create and initialize the context */
>> + if ((mac = EVP_MAC_fetch(NULL, "POLY1305", NULL)) == NULL ||
>> + (poly_ctx = EVP_MAC_CTX_new(mac)) == NULL ||
>
> You're initializing the MAC context on every call to this function.
> If you initialize the context once, cache it (say, as a static) and
> reuse it does it go any faster?

That's a fine question and one I hope to explore today. I also noticed
that I'm neglecting to free the the EVP_MAC and the EVP_MAC_CTX. Kind of
jumped the gun on that.

Chris
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
Re: [PATCH] Use EVP_MAC interface for Poly1305 if supported. [ In reply to ]
New version of the patch. This time the MAC ctx is part of the
chachapoly_ctx. This improved performance even more. The testbed are 2
AMD Epyc 7502Ps connected via 10Gb through a local switch. The test is
"./hpnssh remote "dd if=/dev/zero bs=1M count=20000" > /dev/null". The
results are an average of 10 runs. Baseline is 604.9MB/s and the EVP
version of poly1305 hit 733.5 MB/s. So that's just a bit more than 21%
improvement in throughput.

Please note that these results are for hpnssh and not mainline OpenSSH.
The results for OpenSSH (9.0p1) are actually a lot more dramatic.
Baseline on the same testbed is 297MB/s. The EVP version clocked in at
637MB/s. I tested compatibility against other versions of OpenSSH and it
does work. I feel like I must be doing something wrong but if I am it's
not obvious to me.

Chris

diff --git a/cipher-chachapoly-libcrypto.c b/cipher-chachapoly-libcrypto.c
index 719f9c843..199f2974e 100644
--- a/cipher-chachapoly-libcrypto.c
+++ b/cipher-chachapoly-libcrypto.c
@@ -37,12 +37,16 @@

struct chachapoly_ctx {
EVP_CIPHER_CTX *main_evp, *header_evp;
+#if OPENSSL_VERSION_NUMBER >= 0x30000000UL
+ EVP_MAC_CTX *poly_ctx;
+#endif
};

struct chachapoly_ctx *
chachapoly_new(const u_char *key, u_int keylen)
{
struct chachapoly_ctx *ctx;
+ EVP_MAC *mac = NULL;

if (keylen != (32 + 32)) /* 2 x 256 bit keys */
return NULL;
@@ -57,6 +61,12 @@ chachapoly_new(const u_char *key, u_int keylen)
goto fail;
if (EVP_CIPHER_CTX_iv_length(ctx->header_evp) != 16)
goto fail;
+#if OPENSSL_VERSION_NUMBER >= 0x30000000UL
+ if ((mac = EVP_MAC_fetch(NULL, "POLY1305", NULL)) == NULL)
+ goto fail;
+ if ((ctx->poly_ctx = EVP_MAC_CTX_new(mac)) == NULL)
+ goto fail;
+#endif
return ctx;
fail:
chachapoly_free(ctx);
@@ -70,6 +80,9 @@ chachapoly_free(struct chachapoly_ctx *cpctx)
return;
EVP_CIPHER_CTX_free(cpctx->main_evp);
EVP_CIPHER_CTX_free(cpctx->header_evp);
+#if OPENSSL_VERSION_NUMBER >= 0x30000000UL
+ EVP_MAC_CTX_free(cpctx->poly_ctx);
+#endif
freezero(cpctx, sizeof(*cpctx));
}

@@ -90,6 +103,13 @@ chachapoly_crypt(struct chachapoly_ctx *ctx, u_int
seqnr, u_char *dest,
int r = SSH_ERR_INTERNAL_ERROR;
u_char expected_tag[POLY1305_TAGLEN], poly_key[POLY1305_KEYLEN];

+ /* 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 */
+#if OPENSSL_VERSION_NUMBER >= 0x30000000UL
+ size_t poly_out_len;
+#endif
/*
* Run ChaCha20 once to generate the Poly1305 key. The IV is the
* packet sequence number.
@@ -104,11 +124,25 @@ chachapoly_crypt(struct chachapoly_ctx *ctx, u_int
seqnr, u_char *dest,
goto out;
}

+#if OPENSSL_VERSION_NUMBER >= 0x30000000UL
+ /* init the MAC each time to get the new key */
+ if(!EVP_MAC_init(ctx->poly_ctx, (const u_char *)poly_key,
POLY1305_KEYLEN, NULL)) {
+ r = SSH_ERR_LIBCRYPTO_ERROR;
+ goto out;
+ }
+#endif
+
/* If decrypting, check tag before anything else */
if (!do_encrypt) {
const u_char *tag = src + aadlen + len;
-
+#if OPENSSL_VERSION_NUMBER >= 0x30000000UL
+ /* EVP_MAC_update doesn't put the poly_mac into a buffer
+ * we need EVP_MAC_final for that */
+ EVP_MAC_update(ctx->poly_ctx, src, aadlen + len);
+ EVP_MAC_final(ctx->poly_ctx, expected_tag,
&poly_out_len, (size_t)POLY1305_TAGLEN);
+#else
poly1305_auth(expected_tag, src, aadlen + len, poly_key);
+#endif
if (timingsafe_bcmp(expected_tag, tag, POLY1305_TAGLEN)
!= 0) {
r = SSH_ERR_MAC_INVALID;
goto out;
@@ -134,8 +168,13 @@ 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);
+#if OPENSSL_VERSION_NUMBER >= 0x30000000UL
+ EVP_MAC_update(ctx->poly_ctx, dest, aadlen + len);
+ EVP_MAC_final(ctx->poly_ctx, dest + aadlen + len,
&poly_out_len, (size_t)POLY1305_TAGLEN);
+#else
+ poly1305_auth(dest + aadlen + len, dest, aadlen + len,
+ poly_key);
+#endif
}
r = 0;
out:

On 10/24/22 11:53 AM, Chris Rapier wrote:
> On 10/22/22 6:49 PM, Darren Tucker wrote:
>> On Sat, 22 Oct 2022 at 07:53, Chris Rapier <rapier@psc.edu> wrote:
>> [...]
>>> I normally wouldn't clutter up the code with library version specific
>>> ifdefs but it might be worth considering.
>>
>> Instead of ifdefs, you can check if the MAC init succeeded before
>> calling the EVP functions, else fall back to the existing code path.
>
> As pointed out, this is only in OSSL3. That said, for hpnssh we've been
> looking at extracting the necessary code/assembly from OSSL3 and
> incorporating it into our code base to provide this functionality.
> Maybe. Depends on the complexity of the task.
>>> +       /* fetch the mac and create and initialize the context */
>>> +       if ((mac = EVP_MAC_fetch(NULL, "POLY1305", NULL)) == NULL ||
>>> +           (poly_ctx = EVP_MAC_CTX_new(mac)) == NULL ||
>>
>> You're initializing the MAC context on every call to this function.
>> If you initialize the context once, cache it (say, as a static) and
>> reuse it does it go any faster?
>
> That's a fine question and one I hope to explore today. I also noticed
> that I'm neglecting to free the the EVP_MAC and the EVP_MAC_CTX. Kind of
> jumped the gun on that.
>
> Chris
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
Re: [PATCH] Use EVP_MAC interface for Poly1305 if supported. [ In reply to ]
On Tue, 25 Oct 2022 at 06:23, Chris Rapier <rapier@psc.edu> wrote:
[...]
> I tested compatibility against other versions of OpenSSH and it
> does work. I feel like I must be doing something wrong but if I am it's
> not obvious to me.

For changes where there are you have questions about interop,
installing PuTTY and running the relevant regress tests ('make t-exec
LTESTS="putty-transfer putty-ciphers putty-kex"') will give you some
reassurance that the changes interop OK (as long as PuTTY implements
the thing you have the question about, which in this case it does).

--
Darren Tucker (dtucker at dtucker.net)
GPG key 11EAA6FA / A86E 3E07 5B19 5880 E860 37F4 9357 ECEF 11EA A6FA (new)
Good judgement comes with experience. Unfortunately, the experience
usually comes from bad judgement.
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
Re: [PATCH] Use EVP_MAC interface for Poly1305 if supported. [ In reply to ]
On Tue, 25 Oct 2022 at 06:23, Chris Rapier <rapier@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.

> + 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.


--
Darren Tucker (dtucker at dtucker.net)
GPG key 11EAA6FA / A86E 3E07 5B19 5880 E860 37F4 9357 ECEF 11EA A6FA (new)
Good judgement comes with experience. Unfortunately, the experience
usually comes from bad judgement.
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
Re: [PATCH] Use EVP_MAC interface for Poly1305 if supported. [ In reply to ]
On 10/24/22 4:23 PM, Darren Tucker wrote:
> On Tue, 25 Oct 2022 at 06:23, Chris Rapier <rapier@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
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
Re: [PATCH] Use EVP_MAC interface for Poly1305 if supported. [ In reply to ]
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@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
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
Re: [PATCH] Use EVP_MAC interface for Poly1305 if supported. [ In reply to ]
Ignore this patch. I forgot to update cipher_chachapoly.c to deal with
the new definition of poly1305_auth. Sorry about that.

On 10/25/22 2:20 PM, Chris Rapier wrote:
> 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@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
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev