* [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
* 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
* [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 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.