buf2coap is reading one byte outside the input buffer

[sijohans]

Hello,

I am looking through this code base and run into an issue. It seems like the buf2coap function is reading one byte outside the input buffer (see https://github.com/Fraunhofer-AISEC/uoscore-uedhoc/blob/main/modules/oscore/src/coap.c#L253).

I ran this function on test data from the test folder, like in oscore_client_test1 (https://github.com/Fraunhofer-AISEC/uoscore-uedhoc/blob/main/test/src/main.c#L589). If checking payload_len prior to the payload marker this does not occur.

The T1__COAP_REQ does not seems to include a payload marker.

I crated my own main file and compiled it in linux environment (without zephyr) with address sanitizer (-fsanitize=address). The same issue can occur earlier in the code since there is no much size checking. I guess the used coap library should have generated a valid CoAP message before this function is called.

[StefanHri]

Thank you for your feedback sijohans!

I tried to address the issue, see https://github.com/Fraunhofer-AISEC/uoscore-uedhoc/blob/0b2a48f569952038d660b517c9108db6eb3ed328/modules/oscore/src/coap.c#L260 and also to add size checking for the other parts of the parsing.

"The T1__COAP_REQ does not seems to include a payload marker." This is correct. T1__COAP_REQ comes von Appendix C4 in RFC8613 and contains only uri-path and uri-host options.

"I guess the used coap library should have generated a valid CoAP message before this function is called." Unfortunately, to require that is possible only in the coap2oscore execution. In the oscore2coap() the packet is received over the network thus we don't know what it contains. Here to check the sizes is very important. Thank you for pointing me to that problem!

If you are satisfied with the changes I will merge the branch in the main and close the issue.

[sijohans]

Hello again,

I saw your branch and i think it is a good start. I locally just made this change on line 253:

while (*tmp_p != 0xFF && payload_len != 0) 

I extended my function to also do the oscore2coap by creating an additional context and i also saw some crashes there.

I think in general when receiving data from the "outside", we need to have very robust interfaces so that malformed data cannot cause undefined behaviour. So, identifying everywhere data from the outside is parsed and look through that piece of code. Fuzz-testing is a very good tool here. It will detect some bugs (like accessing memory outside buffers, hangs in while loops) but not logical issues. I can make a short example on how to use e.g. https://llvm.org/docs/LibFuzzer.html to fuzz the buf2coap function.

In order to fuzz test the coap2oscore or oscore2coap function we will need to add some additional steps (mocking out crypto and hash) to get good fuzzing performance.

I am evaluating coap, oscore and edhoc libraries to use in an IoT project. This code base looks promising :)

[sijohans]

Something like this for fuzzing:

#include <stdint.h>
#include <stddef.h>
#include "coap.h"

/*
 * $ clang -g3 fuzz.c -I../inc -fsanitize=fuzzer,address coap.c memcpy_s.c -o buf2coap.fuzz
 * $ ./buf2coap.fuzz
 */

int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) {

    struct o_coap_packet coap_packet;
    struct byte_array in;
    in.ptr = data;
    in.len = size;

    buf2coap(&in, &coap_packet);

    return 0;  // Non-zero return values are reserved for future use.
}

Will cause crashes e.g. if size == 0.

[sijohans]

Pulled in the changes from sanitizers and started that fuzz test. I get a crash here: https://github.com/Fraunhofer-AISEC/uoscore-uedhoc/blob/0b2a48f569952038d660b517c9108db6eb3ed328/modules/oscore/src/coap.c#L250

