Re: [hackers] [quark][PATCH] Fix buffer over-read in decode()
Hey, where are you getting these e-mail diffs from? Am I not subscribed to some Suckless mailing list?
Re: [hackers] [quark][PATCH] Fix buffer over-read in decode()
On Sun, 21 Aug 2022 20:09:16 + HushBugger wrote: > On Wed, 2022-08-17 at 08:49 +0600, NRK wrote: > > I think the `s++` should be removed from the for loop and `s` should > > be incremented as needed inside the loop instead. > > Agreed. I've changed it. Thank you for working out this patch, I have applied it! :)
Re: [hackers] [quark][PATCH] Fix buffer over-read in decode()
On Wed, 2022-08-17 at 08:49 +0600, NRK wrote:
> I think the `s++` should be removed from the for loop and `s` should
> be incremented as needed inside the loop instead.
Agreed. I've changed it.
From 4a3190695eb3f728496f7f242ab43dfe23a66518 Mon Sep 17 00:00:00 2001
From: HushBugger
Date: Tue, 16 Aug 2022 22:37:50 +0200
Subject: [PATCH] Fix buffer over-read in decode()
The format specifier for parsing percent-formatted characters uses a
maximum number of digits, not an exact number of digits.
If the hex number has only one digit this will skip a character,
potentially pointing past the terminating null byte.
---
http.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/http.c b/http.c
index 5b9dade..36f8b1c 100644
--- a/http.c
+++ b/http.c
@@ -135,12 +135,14 @@ decode(const char src[PATH_MAX], char dest[PATH_MAX])
uint8_t n;
const char *s;
- for (s = src, i = 0; *s; s++, i++) {
- if (*s == '%' && (sscanf(s + 1, "%2hhx", ) == 1)) {
+ for (s = src, i = 0; *s; i++) {
+ if (*s == '%' && isxdigit((unsigned char)s[1]) &&
+ isxdigit((unsigned char)s[2])) {
+ sscanf(s + 1, "%2hhx", );
dest[i] = n;
- s += 2;
+ s += 3;
} else {
- dest[i] = *s;
+ dest[i] = *s++;
}
}
dest[i] = '\0';
--
2.30.2
Re: [hackers] [quark][PATCH] Fix buffer over-read in decode()
On Tue, Aug 16, 2022 at 08:58:37PM +, HushBugger wrote:
> Thanks, I don't have a lot of practical C experience.
The reason for the cast is because is a poorly designed
library where the caller needs to ensure that the arg is representable
as an `unsigned char` (i.e 0 .. UCHAR_MAX) or as `EOF` [0].
for (s = src, i = 0; *s; s++, i++) {
- if (*s == '%' && (sscanf(s + 1, "%2hhx", ) == 1)) {
+ if (*s == '%' && isxdigit((unsigned char)s[1]) &&
+ isxdigit((unsigned char)s[2])) {
+ sscanf(s + 1, "%2hhx", );
dest[i] = n;
s += 2;
At a glance, the `s += 2` looks incorrect to me because it's reading 3
bytes, not 2. But then I saw that the for loop increments `s` as well.
IMO messing around with the loop counter in two places is not a good
idea because it makes the code harder to reason about than it needs to
be. I think the `s++` should be removed from the for loop and `s` should
be incremented as needed inside the loop instead.
- s += 2;
+ s += 3;
} else {
- dest[i] = *s;
+ dest[i] = *s++;
But this is mostly style nit, so feel free to ignore.
[0]: https://port70.net/~nsz/c/c11/n1570.html#7.4
- NRK
Re: [hackers] [quark][PATCH] Fix buffer over-read in decode()
On Tue, 2022-08-16 at 21:32 +0200, Hiltjo Posthuma wrote:
> Haven't tested the patch and not sure it is correct, but if so then
> isxdigit needs a cast using (unsigned char).
Thanks, I don't have a lot of practical C experience. Or experience
with submitting code through email, I seem to have mangled the last
patch. (Should've used git send-email.)
I'll make it an attachment this time, I hope that works.
From 54910d33ba7d1420e39662d0643eea156eac1187 Mon Sep 17 00:00:00 2001
From: HushBugger
Date: Tue, 16 Aug 2022 22:37:50 +0200
Subject: [PATCH] Fix buffer over-read in decode()
The format specifier for parsing percent-formatted characters uses a
maximum number of digits, not an exact number of digits.
If the hex number has only one digit this will skip a character,
potentially pointing past the terminating null byte.
---
http.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/http.c b/http.c
index 5b9dade..bd58c4f 100644
--- a/http.c
+++ b/http.c
@@ -136,7 +136,9 @@ decode(const char src[PATH_MAX], char dest[PATH_MAX])
const char *s;
for (s = src, i = 0; *s; s++, i++) {
- if (*s == '%' && (sscanf(s + 1, "%2hhx", ) == 1)) {
+ if (*s == '%' && isxdigit((unsigned char)s[1]) &&
+ isxdigit((unsigned char)s[2])) {
+ sscanf(s + 1, "%2hhx", );
dest[i] = n;
s += 2;
} else {
--
2.30.2
Re: [hackers] [quark][PATCH] Fix buffer over-read in decode()
On Tue, Aug 16, 2022 at 05:42:50PM +, HushBugger wrote:
> The format specifier for parsing percent-formatted characters uses
> a maximum number of digits, not an exact number of digits.
>
> If the hex number has only one digit this will skip a character,
> potentially pointing past the terminating null byte.
> ---
> http.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/http.c b/http.c
> index 5b9dade..fb2dc42 100644
> --- a/http.c
> +++ b/http.c
> @@ -136,7 +136,8 @@ decode(const char src[PATH_MAX], char dest[PATH_MAX])
> const char *s;
>
> for (s = src, i = 0; *s; s++, i++) {
> - if (*s == '%' && (sscanf(s + 1, "%2hhx", ) == 1)) {
> + if (*s == '%' && isxdigit(s[1]) && isxdigit(s[2])) {
> + sscanf(s + 1, "%2hhx", );
> dest[i] = n;
> s += 2;
> } else {
> --
> 2.36.2
>
Haven't tested the patch and not sure it is correct, but if so then isxdigit
needs a cast using (unsigned char).
--
Kind regards,
Hiltjo
[hackers] [quark][PATCH] Fix buffer over-read in decode()
The format specifier for parsing percent-formatted characters uses
a maximum number of digits, not an exact number of digits.
If the hex number has only one digit this will skip a character,
potentially pointing past the terminating null byte.
---
http.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/http.c b/http.c
index 5b9dade..fb2dc42 100644
--- a/http.c
+++ b/http.c
@@ -136,7 +136,8 @@ decode(const char src[PATH_MAX], char
dest[PATH_MAX])
const char *s;
for (s = src, i = 0; *s; s++, i++) {
- if (*s == '%' && (sscanf(s + 1, "%2hhx", ) == 1)) {
+ if (*s == '%' && isxdigit(s[1]) && isxdigit(s[2])) {
+ sscanf(s + 1, "%2hhx", );
dest[i] = n;
s += 2;
} else {
--
2.36.2
