intel / tinycrypt Goto Github PK

It appears to me that tc_ctr_prng_reseed() in ctr_prng.c has a bug(). I believe the following:
/* 10.2.1.4.1 step 4 */
tc_ctr_prng_update(ctx, entropy);

Should be:
/* 10.2.1.4.1 step 4 */
tc_ctr_prng_update(ctx, seed_material);

Bug in the AES-CCM code. The “sanity” check for (plen <= alen) is incorrect. Total message length is not passed in, in this implementation plen and alen are independent. This is a serious problem that causes decryptions to incorrectly fail.

Hi, I'm Jakub from TrustInSoft, an advanced source code analyzer publisher for C and C++. I set up TrustInSoft CI on your tests: https://ci.trust-in-soft.com/projects/jakub-zwolakowski/tinycrypt/

I have found some issues in your code. Here you can check out the issues in TrustInSoft CI (first link in each line) and see your source code for reference (second link in each line):

1) Signed overflow (due to implicit integer conversion) in file 'lib/source/aes_encrypt.c' line 86
2) Invalid memory access (out of bounds read, as a consequence of the wrong length given in the test) in file 'lib/source/utils.c' line 44
3) Invalid pointer arithmetic (pointer arithmetic inside object, computing &array[-1]) in file 'lib/source/cmac_mode.c' line 89
4) Another signed overflow (due to implicit integer conversion) in file 'lib/source/ctr_mode.c' line 61
5) Another invalid pointer arithmetic (inside object, computing &array[-1]) in file 'lib/source/ecc.c' line 238
6) Uninitialized memory (read) in file 'lib/source/sha256.c' line 174

Can you let me know if you find those findings interesting? Would you assess them as harmless or dangerous?

TrustInSoft CI is a new tool based on formal methods. We're currently testing it on Open Source projects so any of your feedback will be greatly appreciated.

set_encrypt_key assumes that unsigned int is at least 32bits. It should I think be using either uint32_t or better yet uint32_fast_t.

SHA256 is also riddled with the same flaw

I suggest to reuse a higher level build system than your current small make file so that powerful checks for software features will become easier.

• CMake
• Autotools

If the tc_cmac_update() function is called and fills the internal leftover cache completely, the following tc_cmac_final() call will produce a wrong token.

If the tc_cmac_update() is called with a data length which fills up the internal leftover cache completely, the leftover data will be processed instantly and is left empty. This is not the right behavior, because tc_cmac_final() requires that the last block is still in the leftover cache and not processed, because it need special treatment.

See pull request for fix and further information: #52

I would like to point out that identifiers like "__TC_AES_H__" and "__TC_UTILS_H__" do eventually not fit to the expected naming convention of the C++ language standard.
Would you like to adjust your selection for unique names?

Lines 50..52 of contants.h try to define bool type as follows:

#ifndef bool
enum {false, true} bool;
#endif

There are several issues with the above code, that causes multiple definitions of the same symbol linker error on some strict linkers.

1. The above definition, besides it defines the two unnamed enum elements treu and false, defines a variable bool. The right way would be using typedef keyword
2. The defined variable bool does not define the pre-processor symbol bool, so it does not make sense to enclose the definition

