[PATCH] Fix small overread during SASLprep
Hi all,
pg_utf8_string_len() doesn't check the remaining string length before
calling pg_utf8_is_legal(), so there's a possibility of jumping a
couple of bytes past the end of the string. (The overread stops there,
because the function won't validate a sequence containing a null
byte.)
Here's a quick patch to fix it. I didn't see any other uses of
pg_utf8_is_legal() with missing length checks.
Thanks,
--Jacob
Attachments:
pg_utf8_string_len-honor-null-terminators.patchapplication/octet-stream; name=pg_utf8_string_len-honor-null-terminators.patchDownload
From 044cdf5835e93df01aa7b3ca439ab4740997fbe9 Mon Sep 17 00:00:00 2001
From: Jacob Champion <jacob.champion@enterprisedb.com>
Date: Thu, 22 Aug 2024 09:53:57 -0700
Subject: [PATCH] pg_utf8_string_len: honor null terminators
Callers of pg_utf8_is_legal() must verify that there's enough length
remaining in the string, and pg_utf8_string_len() wasn't doing that, so
SASLprep could jump slightly past the end of the provided password.
---
src/common/saslprep.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/common/saslprep.c b/src/common/saslprep.c
index 315ccacd7c..78f6fcbd80 100644
--- a/src/common/saslprep.c
+++ b/src/common/saslprep.c
@@ -1004,15 +1004,17 @@ pg_utf8_string_len(const char *source)
const unsigned char *p = (const unsigned char *) source;
int l;
int num_chars = 0;
+ size_t len = strlen(source);
- while (*p)
+ while (len)
{
l = pg_utf_mblen(p);
- if (!pg_utf8_islegal(p, l))
+ if (len < l || !pg_utf8_islegal(p, l))
return -1;
p += l;
+ len -= l;
num_chars++;
}
--
2.34.1
On 9 Sep 2024, at 17:29, Jacob Champion <jacob.champion@enterprisedb.com> wrote:
pg_utf8_string_len() doesn't check the remaining string length before
calling pg_utf8_is_legal(), so there's a possibility of jumping a
couple of bytes past the end of the string. (The overread stops there,
because the function won't validate a sequence containing a null
byte.)Here's a quick patch to fix it. I didn't see any other uses of
pg_utf8_is_legal() with missing length checks.
Just to make sure I understand, this is for guarding against overreads in
validation of strings containing torn MB characters? Assuming I didn't
misunderstand you this patch seems correct to me.
--
Daniel Gustafsson
On Mon, Sep 9, 2024 at 11:30 AM Daniel Gustafsson <daniel@yesql.se> wrote:
Just to make sure I understand, this is for guarding against overreads in
validation of strings containing torn MB characters?
Right. Our SASLprep code doesn't require/enforce UTF8-encoded inputs.
Assuming I didn't
misunderstand you this patch seems correct to me.
Thanks for the review!
--Jacob
On 9 Sep 2024, at 20:41, Jacob Champion <jacob.champion@enterprisedb.com> wrote:
On Mon, Sep 9, 2024 at 11:30 AM Daniel Gustafsson <daniel@yesql.se> wrote:
Just to make sure I understand, this is for guarding against overreads in
validation of strings containing torn MB characters?Right. Our SASLprep code doesn't require/enforce UTF8-encoded inputs.
Thanks for confirming, I'll have another look in the morning and will apply
then unless there are objections.
--
Daniel Gustafsson
On 9 Sep 2024, at 23:21, Daniel Gustafsson <daniel@yesql.se> wrote:
On 9 Sep 2024, at 20:41, Jacob Champion <jacob.champion@enterprisedb.com> wrote:
On Mon, Sep 9, 2024 at 11:30 AM Daniel Gustafsson <daniel@yesql.se> wrote:
Just to make sure I understand, this is for guarding against overreads in
validation of strings containing torn MB characters?Right. Our SASLprep code doesn't require/enforce UTF8-encoded inputs.
Thanks for confirming, I'll have another look in the morning and will apply
then unless there are objections.
Pushed, thanks!
--
Daniel Gustafsson