Buffer Over-read Affecting dtls_create_cookie()

[bathooman]

Hello again,

This bug occurs when the server receives a malformed Client Hello to initialize a handshake and it affects the master branch. This bug is found by employing symbolic execution technique. Suppose you have a malformed Client Hello where the length field in the handshake layer is 0. When the mentioned Client Hello is received by the server, dtls_verify_peer() calls dtls_create_cookie() for cookie generation. Inside dtls_create_cookie(), TinyDTLS defines e which is unsigned (size_t). Later, when we call the following update function:

dtls_hmac_update(&hmac_context, 
		   msg + DTLS_HS_LENGTH + e,
		   dtls_get_fragment_length(DTLS_HANDSHAKE_HEADER(msg)) - e);

the macro dtls_get_fragment_length(DTLS_HANDSHAKE_HEADER(msg)) will return 0. Therefore, since e is unsigned, dtls_get_fragment_length(DTLS_HANDSHAKE_HEADER(msg)) - e will wrap around and evaluates to a very huge number(e.g 18446744073709551580). This huge value, in turns will cause a memory over-read in the memcpy in sha2.c.
I have attached the means to reproduce the mentioned bug. To do so, after downloading the suite, in tinydtls-master-witness/tests, execute the script setupserver.sh to compile TinyDTLS. I have not changed anything in the code for the sake of this demonstration. Successful execution of setupserver.sh will run the dtls-server on port 20220. Now, executing ./reproduce.sh in tinydtls-master-witness/tests should crash the server as shown in the following figure:

I hope I could explain it understandably and I appreciate your confirmation of the bug.
tinydtls-master-witness.zip

Best,
Hooman Asadian

[mrdeep1]

@bathooman Once again thank you for the detailed analysis of the issue. In this case, this is no longer an issue if you use the develop branch where this has been fixed. There has been a lot of work done between the master and develop branch in handling these boundary conditions.

Please confirm when using the develop branch that this is no longer an issue for you.

[bathooman]

@mrdeep1 I confirm that this bug is resolved in the develop branch. I assume this issue is no longer needed. Or should we leave it open since it still affects master branch.

[mrdeep1]

Thanks for confirming this. As it is unlikely that there will be any bug fixes for the master branch (other than merging in develop at a freeze point), this issue is no longer needed and can be closed (but still can be found if needed).

[bathooman]

Dear @mrdeep1,

I have checked for this bug thoroughly again. buffer over-read can still take place in a different scenario. Suppose you have a normal Client Hello like the following:

If you change the Fragment Length with a value bigger than 42 which is the real Fragment Length and keep everything else unchanged, you will have memory over-read again. Suppose, we have 73 as the Fragment Length. We successfully pass the check on the Fragment Length. However, the e has a value equal to 42 (after we skipped the Handshake Layer, Cipher Suites, Compression Methods, and the Length fields). Now, we call the hmac_update where fragment_length - e = 31. Finally, we will have 29 bytes of over-read when this memcpy is called. The root cause of this problem is the missing check to see if the real length of the received record (msg_len) is consistent with the value of the Fragment Length. In the attached witness, I have added the following assertion before calling the hmac_update so that it is easier to see the root cause:

assert(msglen == fragment_length + sizeof(dtls_handshake_header_t));

I have attached the means to reproduce the mentioned bug. To do so, after downloading the suite, in tinydtls-master-witness/tests, execute the script setupserver.sh to compile TinyDTLS. I have not changed anything in the code for the sake of this demonstration. Successful execution of setupserver.sh will run the dtls-server on port 20220. Now, executing ./reproduce.sh in tinydtls-master-witness/tests should cause the mentioned assertion to fail:

I will respectfully reopen this issue. Please tell me if more information is needed.
tinydtls-develop-witness.zip

Best,
Hooman Asadian

[mrdeep1]

@bathooman The develop branch does not properly support fragmentation - hence the crafted fragmented handshake causes things to fall over. I will create a temporary holding fix for the develop branch to reject fragmented handshakes until the feature/handshake_fragmentation branch is merged into develop.

[mrdeep1]

#63 raised for feature/handshake_fragmentation branch
#64 raised for develop branch as interim fix before feature/handshake_fragmentation branch is merged.

[bathooman]

Unfortunately, PR #64 will not solve the issue. Suppose we have the following malformed Client Hello:

The value for length and Fragment Length in the Handshake Protocol is 70 while the real value should be 42. With the mentioned values, we pass the new check. Later, when TinyDTLS is generating a cookie, we use Fragment Length here which causes buffer over-read. I have added the following assertion to make it easier to see the problem:

assert(msglen == fragment_length + sizeof(dtls_handshake_header_t));

I have attached the means to reproduce the mentioned bug. To do so, after downloading the suite, in tinydtls-fixes_59b/tests, execute the script setupserver.sh to compile TinyDTLS. I have not changed anything in the code for the sake of this demonstration. Successful execution of setupserver.sh will run the dtls-server on port 20220. Now, executing ./reproduce.sh in tinydtls-fixes_59b/tests should cause the mentioned assertion to fail:

tinydtls-fixes_59b.zip

Best,
Hooman

[mrdeep1]

This should now be fixed by the updated #64.

[bathooman]

Dear @mrdeep1

PR #64 will solve the problem. It also solves the non-conformance bug regarding the Fragment Offset and Fragment Length. As RFC 6347 in section 4.2.3 states:

An unfragmented message is a degenerate case with fragment_offset=0 and fragment_length=length.

PR #64 also enforces this requirement.

Regards,
Hooman