[Bug]: buffer overflow in wc_Chacha_Process on aarch64/android with asm enabled

[byron-hawkins]

Contact Details

byron@introspicion.com

Version

v5.7.2-stable (github clone)

Description

Using the configuration indicated in the attached wolfssl.config.log, a simple encryption of a 14-byte text buffer writes 12 bytes beyond the end of the ChaCha struct. In function wc_Chacha_encrypt_64, inline assembly at line 2181 begins writing at 4 bytes from the end of the ChaCha struct, and writes at least 8 bytes too far (corrupting a pointer that happens to be located there in my code). Aligning the ChaCha struct to 16 bytes on the stack did not make any difference. It appears the assembly instruction at line 2181 is the wrong variant for variable over. Rebuilding with --enable-armasm=no resolves the problem.

wolfssl.config.log

Reproduction steps

1. ./configure --enable-curl=yes --build x86_64-linux-gnu --host aarch64 --with-arm-target=cortex --enable-armasm --enable-chacha=yes
2. build and run any basic encryption step
3. buffer overflow corrupts memory following the ChaCha struct

Relevant log output

Configuration log is large, so it is attached instead.

[dgarske]

Hi @byron-hawkins ,

Can you expand on this step “build and run any basic encryption step”? In your application are you including the wolfssl/options.h before any other wolfSSL headers? My concern is a mismatch of the ChaCha20 struct from what we build from what your test applications uses. Are you able to share your test application to review?

Also what is the ARM processor you are running on?

Thanks,
David Garske, wolfSSL

[byron-hawkins]

After adding #include "wolfssl/options.h" above #include "wolfssl/wolfcrypt/chacha.h", the problem is gone!

How is a user intended to discover this requirement? On the quick start page, there are examples where options.h is included before other headers, but it is nowhere stated as a requirement. That page refers to the tutorial, which also does not mention this requirement. Since it is a do-or-die usage constraint, I would suggest a preprocessor strategy to protect the build, for example, by putting #error statements wherever an include file has been reached ahead of options.h. This would obviously involve many source file changes, but would be easily automated.

[dgarske]

Hi @byron-hawkins ,

I am happy to hear that resolved things. Internally we've discussed this many times. Most of the solutions would break backwards compatibility.

It's the most common issue. From the FAQ its item 1. See: https://www.wolfssl.com/docs/frequently-asked-questions-faq/#How_do_I_manage_the_build_configuration_for_wolfSSL?

Please let us know if anything else comes up.

Thanks,
David Garske, wolfSSL

[byron-hawkins]

If it may be of any use, from a user perspective, when a technical issue comes up in a build, the FAQ is absolutely the last place I would ever look for information. What I would imagine finding in an FAQ are high-level topics about encryption strategies and the various pros and cons of the options available, along with compatibility topics and other such high-level, design-oriented, qualitative, subjective discussion. What would work better for me is any of the following:

1. wherever there is code that absolutely requires a certain preprocessor variable, put in an #error if the variable is not set when compilation reaches that code (e.g., in wc_Chacha_encrypt_64 if WOLFSSL_ARMASM is off). Backward compatibility cannot be a problem because such configurations are totally broken.
2. print a clearly visible message at the end of ./configure that says, in as few words as possible, "this build will not work unless #include "wolfssl/options.h" occurs before any other WolfSSL or WolfCrypt header." Limit the number of words as much as possible, and make sure this message appears at the very end, followed by nothing.
3. put at least #warning if a generated options.h is reached after any other WolfSSL or WolfCrypt header.
4. anywhere there printed the recipe, "./autogen.sh ... ./configure ... make", include a word-minimal statement that (a) there will be an options.h file generated to reflect all options chosen in ./configure, and (b) no usage of the build is possible unless this header is reached before all other WolfSSL and WolfCrypt headers. Never print "./configure" in the documentation without immediately restating this point.

[dgarske]

Hi @byron-hawkins ,

That is excellent feedback! We will make improvements and provide a link to those when done.

Thanks,
David Garske, wolfSSL