[Invalid] sodium_bin2base64() can cause a buffer overflow

[davidebeatrici]

When b64_maxlen's value is less than the required output buffer, sodium_bin2base64() is supposed to fail:

https://github.com/jedisct1/libsodium/blob/927dfe8e2eaa86160d3ba12a7e3258fbc322909c/src/libsodium/sodium/codecs.c#L202-L204

const char str[32] = "Hi there, I'm a string!";
char buf[44];
sodium_bin2base64(buf, sizeof(buf), str, sizeof(str), sodium_base64_VARIANT_ORIGINAL);
printf("%s\n", buf);

Causes a segmentation fault.

const char str[32] = "Hi there, I'm a string!";
char buf[sodium_base64_ENCODED_LEN(sizeof(str), sodium_base64_VARIANT_ORIGINAL)];
sodium_bin2base64(buf, sizeof(buf), str, sizeof(str), sodium_base64_VARIANT_ORIGINAL);
printf("%s\n", buf);

Prints SGkgdGhlcmUsIEknbSBhIHN0cmluZyEAAAAAAAAAAAA=.

The size of buf is 45.

[jedisct1]

It calls sodium_misuse(), that calls abort(). This is not a segmentation fault.

The function expects a buffer large enough to hold the result. If this is not the case, there is a bug in the code.

Applications are unlikely to check the returned value of these conversion functions. On the other hand, giving an incorrectly sized buffer can happen easily. So the safest thing to do in such an unexpected scenario is to abort, rather than keep the execution going with a truncated result or an unmodified output buffer.

[jedisct1]

When an error is expected, such as a signature that doesn't verify, libsodium and libhydrogen return a soft error.

But if a function is used incorrectly, sodium_misuse() is called to abort the execution. It never happens unless there is a bug in the application or in bindings, like in this example, an output buffer that would cause an overflow.

In JavaScript, an exception would be raised. But in C, you can't assume applications to always perform error checking, especially when do simple operations that have no reasons to fail.

sodium_misuse() has proven to be useful against classic sizeof(pointer instead of array) mistakes.

[jedisct1]

By the way, you can change what happens on a panic with sodium_set_misuse_handler().

But really, the safest thing to do if that ever happens is to immediately abort.

[davidebeatrici]

My fault, I read Aborted (core dumped) and instantly assumed it was a segmentation fault, without checking what sodium_misuse() actually did, sorry about that.

Thank you very much for the detailed explanation! Calling abort() is indeed very useful in such a case. However, should perhaps a message be printed to stderr? Otherwise, if no debug symbols are installed for the library, the debugger doesn't know why the process forcibly exits.

Also, there's still a thing that bugs me about sodium_bin2base64():

const char str[32] = "Hi there, I'm a string!";
char buf[44];
sodium_bin2base64(buf, sizeof(buf), str, sizeof(str), sodium_base64_VARIANT_ORIGINAL_NO_PADDING);
printf("%s\n", buf);

Prints SGkgdGhlcmUsIEknbSBhIHN0cmluZyEAAAAAAAAAAAA (truncated output).

Decreasing the size of buf by 1 (43) causes the function to call abort(). Is perhaps the terminator character not taken into account with sodium_base64_VARIANT_ORIGINAL_NO_PADDING?

[jedisct1]

Calling abort() is indeed very useful in such a case. However, should perhaps a message be printed to stderr?

Applications can use descriptor 2 for something else, or be displayed to untrusted users. And if misuse() is hit in an application that usually works as expected, it may be due to memory corruption. In that case, we want to terminate as soon as possible rather than call anything that may act as an exploit vector.

Is perhaps the terminator character not taken into account with sodium_base64_VARIANT_ORIGINAL_NO_PADDING

Adding an extra \0 to end a string only exists in C. In all other languages, string lengths are stored separately, so they reserve the actual string size, and if an extra zero that has nothing to do with the string itself was added, we would get a buffer overflow. If there is space for an additional byte, a zero will be written. If there is exactly the required size, only the encoded content will be written. And if there is less, the function will trap. (scratch that, libsodium always terminates strings with \0)

Encoding 32 bytes require 43 characters (this is the length of the SGkgdGhlcmUsIEknbSBhIHN0cmluZyEAAAAAAAAAAAA string in your case). With an output buffer that's only 43 bytes, the string couldn't be zero terminated.

[davidebeatrici]

All clear!

[jedisct1]

By the way, besides being constant-time for a given length, libsodium's base64 implementation ensures that input data and encoded strings are bijective.

Most implementations accept encoded strings they would never create, and don't handle these consistently.

For example the string PHP is invalid base64. Even without padding, some bits would be missing.

PHP & JS

echo base64_encode(base64_decode(“PHP”));

Buffer.alloc(2, "PHP", "base64").toString("base64")

Both accept the PHP string, but would have encoded it PHM=.

base64 command-line tool (macOS):

echo PHP|base64 -d|base64

No error, the command returns 0, but returns an empty output.

echo AAAPHP|base64 -d|base64

Now it returns AAAP, having silently ignored two characters that are not padding.

base64 command-line tool (GNU coreutils)

Prints Invalid input and exits with error code 1, yay.

OpenSSL

echo PHP|openssl enc -d -base64

echo AAAPHP|openssl enc -d -base64

Exit code 0, but returns an empty output in both cases.

[davidebeatrici]

Wow, I wasn't aware of that.

Thank you again for the explanation!

[jedisct1]

Base64 is not a good encoding scheme, and I would simply opt for hex encoding most of the time.

• Base64 is malleable
• It has a bunch of incompatible variants
• It is visually confusing (includes l, O, 1 and 0 characters for example)
• It is slower to encode and decode than hex encoding
• Computing the length before and after decoding is complicated
• Hex encoding is smaller after compression

[davidebeatrici]

Unfortunately I have to stick to Base64, as I'm implementing WireGuard in SoftEther VPN.

The private and public key are saved in Base64.