[dev] [sbase] sha512-224sum: stack-buffer-overflow

[Frank Busse]

Hi,

and another one reported by KLEE:

---
$ printf "" | ./sha512-224sum
ERROR: AddressSanitizer: stack-buffer-overflow
---

Best,

Frank

[Hiltjo Posthuma]

Maybe a starting point is in with processblock():

libutil/sha512.c: sha512_update() (defined in sha512-224.h).

There might be a mismatch in the buffer that is available there.

Initial commit for sha512-224:

a392cd475e1d164c940ab3e3cb893f533af2445a

It is probably adapted from the sha512 implementation, but with some size mismatch.

We should add more references on how this implementation came to be (if there
is any?). "public domain sha512/224 implementation based on fips180-3".

My small peasant brain is too simple for this crypto stuff.
Do we even need this variant of sha512-224? Maybe a rm is a simple solution.

--
Kind regards,
Hiltjo

[Roberto E. Vargas Caballero]

Hi,

On Sun, Oct 26, 2025 at 12:30:57PM +0100, Hiltjo Posthuma wrote:
> On Thu, Oct 23, 2025 at 03:29:08PM +0200, Frank Busse wrote:
> > Hi,
> >
> > and another one reported by KLEE:
> >
> > ---
> > $ printf "" | ./sha512-224sum
> > ERROR: AddressSanitizer: stack-buffer-overflow
> > ---

I think this is a case where we would get a big benefit
of having the backtrace.

> Maybe a starting point is in with processblock():
>
> libutil/sha512.c: sha512_update() (defined in sha512-224.h).
>
> There might be a mismatch in the buffer that is available there.

I think the problem comes more from the fact that the code
is not ready for empty strings and it does something like
strlen(s) - 1 to remove the eos and then it is fucked.
I am going to take a look to see if this is the case. The
backtrace can help a lot here.

>
> Initial commit for sha512-224:
>
> a392cd475e1d164c940ab3e3cb893f533af2445a
>
> It is probably adapted from the sha512 implementation, but with some size mismatch.
>
> We should add more references on how this implementation came to be (if there
> is any?). "public domain sha512/224 implementation based on fips180-3".

Indeed, the base commit does not say anything about where it comes from
so, this makes harder fixing the issue.

> My small peasant brain is too simple for this crypto stuff.

Mine too xD

> Do we even need this variant of sha512-224? Maybe a rm is a simple solution.

I am going to try fixing it in a fast way, but if it becomes
too complex, I think rm can be a solution xD.

Regards,

[Santtu Lakkala]

On 26.10.2025 13.30, Hiltjo Posthuma wrote:

> Maybe a starting point is in with processblock():
>
> libutil/sha512.c: sha512_update() (defined in sha512-224.h).
>

This is actually a simple case of passing wrongly sized buffer to
sha512_sum_n() when finalizing, and happens with any input. The problem
lies with sha512_sum_n() requiring the digest length argument to be in
multiples of uint64_t, but for 224 bits this would be 3.5. Either the
argument needs to be changed to be more granular, or the
sha512_224_sum() in libutil/sha512-224.c needs to use a temporary buffer.

The SHA-256 based sha224sum actually has the same problem, but there the
fix is simpler, just changing the argument of sha256_sum_n to 7, as
SHA-256 internally uses 32-bit ints.

--
Santtu

[Roberto E. Vargas Caballero]

Hi,

On Mon, Nov 03, 2025 at 02:52:21PM +0200, Santtu Lakkala wrote:
> On 26.10.2025 13.30, Hiltjo Posthuma wrote:
>
> > Maybe a starting point is in with processblock():
> >
> > libutil/sha512.c: sha512_update() (defined in sha512-224.h).
> >
>
> This is actually a simple case of passing wrongly sized buffer to
> sha512_sum_n() when finalizing, and happens with any input. The problem lies

Ok, that makes sense. I suppose the buffer passes depends of the 'n' used
in the specific hash.

> with sha512_sum_n() requiring the digest length argument to be in multiples
> of uint64_t, but for 224 bits this would be 3.5. Either the argument needs
> to be changed to be more granular, or the sha512_224_sum() in
> libutil/sha512-224.c needs to use a temporary buffer.

uhmmmm, using an internal buffer can produce some problems in threaded programs.
Maybe we can add a buffer to the ctxt parameter. Do you think the problem can
be solved in that way?

> The SHA-256 based sha224sum actually has the same problem, but there the fix
> is simpler, just changing the argument of sha256_sum_n to 7, as SHA-256
> internally uses 32-bit ints.

