* [PATCH] crypt: Get rid of alloca usage in __sha256_crypt_r
@ 2023-09-18 20:26 Joe Simmons-Talbott
2023-09-19 16:18 ` Adhemerval Zanella Netto
0 siblings, 1 reply; 4+ messages in thread
From: Joe Simmons-Talbott @ 2023-09-18 20:26 UTC (permalink / raw)
To: libc-alpha; +Cc: Joe Simmons-Talbott
Replace alloca usage with scratch_buffers to avoid potential stack
overflow.
---
crypt/sha256-crypt.c | 65 ++++++++++++++++++++++++++++----------------
1 file changed, 42 insertions(+), 23 deletions(-)
diff --git a/crypt/sha256-crypt.c b/crypt/sha256-crypt.c
index e90eb590bb..d686c67cd5 100644
--- a/crypt/sha256-crypt.c
+++ b/crypt/sha256-crypt.c
@@ -18,6 +18,7 @@
#include <assert.h>
#include <errno.h>
+#include <scratch_buffer.h>
#include <stdbool.h>
#include <stdlib.h>
#include <string.h>
@@ -118,6 +119,14 @@ __sha256_crypt_r (const char *key, const char *salt, char *buffer, int buflen)
size_t alloca_used = 0;
char *free_key = NULL;
char *free_pbytes = NULL;
+ struct scratch_buffer sbuf;
+ scratch_buffer_init (&sbuf);
+ struct scratch_buffer p_bytes_buf;
+ scratch_buffer_init (&p_bytes_buf);
+ struct scratch_buffer salt_buf;
+ scratch_buffer_init (&salt_buf);
+ struct scratch_buffer s_bytes_buf;
+ scratch_buffer_init (&s_bytes_buf);
/* Find beginning of salt string. The prefix should normally always
be present. Just in case it is not. */
@@ -146,14 +155,11 @@ __sha256_crypt_r (const char *key, const char *salt, char *buffer, int buflen)
{
char *tmp;
- if (__libc_use_alloca (alloca_used + key_len + __alignof__ (uint32_t)))
- tmp = alloca_account (key_len + __alignof__ (uint32_t), alloca_used);
- else
- {
- free_key = tmp = (char *) malloc (key_len + __alignof__ (uint32_t));
- if (tmp == NULL)
- return NULL;
- }
+ if (!scratch_buffer_set_array_size (
+ &sbuf, 1, key_len + __alignof__ (uint32_t)))
+ return NULL;
+
+ tmp = sbuf.data;
key = copied_key =
memcpy (tmp + __alignof__ (uint32_t)
@@ -164,8 +170,14 @@ __sha256_crypt_r (const char *key, const char *salt, char *buffer, int buflen)
if (((uintptr_t) salt) % __alignof__ (uint32_t) != 0)
{
- char *tmp = (char *) alloca (salt_len + __alignof__ (uint32_t));
- alloca_used += salt_len + __alignof__ (uint32_t);
+ char *tmp;
+ if (!scratch_buffer_set_array_size (
+ &salt_buf, 1, salt_len + __alignof__ (uint32_t)))
+ {
+ scratch_buffer_free (&sbuf);
+ return NULL;
+ }
+ tmp = salt_buf.data;
salt = copied_salt =
memcpy (tmp + __alignof__ (uint32_t)
- ((uintptr_t) tmp) % __alignof__ (uint32_t),
@@ -178,7 +190,8 @@ __sha256_crypt_r (const char *key, const char *salt, char *buffer, int buflen)
NSSLOWInitContext *nss_ictx = NSSLOW_Init ();
if (nss_ictx == NULL)
{
- free (free_key);
+ scratch_buffer_free (&sbuf);
+ scratch_buffer_free (&salt_buf);
return NULL;
}
NSSLOWHASHContext *nss_ctx = NULL;
@@ -243,17 +256,13 @@ __sha256_crypt_r (const char *key, const char *salt, char *buffer, int buflen)
sha256_finish_ctx (&alt_ctx, nss_alt_ctx, temp_result);
/* Create byte sequence P. */
- if (__libc_use_alloca (alloca_used + key_len))
- cp = p_bytes = (char *) alloca (key_len);
- else
+ if (!scratch_buffer_set_array_size (&p_bytes_buf, 1, key_len))
{
- free_pbytes = cp = p_bytes = (char *)malloc (key_len);
- if (free_pbytes == NULL)
- {
- free (free_key);
- return NULL;
- }
+ scratch_buffer_free (&sbuf);
+ scratch_buffer_free (&salt_buf);
+ return NULL;
}
+ cp = p_bytes = p_bytes_buf.data;
for (cnt = key_len; cnt >= 32; cnt -= 32)
cp = mempcpy (cp, temp_result, 32);
@@ -270,7 +279,15 @@ __sha256_crypt_r (const char *key, const char *salt, char *buffer, int buflen)
sha256_finish_ctx (&alt_ctx, nss_alt_ctx, temp_result);
/* Create byte sequence S. */
- cp = s_bytes = alloca (salt_len);
+ if (!scratch_buffer_set_array_size (&s_bytes_buf, 1, salt_len))
+ {
+ scratch_buffer_free (&sbuf);
+ scratch_buffer_free (&p_bytes_buf);
+ scratch_buffer_free (&salt_buf);
+ return NULL;
+ }
+ cp = s_bytes = s_bytes_buf.data;
+
for (cnt = salt_len; cnt >= 32; cnt -= 32)
cp = mempcpy (cp, temp_result, 32);
memcpy (cp, temp_result, cnt);
@@ -381,8 +398,10 @@ __sha256_crypt_r (const char *key, const char *salt, char *buffer, int buflen)
if (copied_salt != NULL)
explicit_bzero (copied_salt, salt_len);
- free (free_key);
- free (free_pbytes);
+ scratch_buffer_free (&sbuf);
+ scratch_buffer_free (&p_bytes_buf);
+ scratch_buffer_free (&salt_buf);
+ scratch_buffer_free (&s_bytes_buf);
return buffer;
}
--
2.39.2
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] crypt: Get rid of alloca usage in __sha256_crypt_r
2023-09-18 20:26 [PATCH] crypt: Get rid of alloca usage in __sha256_crypt_r Joe Simmons-Talbott
@ 2023-09-19 16:18 ` Adhemerval Zanella Netto
2023-09-20 15:23 ` Zack Weinberg
0 siblings, 1 reply; 4+ messages in thread
From: Adhemerval Zanella Netto @ 2023-09-19 16:18 UTC (permalink / raw)
To: Joe Simmons-Talbott, libc-alpha, Zack Weinberg
On 18/09/23 17:26, Joe Simmons-Talbott wrote:
> Replace alloca usage with scratch_buffers to avoid potential stack
> overflow.
Zack,
Would it possible to import libxcrypt code for sha256/sha512/md5 that
already does not user alloca or malloc? I think it would be better to
just touch a code that is already deprecated and meant to be used as
fallback only.
As a side-note, do we still need the USE_NSS/fips support on this now
that is not built by default?
> ---
> crypt/sha256-crypt.c | 65 ++++++++++++++++++++++++++++----------------
> 1 file changed, 42 insertions(+), 23 deletions(-)
>
> diff --git a/crypt/sha256-crypt.c b/crypt/sha256-crypt.c
> index e90eb590bb..d686c67cd5 100644
> --- a/crypt/sha256-crypt.c
> +++ b/crypt/sha256-crypt.c
> @@ -18,6 +18,7 @@
>
> #include <assert.h>
> #include <errno.h>
> +#include <scratch_buffer.h>
> #include <stdbool.h>
> #include <stdlib.h>
> #include <string.h>
> @@ -118,6 +119,14 @@ __sha256_crypt_r (const char *key, const char *salt, char *buffer, int buflen)
> size_t alloca_used = 0;
> char *free_key = NULL;
> char *free_pbytes = NULL;
> + struct scratch_buffer sbuf;
> + scratch_buffer_init (&sbuf);
> + struct scratch_buffer p_bytes_buf;
> + scratch_buffer_init (&p_bytes_buf);
> + struct scratch_buffer salt_buf;
> + scratch_buffer_init (&salt_buf);
> + struct scratch_buffer s_bytes_buf;
> + scratch_buffer_init (&s_bytes_buf);
>
> /* Find beginning of salt string. The prefix should normally always
> be present. Just in case it is not. */
> @@ -146,14 +155,11 @@ __sha256_crypt_r (const char *key, const char *salt, char *buffer, int buflen)
> {
> char *tmp;
>
> - if (__libc_use_alloca (alloca_used + key_len + __alignof__ (uint32_t)))
> - tmp = alloca_account (key_len + __alignof__ (uint32_t), alloca_used);
> - else
> - {
> - free_key = tmp = (char *) malloc (key_len + __alignof__ (uint32_t));
> - if (tmp == NULL)
> - return NULL;
> - }
> + if (!scratch_buffer_set_array_size (
> + &sbuf, 1, key_len + __alignof__ (uint32_t)))
> + return NULL;
> +
> + tmp = sbuf.data;
>
> key = copied_key =
> memcpy (tmp + __alignof__ (uint32_t)
> @@ -164,8 +170,14 @@ __sha256_crypt_r (const char *key, const char *salt, char *buffer, int buflen)
>
> if (((uintptr_t) salt) % __alignof__ (uint32_t) != 0)
> {
> - char *tmp = (char *) alloca (salt_len + __alignof__ (uint32_t));
> - alloca_used += salt_len + __alignof__ (uint32_t);
> + char *tmp;
> + if (!scratch_buffer_set_array_size (
> + &salt_buf, 1, salt_len + __alignof__ (uint32_t)))
> + {
> + scratch_buffer_free (&sbuf);
> + return NULL;
> + }
> + tmp = salt_buf.data;
> salt = copied_salt =
> memcpy (tmp + __alignof__ (uint32_t)
> - ((uintptr_t) tmp) % __alignof__ (uint32_t),
> @@ -178,7 +190,8 @@ __sha256_crypt_r (const char *key, const char *salt, char *buffer, int buflen)
> NSSLOWInitContext *nss_ictx = NSSLOW_Init ();
> if (nss_ictx == NULL)
> {
> - free (free_key);
> + scratch_buffer_free (&sbuf);
> + scratch_buffer_free (&salt_buf);
> return NULL;
> }
> NSSLOWHASHContext *nss_ctx = NULL;
> @@ -243,17 +256,13 @@ __sha256_crypt_r (const char *key, const char *salt, char *buffer, int buflen)
> sha256_finish_ctx (&alt_ctx, nss_alt_ctx, temp_result);
>
> /* Create byte sequence P. */
> - if (__libc_use_alloca (alloca_used + key_len))
> - cp = p_bytes = (char *) alloca (key_len);
> - else
> + if (!scratch_buffer_set_array_size (&p_bytes_buf, 1, key_len))
> {
> - free_pbytes = cp = p_bytes = (char *)malloc (key_len);
> - if (free_pbytes == NULL)
> - {
> - free (free_key);
> - return NULL;
> - }
> + scratch_buffer_free (&sbuf);
> + scratch_buffer_free (&salt_buf);
> + return NULL;
> }
> + cp = p_bytes = p_bytes_buf.data;
>
> for (cnt = key_len; cnt >= 32; cnt -= 32)
> cp = mempcpy (cp, temp_result, 32);
> @@ -270,7 +279,15 @@ __sha256_crypt_r (const char *key, const char *salt, char *buffer, int buflen)
> sha256_finish_ctx (&alt_ctx, nss_alt_ctx, temp_result);
>
> /* Create byte sequence S. */
> - cp = s_bytes = alloca (salt_len);
> + if (!scratch_buffer_set_array_size (&s_bytes_buf, 1, salt_len))
> + {
> + scratch_buffer_free (&sbuf);
> + scratch_buffer_free (&p_bytes_buf);
> + scratch_buffer_free (&salt_buf);
> + return NULL;
> + }
> + cp = s_bytes = s_bytes_buf.data;
> +
> for (cnt = salt_len; cnt >= 32; cnt -= 32)
> cp = mempcpy (cp, temp_result, 32);
> memcpy (cp, temp_result, cnt);
> @@ -381,8 +398,10 @@ __sha256_crypt_r (const char *key, const char *salt, char *buffer, int buflen)
> if (copied_salt != NULL)
> explicit_bzero (copied_salt, salt_len);
>
> - free (free_key);
> - free (free_pbytes);
> + scratch_buffer_free (&sbuf);
> + scratch_buffer_free (&p_bytes_buf);
> + scratch_buffer_free (&salt_buf);
> + scratch_buffer_free (&s_bytes_buf);
> return buffer;
> }
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] crypt: Get rid of alloca usage in __sha256_crypt_r
2023-09-19 16:18 ` Adhemerval Zanella Netto
@ 2023-09-20 15:23 ` Zack Weinberg
2023-09-21 12:59 ` Adhemerval Zanella Netto
0 siblings, 1 reply; 4+ messages in thread
From: Zack Weinberg @ 2023-09-20 15:23 UTC (permalink / raw)
To: Adhemerval Zanella, Joe Simmons-Talbott, bjoern.esser
Cc: GNU libc development
On Tue, Sep 19, 2023, at 12:18 PM, Adhemerval Zanella Netto wrote:
> On 18/09/23 17:26, Joe Simmons-Talbott wrote:
>> Replace alloca usage with scratch_buffers to avoid potential stack
>> overflow.
> Would it possible to import libxcrypt code for sha256/sha512/md5 that
> already does not user alloca or malloc?
I would have no objection. I thought the plan was to delete libc's crypt altogether,
though? If that's slated to happen within the next couple of releases, it might be
less churn to take Joe's patch, or just to do the deletion now.
cc:ing Bjoern Esser in case he has an opinion.
zw
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] crypt: Get rid of alloca usage in __sha256_crypt_r
2023-09-20 15:23 ` Zack Weinberg
@ 2023-09-21 12:59 ` Adhemerval Zanella Netto
0 siblings, 0 replies; 4+ messages in thread
From: Adhemerval Zanella Netto @ 2023-09-21 12:59 UTC (permalink / raw)
To: Zack Weinberg, Joe Simmons-Talbott, bjoern.esser; +Cc: GNU libc development
On 20/09/23 12:23, Zack Weinberg wrote:
> On Tue, Sep 19, 2023, at 12:18 PM, Adhemerval Zanella Netto wrote:
>> On 18/09/23 17:26, Joe Simmons-Talbott wrote:
>>> Replace alloca usage with scratch_buffers to avoid potential stack
>>> overflow.
>> Would it possible to import libxcrypt code for sha256/sha512/md5 that
>> already does not user alloca or malloc?
>
> I would have no objection. I thought the plan was to delete libc's crypt altogether,
> though? If that's slated to happen within the next couple of releases, it might be
> less churn to take Joe's patch, or just to do the deletion now.
>
> cc:ing Bjoern Esser in case he has an opinion.
In fact I think it a good time to start the libcrypt removal.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-09-21 12:59 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-09-18 20:26 [PATCH] crypt: Get rid of alloca usage in __sha256_crypt_r Joe Simmons-Talbott
2023-09-19 16:18 ` Adhemerval Zanella Netto
2023-09-20 15:23 ` Zack Weinberg
2023-09-21 12:59 ` Adhemerval Zanella Netto
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).