* [PATCH v2 0/2] Fix out-of-bounds potential in decode_base64 and add regression tests
@ 2025-12-02 9:22 Jonas Rebmann
2025-12-02 9:22 ` [PATCH v2 1/2] lib: base64: Fix out-of-bounds potential by respecting dst_len Jonas Rebmann
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Jonas Rebmann @ 2025-12-02 9:22 UTC (permalink / raw)
To: Sascha Hauer, BAREBOX; +Cc: Jonas Rebmann
I took a closer look at decode_base64 while reviewing coverity report
584740 (Out-of-bounds access). 1/2 resolves the issue (although coverity
seems to suspect an out-of-bounds access for the wrong reason and might
keep doing so), 2/2 adds a selftest I used to debug the issue.
Signed-off-by: Jonas Rebmann <jre@pengutronix.de>
---
Changes in v2:
- correct name of helper function from __expect_streq() to
__expect_base64()
- Link to v1: https://lore.barebox.org/barebox/20251201-base64-bounds-v1-0-3ae2b2e8b7cb@pengutronix.de
---
Jonas Rebmann (2):
lib: base64: Fix out-of-bounds potential by respecting dst_len
test: self: add base64 selftests
lib/base64.c | 10 +++++-----
test/self/Kconfig | 7 +++++++
test/self/Makefile | 1 +
test/self/base64.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 58 insertions(+), 5 deletions(-)
---
base-commit: ec00fef65d808f8bc9c5655262b0e4f8ce2c4e92
change-id: 20251201-base64-bounds-ed379c2c6ff7
Best regards,
--
Jonas Rebmann <jre@pengutronix.de>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 1/2] lib: base64: Fix out-of-bounds potential by respecting dst_len
2025-12-02 9:22 [PATCH v2 0/2] Fix out-of-bounds potential in decode_base64 and add regression tests Jonas Rebmann
@ 2025-12-02 9:22 ` Jonas Rebmann
2025-12-03 8:59 ` Sascha Hauer
2025-12-02 9:22 ` [PATCH v2 2/2] test: self: add base64 selftests Jonas Rebmann
2025-12-08 7:45 ` [PATCH v2 0/2] Fix out-of-bounds potential in decode_base64 and add regression tests Sascha Hauer
2 siblings, 1 reply; 6+ messages in thread
From: Jonas Rebmann @ 2025-12-02 9:22 UTC (permalink / raw)
To: Sascha Hauer, BAREBOX; +Cc: Jonas Rebmann
__decode_base64 generally writes the input in 3 bytes increments,
corresponding to 4 bytes increments in the base64 input buffer. This
means that in order to respect dst_len as the size of the output buffer,
the case of exceeding dst_len within a loop iteration must be
considered.
In such a case, refrain from writing the last one or two bytes if that
write would be past dst_len.
Signed-off-by: Jonas Rebmann <jre@pengutronix.de>
---
lib/base64.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/lib/base64.c b/lib/base64.c
index d5ab217528..3e29f0a56c 100644
--- a/lib/base64.c
+++ b/lib/base64.c
@@ -163,19 +163,19 @@ static int __decode_base64(char *p_dst, int dst_len, const char *src, bool url)
*/
if (count > 1)
*dst++ = six_bit[0] << 2 | six_bit[1] >> 4;
- if (count > 2)
+ if (count > 2 && dst_len > 1)
*dst++ = six_bit[1] << 4 | six_bit[2] >> 2;
- if (count > 3)
+ if (count > 3 && dst_len > 2)
*dst++ = six_bit[2] << 6 | six_bit[3];
+ /* last character was "=" */
+ if (count != 0)
+ length += min(count - 1, dst_len);
/*
* Note that if we decode "AA==" and ate first '=',
* we just decoded one char (count == 2) and now we'll
* do the loop once more to decode second '='.
*/
dst_len -= count-1;
- /* last character was "=" */
- if (count != 0)
- length += count - 1;
}
ret:
p_dst = dst;
--
2.51.2.535.g419c72cb8a
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2 2/2] test: self: add base64 selftests
2025-12-02 9:22 [PATCH v2 0/2] Fix out-of-bounds potential in decode_base64 and add regression tests Jonas Rebmann
2025-12-02 9:22 ` [PATCH v2 1/2] lib: base64: Fix out-of-bounds potential by respecting dst_len Jonas Rebmann
@ 2025-12-02 9:22 ` Jonas Rebmann
2025-12-08 7:45 ` [PATCH v2 0/2] Fix out-of-bounds potential in decode_base64 and add regression tests Sascha Hauer
2 siblings, 0 replies; 6+ messages in thread
From: Jonas Rebmann @ 2025-12-02 9:22 UTC (permalink / raw)
To: Sascha Hauer, BAREBOX; +Cc: Jonas Rebmann
These tests are specifically tailored around respecting the dst_len
parameter.
Signed-off-by: Jonas Rebmann <jre@pengutronix.de>
---
test/self/Kconfig | 7 +++++++
test/self/Makefile | 1 +
test/self/base64.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 53 insertions(+)
diff --git a/test/self/Kconfig b/test/self/Kconfig
index 936b12072e..adef8609ef 100644
--- a/test/self/Kconfig
+++ b/test/self/Kconfig
@@ -28,6 +28,7 @@ config SELFTEST_AUTORUN
config SELFTEST_ENABLE_ALL
bool "Enable all self-tests"
+ select SELFTEST_BASE64
select SELFTEST_RANGE
select SELFTEST_PRINTF
select SELFTEST_MALLOC
@@ -52,6 +53,12 @@ config SELFTEST_ENABLE_ALL
help
Selects all self-tests compatible with current configuration
+config SELFTEST_BASE64
+ bool "base64 selftest"
+ select BASE64
+ help
+ Tests base64 implementation
+
config SELFTEST_RANGE
bool "range.h selftest"
help
diff --git a/test/self/Makefile b/test/self/Makefile
index 0bd947928a..d244c19052 100644
--- a/test/self/Makefile
+++ b/test/self/Makefile
@@ -1,6 +1,7 @@
# SPDX-License-Identifier: GPL-2.0
obj-$(CONFIG_SELFTEST) += core.o
+obj-$(CONFIG_SELFTEST_BASE64) += base64.o
obj-$(CONFIG_SELFTEST_RANGE) += range.o
obj-$(CONFIG_SELFTEST_MALLOC) += malloc.o
obj-$(CONFIG_SELFTEST_TALLOC) += talloc.o
diff --git a/test/self/base64.c b/test/self/base64.c
new file mode 100644
index 0000000000..c9140aedb9
--- /dev/null
+++ b/test/self/base64.c
@@ -0,0 +1,45 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <common.h>
+#include <bselftest.h>
+#include <base64.h>
+#include <string.h>
+
+BSELFTEST_GLOBALS();
+
+static void __expect_base64(const char *func, int line, int dst_len,
+ const char *src, int expect_len, const char *expect)
+{
+ int ret;
+ char *buf = strdup("canary");
+ bool fail = false;
+
+ total_tests++;
+ ret = decode_base64(buf, dst_len, src);
+ if (!streq_ptr(buf, expect)) {
+ fail = true;
+ printf("%s:%d: got '%s', but '%s' expected\n", func, line, buf,
+ expect);
+ }
+ if (ret != expect_len) {
+ fail = true;
+ printf("%s:%d: got length %i, but %i expected\n", func, line,
+ ret, expect_len);
+ }
+ if (fail)
+ failed_tests++;
+ free(buf);
+}
+
+#define expect_base64(dst_len, src, expect_len, expect) \
+ __expect_base64(__func__, __LINE__, dst_len, src, expect_len, expect)
+
+static void test_base64(void)
+{
+ expect_base64(1, "QUJD", 1, "Aanary");
+ expect_base64(5, "QUJD", 3, "ABCary");
+ expect_base64(5, "$UJD", 0, "canary");
+}
+bselftest(parser, test_base64);
--
2.51.2.535.g419c72cb8a
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] lib: base64: Fix out-of-bounds potential by respecting dst_len
2025-12-02 9:22 ` [PATCH v2 1/2] lib: base64: Fix out-of-bounds potential by respecting dst_len Jonas Rebmann
@ 2025-12-03 8:59 ` Sascha Hauer
2025-12-03 12:01 ` Ahmad Fatoum
0 siblings, 1 reply; 6+ messages in thread
From: Sascha Hauer @ 2025-12-03 8:59 UTC (permalink / raw)
To: Jonas Rebmann; +Cc: BAREBOX
On Tue, Dec 02, 2025 at 10:22:44AM +0100, Jonas Rebmann wrote:
> __decode_base64 generally writes the input in 3 bytes increments,
> corresponding to 4 bytes increments in the base64 input buffer. This
> means that in order to respect dst_len as the size of the output buffer,
> the case of exceeding dst_len within a loop iteration must be
> considered.
>
> In such a case, refrain from writing the last one or two bytes if that
> write would be past dst_len.
>
> Signed-off-by: Jonas Rebmann <jre@pengutronix.de>
> ---
> lib/base64.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
I wonder if we should switch to the kernel functions from lib/base64.c
instead. They look much more straight forward than the busybox based
implementation.
Sascha
>
> diff --git a/lib/base64.c b/lib/base64.c
> index d5ab217528..3e29f0a56c 100644
> --- a/lib/base64.c
> +++ b/lib/base64.c
> @@ -163,19 +163,19 @@ static int __decode_base64(char *p_dst, int dst_len, const char *src, bool url)
> */
> if (count > 1)
> *dst++ = six_bit[0] << 2 | six_bit[1] >> 4;
> - if (count > 2)
> + if (count > 2 && dst_len > 1)
> *dst++ = six_bit[1] << 4 | six_bit[2] >> 2;
> - if (count > 3)
> + if (count > 3 && dst_len > 2)
> *dst++ = six_bit[2] << 6 | six_bit[3];
> + /* last character was "=" */
> + if (count != 0)
> + length += min(count - 1, dst_len);
> /*
> * Note that if we decode "AA==" and ate first '=',
> * we just decoded one char (count == 2) and now we'll
> * do the loop once more to decode second '='.
> */
> dst_len -= count-1;
> - /* last character was "=" */
> - if (count != 0)
> - length += count - 1;
> }
> ret:
> p_dst = dst;
>
> --
> 2.51.2.535.g419c72cb8a
>
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] lib: base64: Fix out-of-bounds potential by respecting dst_len
2025-12-03 8:59 ` Sascha Hauer
@ 2025-12-03 12:01 ` Ahmad Fatoum
0 siblings, 0 replies; 6+ messages in thread
From: Ahmad Fatoum @ 2025-12-03 12:01 UTC (permalink / raw)
To: Sascha Hauer, Jonas Rebmann; +Cc: BAREBOX
Hi Sascha,
On 12/3/25 9:59 AM, Sascha Hauer wrote:
> On Tue, Dec 02, 2025 at 10:22:44AM +0100, Jonas Rebmann wrote:
>> __decode_base64 generally writes the input in 3 bytes increments,
>> corresponding to 4 bytes increments in the base64 input buffer. This
>> means that in order to respect dst_len as the size of the output buffer,
>> the case of exceeding dst_len within a loop iteration must be
>> considered.
>>
>> In such a case, refrain from writing the last one or two bytes if that
>> write would be past dst_len.
>>
>> Signed-off-by: Jonas Rebmann <jre@pengutronix.de>
>> ---
>> lib/base64.c | 10 +++++-----
>> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> I wonder if we should switch to the kernel functions from lib/base64.c
> instead. They look much more straight forward than the busybox based
> implementation.
Just need to take care to support base64url, which is needed for the
JSON web token support.
Cheers,
Ahmad
>
> Sascha
>
>>
>> diff --git a/lib/base64.c b/lib/base64.c
>> index d5ab217528..3e29f0a56c 100644
>> --- a/lib/base64.c
>> +++ b/lib/base64.c
>> @@ -163,19 +163,19 @@ static int __decode_base64(char *p_dst, int dst_len, const char *src, bool url)
>> */
>> if (count > 1)
>> *dst++ = six_bit[0] << 2 | six_bit[1] >> 4;
>> - if (count > 2)
>> + if (count > 2 && dst_len > 1)
>> *dst++ = six_bit[1] << 4 | six_bit[2] >> 2;
>> - if (count > 3)
>> + if (count > 3 && dst_len > 2)
>> *dst++ = six_bit[2] << 6 | six_bit[3];
>> + /* last character was "=" */
>> + if (count != 0)
>> + length += min(count - 1, dst_len);
>> /*
>> * Note that if we decode "AA==" and ate first '=',
>> * we just decoded one char (count == 2) and now we'll
>> * do the loop once more to decode second '='.
>> */
>> dst_len -= count-1;
>> - /* last character was "=" */
>> - if (count != 0)
>> - length += count - 1;
>> }
>> ret:
>> p_dst = dst;
>>
>> --
>> 2.51.2.535.g419c72cb8a
>>
>>
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 0/2] Fix out-of-bounds potential in decode_base64 and add regression tests
2025-12-02 9:22 [PATCH v2 0/2] Fix out-of-bounds potential in decode_base64 and add regression tests Jonas Rebmann
2025-12-02 9:22 ` [PATCH v2 1/2] lib: base64: Fix out-of-bounds potential by respecting dst_len Jonas Rebmann
2025-12-02 9:22 ` [PATCH v2 2/2] test: self: add base64 selftests Jonas Rebmann
@ 2025-12-08 7:45 ` Sascha Hauer
2 siblings, 0 replies; 6+ messages in thread
From: Sascha Hauer @ 2025-12-08 7:45 UTC (permalink / raw)
To: BAREBOX, Jonas Rebmann
On Tue, 02 Dec 2025 10:22:43 +0100, Jonas Rebmann wrote:
> I took a closer look at decode_base64 while reviewing coverity report
> 584740 (Out-of-bounds access). 1/2 resolves the issue (although coverity
> seems to suspect an out-of-bounds access for the wrong reason and might
> keep doing so), 2/2 adds a selftest I used to debug the issue.
>
>
Applied, thanks!
[1/2] lib: base64: Fix out-of-bounds potential by respecting dst_len
https://git.pengutronix.de/cgit/barebox/commit/?id=fd1a97622105 (link may not be stable)
[2/2] test: self: add base64 selftests
https://git.pengutronix.de/cgit/barebox/commit/?id=c5190aac91f2 (link may not be stable)
Best regards,
--
Sascha Hauer <s.hauer@pengutronix.de>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-12-08 7:45 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-12-02 9:22 [PATCH v2 0/2] Fix out-of-bounds potential in decode_base64 and add regression tests Jonas Rebmann
2025-12-02 9:22 ` [PATCH v2 1/2] lib: base64: Fix out-of-bounds potential by respecting dst_len Jonas Rebmann
2025-12-03 8:59 ` Sascha Hauer
2025-12-03 12:01 ` Ahmad Fatoum
2025-12-02 9:22 ` [PATCH v2 2/2] test: self: add base64 selftests Jonas Rebmann
2025-12-08 7:45 ` [PATCH v2 0/2] Fix out-of-bounds potential in decode_base64 and add regression tests Sascha Hauer
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.