This is more a style problem, but also a question about the design of the function entropy_to_mnemonic in mnemonics.c.

A variable (const struct dictionary *dict) is declared and used in the code, but it is only a copy of one of the parameter (const struct dictionary *dictionary).

During the function, the parameter is replaced by another local variable. However, this new function seems to be useless, because the variable in parameter (const struct dictionary *dictionary) can do the same job.

Current code using (const struct dictionary *dict) :

    const struct dictionary *dict = dictionary;

    if (dictionary == NULL) {
        dict = default_EN_dictionary();
    }

        if (len + strlen((char*) (dict->words[joint])) + 1 >= capacity) {
            capacity *= 2;
            unsigned char *temp = realloc(out, capacity);
            if (temp == NULL) {
                free(bytes);
                free(out);
                return EC_ALLOCATION_ERROR;
            }
            out = temp;
        }
        sprintf((char*) out + len, "%s", dict->words[joint]);
        len += strlen((char*) (dict->words[joint]));

It is not clear why this new variable is created. In addition, the program works also if we do not use this dict variable, and if we only use (const struct dictionary *dictionary) :

    if (dictionary == NULL) {
        dictionary = default_EN_dictionary();
    }

        if (len + strlen((char*) (dictionary->words[joint])) + 1 >= capacity) {
            capacity *= 2;
            unsigned char *temp = realloc(out, capacity);
            if (temp == NULL) {
                free(bytes);
                free(out);
                return EC_ALLOCATION_ERROR;
            }
            out = temp;
        }
        sprintf((char*) out + len, "%s", dictionary->words[joint]);
        len += strlen((char*) (dictionary->words[joint]));

Finally, during the compilation and according to the options and optimization, it is possible that this variable (const struct dictionary *dict) would be automatically deleted.

Is there a particular reason why this local variable (const struct dictionary *dict) has been used ?