Sorry, I didn't understand this point. I don't see the relation between 7 and
the internal 32-bit ints. Can you elaborate it a bit more? (take in account that
I didn't write that code).

Regards,

[Roberto E. Vargas Caballero]

Hi,

On Mon, Nov 03, 2025 at 04:07:49PM +0200, Santtu Lakkala wrote:
> > Ok, that makes sense. I suppose the buffer passes depends of the 'n' used
> > in the specific hash.
>

> Yes, in case of SHA-512/224, only the 224 first bits of the produced data
> are used. But as the 'n' is in units of 64-bit items, 224 cannot be
> represented as an integer.

64 bit units? Do you mean a power of 2 into a 64 bit integer?

> The buffer would be only 32 bytes, should pose no problems just having the
> temp array as a local variable in the sha512_224_sum() function, something
> like:
>
> void
> sha512_224_sum(void *ctx, uint8_t md[SHA512_224_DIGEST_LENGTH])
> {
> uint8_t buf[32];
> sha512_sum_n(ctx, buf, 4);
> memcpy(md, buf, SHA512_224_DIGEST_LENGTH);
> }
>
> Otherwise sha512_sum_n() needs to be adjusted to take the digest size in
> some other unit.

Ok, in that case having it in the stack makes everything much easier.
I suppose we would have to apply similar changes to the other versions
of the hash, wouldn't we?

> > Sorry, I didn't understand this point. I don't see the relation between 7 and
> > the internal 32-bit ints. Can you elaborate it a bit more? (take in account that
> > I didn't write that code).
>

> Sorry, it was a bit unclear. sha256_sum_n expects the size argument to be in
> units of the internal state array, which is of 32-bit ints. Now as 224 is an
> exact multiple of 32, the overflow can be simply fixed by passing in the
> correct 224/32 = 7 (instead of the current 8).

Ok. At this point is clear that you know much better than me what we should
change. Can you write the patch and I'll review it?

Regards,

[Страхиња Радић]

Дана 25/11/03 12:21PM, Roberto E. Vargas Caballero написа:

> I think the problem comes more from the fact that the code
> is not ready for empty strings and it does something like
> strlen(s) - 1 to remove the eos and then it is fucked.

If that is the case, then maybe assert(3) would help in future such
cases. Granted, it adds LoC, but makes these kind of errors easier to
pinpoint.

[Steffen Nurpmeso]

Страхиња Радић wrote in
<xota56vyn6cmpbgyjhuh776b2iiqdwm7r5h2wlko3rbieqohos@kjxt67tigmx3>:

personal opinion: assert should not be in shipout code, and if
0 is a legitimate thing (anything is legitimate if data comes from
a user), then assert is wrong per se.

--End of <xota56vyn6cmpbgyjhuh776b2iiqdwm7r5h2wlko3rbieqohos@kjxt67tigmx3>

Having said that, my beloved BSD-style Linux Distribution CRUX
(there is only ever Alpine and CRUX for me; void i would need to
recheck, but what did they do to the project founder, who did that
great -- afaik -- package manager; TinyCore was not for me) just
introduced via notify email a dependency on rust today! Pretty
sure that now meanders, now that it is in, and the little overlay
i will introduce will soon no longer be manageable. I need it for
gdk-pixbuf which is needed by gtk3 which is needed by firefox-bin
(Mozilla compiled binary). I have now downloaded the tor browser,
but it seems it also needs gtk environment back and forth. What
a mess. I just cannot live without a browser that can access
"those things", once in a while.

--steffen
|
|Der Kragenbaer, The moon bear,
|der holt sich munter he cheerfully and one by one
|einen nach dem anderen runter wa.ks himself off
|(By Robert Gernhardt)

[Santtu Lakkala]

On 3.11.2025 15.45, Roberto E. Vargas Caballero wrote>> This is actually

a simple case of passing wrongly sized buffer to
>> sha512_sum_n() when finalizing, and happens with any input. The problem lies
>
> Ok, that makes sense. I suppose the buffer passes depends of the 'n' used
> in the specific hash.

Yes, in case of SHA-512/224, only the 224 first bits of the produced
data are used. But as the 'n' is in units of 64-bit items, 224 cannot be
represented as an integer.

>

>> with sha512_sum_n() requiring the digest length argument to be in multiples
>> of uint64_t, but for 224 bits this would be 3.5. Either the argument needs
>> to be changed to be more granular, or the sha512_224_sum() in
>> libutil/sha512-224.c needs to use a temporary buffer.
>
> uhmmmm, using an internal buffer can produce some problems in threaded programs.
> Maybe we can add a buffer to the ctxt parameter. Do you think the problem can
> be solved in that way?

The buffer would be only 32 bytes, should pose no problems just having
the temp array as a local variable in the sha512_224_sum() function,
something like:

void
sha512_224_sum(void *ctx, uint8_t md[SHA512_224_DIGEST_LENGTH])
{
uint8_t buf[32];
sha512_sum_n(ctx, buf, 4);
memcpy(md, buf, SHA512_224_DIGEST_LENGTH);
}

Otherwise sha512_sum_n() needs to be adjusted to take the digest size in
some other unit.

>> The SHA-256 based sha224sum actually has the same problem, but there the fix
>> is simpler, just changing the argument of sha256_sum_n to 7, as SHA-256
>> internally uses 32-bit ints.
>
> Sorry, I didn't understand this point. I don't see the relation between 7 and
> the internal 32-bit ints. Can you elaborate it a bit more? (take in account that
> I didn't write that code).

Sorry, it was a bit unclear. sha256_sum_n expects the size argument to
be in units of the internal state array, which is of 32-bit ints. Now as
224 is an exact multiple of 32, the overflow can be simply fixed by
passing in the correct 224/32 = 7 (instead of the current 8).

--
Santtu