It is probably better to either include `stdbool.h' or copy its content:

#ifndef bool
#ifndef __cplusplus
#define bool   _Bool
#define true   1
#define false  0
#endif
#endif

PS: This issue was open du to https://gerrit.zephyrproject.org/r/#/c/10848

IS this is an error：
tests/test_cbc_mode.c at line 135:
length = ((unsigned int) sizeof(encrypted));
Should it be the following code：
length = ((unsigned int) sizeof(decrypted));

Hello. I am working in a project to create a library, and this library includes Tinycript within it to provide security functionality.

One issue that we have run into is that the Tinycript library causes linking conflicts with other security libraries (the specific one we are having trouble with is nrf crypto, as we are doing embedded development on the nRF52840 board).

Would it be possible to change the source file names of the Tinycript library to be explicitly Tinycript specific? (i.e., prepend all of them with tc_ or something similar)

It seems that this is already done with the functions within Tinycript, and if it was also done to the source file names it would prevent linking conflicts in certain IDE's like Segger Embedded Studio.

I am vaguely aware that this issue can be solved by changing the names of the object files that the linker generates for the source files that are conflicting, but this solution would make it harder for other users to use our library. I think we could also change the names of the files in the local copy of Tinycript in our library, but that seems like it would make updating Tinycript for our library a hassle in the future.

I am also not sure if this is a design choice on purpose, in order to prevent unnecessary duplicate security implementations that are provided by other security libraries, but it seems like it would still just be more flexible for users of the Tinycript library if they had Tinycript specific source file names.

Thank you very much for reading, and thank you for the nice library!

The rest of the code goes to great pains to use _set() but ecc_dh instead dives into GNUisms, not only that but it dives into non archicture portable GNUisms.

D:\workspace_C>git clone https://github.com/intel/tinycrypt
Cloning into 'tinycrypt'...
remote: Enumerating objects: 698, done.
Receiving objects: 100% (698/698), 891.88 KiB | 2.55 MiB/s, done.

Resolving deltas: 100% (458/458), done.

D:\workspace_C>cd tinycrypt

D:\workspace_C\tinycrypt>ls
AUTHORS documentation LICENSE README VERSION
config.mk lib Makefile tests

D:\workspace_C\tinycrypt>make
make -C lib
make[1]: Entering directory D:/workspace_C/tinycrypt/lib' gcc -Os -std=c99 -Wall -Wextra -D_ISOC99_SOURCE -MMD -I../lib/include/ -I../lib/source/ -I../tests/include/ -DENABLE_TESTS -c -o aes_decrypt.o ../lib/source/aes_decrypt.c process_begin: CreateProcess(NULL, gcc -Os -std=c99 -Wall -Wextra -D_ISOC99_SOURCE -MMD -I../lib/include/ -I../lib/source/ -I../tests/include/ -DENABLE_TESTS -c -o aes_decrypt.o ../lib/source/aes_decrypt.c, ...) failed. make (e=2): 系統找不到指定的檔案。 make[1]: *** [aes_decrypt.o] Error 2 make[1]: Leaving directory D:/workspace_C/tinycrypt/lib'
make: *** [all] Error 2

Hi,

I want to use uECC_shared_secret() to derive a shared secret. I have only the x coordinate of the public key. I assume the function uECC_shared_secret() requires the uncompressed key holding both coordinates {x,y}. How to decompress the public key? I found the following function in uECC link, however, this function appears to not be part of tinycrypt.

https://github.com/jsakkine-intel/sgx-le/issues/1

The description of the in parameter of tc_cbc_mode_decrypt is misleading. All examples show that in should point on the cipher text offset by the size of the iv (which is a block):
https://github.com/intel/tinycrypt/blob/master/tests/test_cbc_mode.c#L134 :

p = &encrypted[TC_AES_BLOCK_SIZE];
	length = ((unsigned int) sizeof(encrypted));

	if (tc_cbc_mode_decrypt(decrypted, length, p, length, encrypted, &a) == 0) {
...

But both the note and the description of the in param are suggesting that it should point on the cipher text including the IV:
https://github.com/intel/tinycrypt/blob/master/lib/include/tinycrypt/cbc_mode.h#L128 :

* @note Assumes:- in == iv + ciphertext, i.e. the iv and the ciphertext are
 *                contiguous. This allows for a very efficient decryption
 *                algorithm that would not otherwise be possible

* @param in IN -- ciphertext to decrypt, including IV

I'd propose to modify the doc to something like:

* @note Assumes:- the IV and the ciphertext need to be
 *                contiguous. This allows for a very efficient decryption
 *                algorithm that would not otherwise be possible

* @param in IN -- ciphertext to decrypt, not including IV
* @param iv IN -- the IV for the encrypt/decrypt, must be followed by ciphertext

https://safecurves.cr.yp.to/

This issue is born out of real-world usage, from the Zephyr project where work is ongoing porting to new platform and compliance cleanup.

A VLA is used in tinycrypt/source/hmac.c :: tc_hmac_set_key(), dummy_key. This is unfortunate for a number of reasons, listed later below.

My understanding:
It appears that dummy_key is written when key_size <= TC_SHA256_BLOCK_SIZE and timing between the two branches should be nearly the same to cover up the sidechannel mentioned therein. Normally, key_size == TC_SHA256_BLOCK_SIZE and dummy_key will be written and thrown away. That makes sense, as we only want the timing behavior from tc_sha256_* in the then-branch. Fine. Call to rekey is what matters.

Request:

• Have dummy_key statically sized, to the worst-case (= the usual case) size of TC_SHA256_BLOCK_SIZE as per the patch below.
• Nice to have: Limit scoping of dummy_key and dummy_state to the then-branch.

This patch should improve safety too, as 1) the total stack frame usage can now better be analyzed, 2) some compilers may ignore that dummy_key is only written guarded by key_size <= TC_SHA256_BLOCK_SIZE since it is declared in the scope outside. So 2 means that stack overflow could happen if key_size was big, even if guarded.

Variable length arrays have multiple issues:

1. Insecure: No way to check for stack overflow. Different platforms have different stack sizes. tinycrypt is very appealing for embedded platforms, yet many of these have very limited stack sizes.
2. Exists only in C99. VLAs were made optional in C11.
3. Limits portability.

Patch:

diff --git a/ext/lib/crypto/tinycrypt/source/hmac.c b/ext/lib/crypto/tinycrypt/source/hmac.c
index 89878cec7..6e65ae717 100644
--- a/ext/lib/crypto/tinycrypt/source/hmac.c
+++ b/ext/lib/crypto/tinycrypt/source/hmac.c
@@ -63 +63 @@ int tc_hmac_set_key(TCHmacState_t ctx, const uint8_t *key,
-       const uint8_t dummy_key[key_size];
+       const uint8_t dummy_key[TC_SHA256_BLOCK_SIZE];

I'll also be happy to submit a PR.

The function uECC_sign_with_k() applies side-channel resistance to k by multiplying k with a random number "tmp". This is only applied if g_rng_function is set. unfortunately, g_rng_function is never set. The reason is:

• g_rng_function in ecc_dsa.c is a different variable than g_rng_function in ecc.c.
• When compiling ecc_dsa.c, <tinycrypt/ecc_platform_specific.h> is not included, the copy of g_rng_function points to NULL
• if g_rng_function is NULL, the side channel resistance is disabled

ECC implementation is not compliant to FIPS 186. Appendix B.4 allows either generating 64 extra bits and computing c=random mod n, or choosing random bit strings len(n) until a value less than n-2 is found and used as c. This function is only compliant if it fails for a random bit string with integer value less than n-2 is passed in.