The project name should be PA193..., we missed the "A" :(

With the file example.c, it is not possible to use the functionality "-e entropy_file", because the return of the function entropy_to_mnemonic is not well treated.

Indeed, according to the documentation of the function entropy_to_mnemonic in mnemonics.h, and according to this associated source code in mnemonics.c the function returns the

length of mnemonic phrase in characters or negative error code in case of failure

However, in the example.c file, in the function convert_entropy_to_mnemonics, the variable return_code is compared to 0 :

/* Converting entropy to a mnemonic phrase using the default dictionary. */
    return_code = entropy_to_mnemonic(NULL, entropy, entropy_len, &mnemonic);

    if (return_code != 0) {
        printf("Error: Could not convert entropy.\n");
        return;
    }

This means that, if the entropy_to_mnemonic function returns the correct output (the length of the mnemonic phrase), then the program stops and prints that an error occurred.

One of the possible patch would be to change the error return check :

if (return_code != 0) {

by

if (return_code <= 0) {

in example.c

This is just a comment about some functions which take pointer in parameter and return nothing.

void function (type * parameter_1, ...){
  /* Code */
}

First, these functions do not check if the parameter is NULL or not. Even if these functions are not called with NULL parameter by the internal function of the API, these functions are dangerous because a NULL pointer in parameter is always possible.
So, the value of the pointer should be check at the beginning of these functions.

Then, if the value of the pointer in parameter is NULL, the function should stop and return an error. These functions should return an integer, according to errorcodes.h, so, EC_NULL_POINTER.

Finally, after each call to these functions, the return value (the error) should be checked, and execution flow should change according to the error.

This is in general a good practice, here in this project, it is not a problem because it was designed and implemented in such way that these error checks are not necessary (it works without).

However, if someone call an internal dangerous function of the API (a function which take pointer(s) in parameter and return nothing), then it can crash the developer program.

We can find in the following part of the issue the different function which are dangerous regarding the described problem.

• In example.c :

print_hex function

void print_hex(unsigned char *bytes, size_t len) {
    for (size_t i = 0; i < len; i++) {
        printf("%02x", bytes[i]);
    }
}

(Also present in playground.c, print_hex)

convert_entropy_to_mnemonics function

void convert_mnemonic_to_entropy(char *mnemonics_file_name) {
  /* Code */
}

convert_mnemonics_to_seed function

void convert_mnemonics_to_seed(char *mnemonics_file_name) {
  /* Code */
}

convert_mnemonic_to_entropy function

void convert_mnemonic_to_entropy(char *mnemonics_file_name) {
  /* Code */
}

• In mnemonics.c :

array_128_pad function

void array_128_pad(unsigned char array[128], unsigned char pad) {
    for (uint8_t i = 0; i < 128; i++) {
        array[i] ^= pad;
    }
}

array_64_xor function

void array_64_xor(uint8_t dst[64], uint8_t src[64]) {
    for (uint8_t i = 0; i < 64; i++) {
        dst[i] ^= src[i];
    }
}

[PA193 Aller team here]

The function mnemonic_to_entropy_seed can be invoked in such a way, that it produces an infinite recursion.

While we've investigated the function further it seems, that the function mnemonic_to_seed should have probably be used instead of the recursive call to back to mnemonic_to_entropy_seed at this line.

The function mnemonic_to_entropy_seed is not mentioned in the README.md, however, it is exportable and documented in mnemonics.h, therefore anyone can get the feeling it can be used.

I'll add a new branch (in my fork) with a new test case, showing the bug (understandably, it leads to segmentation fault).

I'm not proposing any specific fix at the moment, because it can either be fixed or the function mnemonic_to_entropy_seed might get removed altogether (it does not add any functionality it just adds a bug now).

This issue is severe in the sense, that it can crash the whole application, that will call the function in question. The call to the function (as you can see in the branch, that will be linked soon) is not even improper one, it is called with valid arguments.

I've noticed a few issues with the use word list. To make it short, it's possible to use an altered word list leading to incoherent mnemonics from the entropy. Here are a few examples of what is possible and should probably be changed to avoid any user misuse.

To show the misuses, I'm using the ./mnemonicsplay with the following changed lines:

27 +     parse_dict_from_file("./english.txt", &dict);
30 -     entropy_to_mnemonic(NULL, input_entropy, TEST_VECTOR_LEN, &mnemonic);
30 +     entropy_to_mnemonic(&dict, input_entropy, TEST_VECTOR_LEN, &mnemonic);

So for the normal default English word list, get get the following:

f585c11aec520db57dd353c69554b21a89b20fb0650966fa0a9d6f74fd989d8f
void come effort suffer camp survey warrior heavy shoot primary clutch crush open amazing screen patrol group space point ten exist slush involve unfold
01f5bced59dec48e362f2c45b5de68b9fd6c92c6634f44d6d40aab69056506f0e35524a518034ddc1192e1dacd32c1ed3eaa3c3b131c88ed8e7e54c49a5d0998
f585c11aec520db57dd353c69554b21a89b20fb0650966fa0a9d6f74fd989d8f
OK

For the same word list with the line amazing\n replaced by \n, we get the right mnemonic short this word:

f585c11aec520db57dd353c69554b21a89b20fb0650966fa0a9d6f74fd989d8f
void come effort suffer camp survey warrior heavy shoot primary clutch crush open  screen patrol group space point ten exist slush involve unfold
75034a79cab52fe55169aca1a4b0692a563f00cb9241c8a99826d85c5b5dec3de43f29dd7fd5210da8e8dcb52eda3ab18ef9787a7166b3e5c164009f6f23667d
f585c11aec520db57dd353c69554b21a89b57630650966fa0a9d6f74fd989d8f
OK

For the same word list with the line amazing\n replaced by " \n" (spaces), we get a segmentation fault:

zsh: segmentation fault (core dumped)  ./mnemonicsplay

Replacing the whole word list by 2048 \n, we get an empty mnemonic:

f585c11aec520db57dd353c69554b21a89b20fb0650966fa0a9d6f74fd989d8f

36daaad268151d3be7d4dac76195b041e9bc4bf7756096a654fc1fa9180ca38671de17c177a635c73a6d219ed6d403ca57a25c6dd4a09f2409eedcfa7a773d50
3ac758eb1d63ac758eb1d63ac758eb1d63ac758eb1d63ac758eb1d63ac758eb1
OK

Replacing the word list by 2048 bug\n:

f585c11aec520db57dd353c69554b21a89b20fb0650966fa0a9d6f74fd989d8f
bug bug bug bug bug bug bug bug bug bug bug bug bug bug bug bug bug bug bug bug bug bug bug bug
e7bbc02cab06c738bf2de71f7bce474e38cb20c3fbad7d0ddd0fc532c3052a7a6c63da210e9c4d5c62cf5b42e7196e683e988ca5f011f74010cad50865345194
f99f33e67dcf99f33e67ccf99f33e67dcf99f33e67ccf99f33e67dcf99f33e67
OK

It should test that no word is twice in the list.

The function entropy_to_mnemonic does allocate bytes on line 60, however, few lines further, if the function append_sha256_bytes was to fail, the memory pointed to by bytes will not be freed.

The user controls both inputs to the function append_sha256_bytes, so one potential way is to find a inputs, that might crash it. Due to previous checks on those inputs the failure might be hard or impossible. But, we can get the function append_sha256_bytes to fail also by patching the OpenSSL functions.

Proof of concept will be soon attached to this thread.

The GCC compiler displays warning during the example.c compilation about the function getopt used in the function check_flags.

On gcc (Ubuntu 7.4.0-1ubuntu1~18.04.1) 7.4.0 :

$ gcc -Wall -Wextra --std=c11 -pedantic mnemonics.c dictionary.c example.c -o example -lcrypto
example.c: In function ‘check_flags’:
example.c:26:20: warning: implicit declaration of function ‘getopt’; did you mean ‘getgid’? [-Wimplicit-function-declaration]
     while ((flag = getopt(argc, argv, "hemc")) != -1) {
                    ^~~~~~
                    getgid

The man page indicates that only #include <unistd.h> is needed to import this function.
In addition, according to the man page, #include <getopt.h> should only be included if we use getopt_long or getopt_long_only, which are not used in the example.c source code.

Hence, the documentation is perhaps misleading. In order to fix this include problem, adding the #include <getopt.h> preprocessor instruction at the beginning of the example.c file is enough.