This is probably due to line 244 (https://github.com/Fraunhofer-AISEC/uoscore-uedhoc/blob/0b2a48f569952038d660b517c9108db6eb3ed328/modules/oscore/src/coap.c#L244), the payload_len will underflow and be very big. I just tried quick adding this to line 236:

} else if (out->header.TKL <= 8 && payload_len >= out->header.TKL) {

Not is does not crash there, instead in the buf2options function. With this input:

000a043000dc

[StefanHri]

Hi sijohans, I totally agree with you that we need a very robust interface for everything which comes from outside. The approach to use fuzzing to identify places where things can go wrong is very good and I would like to do that not only for OSCORE but for EDHOC too.

Currently, there are few directions for further development, therefore I am very interested to understand what the users are missing most in order to prioritize. Will it be OK for you to have a short chat in the next days? If so drop me a line stefan.hristozov@aisec.fraunhofer.de.

[sijohans]

I played with this a little more and made a fork where i do some evaluation: https://github.com/sijohans/uoscore-uedhoc/tree/evaluation

I did this:

• Created a top level CMakeLists.txt that will build the oscore and edhoc libraries (+ the dependencies)
• Just created a playground file which i used with the test data. Here i get crashes when i run with address sanitizers
• Created a fuzz-testing directory with a fuzz testing script (required libFuzzer and AFL to be installed): https://github.com/sijohans/uoscore-uedhoc/tree/evaluation/fuzz-test

(Just copied some old scripts i had since before) Also, i didn't want to mess with other CMakeFiles related to Zephyr (i dont have this configured).

I made two targets, one fuzzes the function buf2coap and one the function oscore2coap (with init params from test vectors) since these are the two functions that will be parsing a lot of external provided data. To get good fuzz performance on the last one, one would need to mock the crypto stuff.

The script (fuzz.sh) allows you to fuzz using AFL or libFuzzer and will also build a debug target. This is one interesting thing i see when i init oscore params like this:

    struct oscore_init_params server_params = {
        .dev_type = SERVER,
        .master_secret.ptr = T1__MASTER_SECRET,
        .master_secret.len = T1__MASTER_SECRET_LEN,
        .sender_id.ptr = T1__RECIPIENT_ID,
        .sender_id.len = T1__RECIPIENT_ID_LEN,
        .recipient_id.ptr = T1__SENDER_ID,
        .recipient_id.len = T1__SENDER_ID_LEN,
        .master_salt.ptr = T1__MASTER_SALT,
        .master_salt.len = T1__MASTER_SALT_LEN,
        .id_context.ptr = T1__ID_CONTEXT,
        .id_context.len = T1__ID_CONTEXT_LEN,
        .aead_alg = AES_CCM_16_64_128,
        .hkdf = SHA_256,
    };
    struct context c_server;
    OscoreError error = oscore_context_init(&server_params, &c_server);

I get this result:

/home/simonj/Development/uoscore-uedhoc/externals/tinycbor/src/cbor_buf_writer.c:57:5: runtime error: null pointer passed as argument 2, which is declared to never be null
/home/simonj/Development/uoscore-uedhoc/modules/oscore/src/memcpy_s.c:22:3: runtime error: null pointer passed as argument 2, which is declared to never be null

Do you have any plans/ideas for extending the project structure (build system, testing, ...) with more tools and e.g. not using Zephyr?

I also saw the samples stuff which works very well to just build on my linux computer, might add them to the cmake build system for evaluation.

[StefanHri]

Thank you sijohans! I will take a deeper look.

The reason for using Zephyr is that Zephyr comes with compilers for many platforms (ARM/RISC-V/x86/Xtensa) and therefore it is very easy to build the libraries for a different platform.

Do you have any specific build system or testing needs?

[sijohans]

Hello again,

I don't have any specific needs but i like to have a project as "pure" as possible. E.g., one easy build system (i like CMake) and so that it is easy to build on in linux or mac environment. I haven't that much experience of zephyr, but i understand that supporting zephyr is of interest. It also makes it easy to build and test on real targets to test performance etc. I will look into configuring Zephyr to see if i can run unit tests etc.

I added another fuzz test to my fork which tests edhoc message 1 parsing: https://github.com/sijohans/uoscore-uedhoc/blob/evaluation/fuzz-test/fuzz_msg1parse.c Here i am interfacing the edhoc_responder_run function. From a testing perspective it would be a lot easier to also expose the msgX_parse functions. I found a bug here with the fuzz-test:

If giving this as input to edhoc_responder_run when it is reading message 1:

00040ab7

    uint8_t c_i[C_I_DEFAULT_SIZE];
    uint64_t c_i_len = sizeof(c_i);

    r = msg1_parse(msg1, msg1_len, &method_corr, suites_i, &suites_i_len,
               g_x, &g_x_len, c_i, &c_i_len, ad_1, ad_1_len);
    if (r != EdhocNoError)
        return r;

The c_i_len will be 23, but C_I_DEFAULT_SIZE is 8. When the error message is created later:

        r = tx_err_msg(RESPONDER, method_corr, c_i, c_i_len, NULL, 0,
                   c->suites_r.ptr, c->suites_r.len);

Additional 15 bytes after uint8_t c_i[C_I_DEFAULT_SIZE]; will be leaked and put into the error message.

Let me know if you need more information how to setup those tools to run these tests-