at_c: discussion on atchops function signatures

[JeremyTubongbanua]

Synopsis

The problem is best explained through an example.

Take atchops_aesctr_encrypt function signature as an example:

int atchops_aesctr_encrypt(const unsigned char *key, const enum atchops_aes_size keybits, unsigned char *iv,
                           const unsigned char *plaintext, // plaintext to encrypt
                           const size_t plaintextlen,
                           unsigned char *ciphertext,   // allocated buffer to populate
                           const size_t ciphertextsize, // number of total bytes allocated in the buffer
                           size_t *ciphertextlen        // written actual length in the buffer
)

This is how usage of this function would typically look like:

unsigned char key[32]; // assume populated
unsigned char iv[16]; // assume populated

unsigned char *plaintext = "Hi\n";
const size_t plaintextlen = strlen(plaintext);

const size_t ciphertextsize = atchops_aesctr_ciphertext_size(plaintextlen); // Plaintext is 4 bytes, but is rounded to 16 bytes because of padding. Therefore, by definition of AES, the expected ciphertextsize is 16.
unsigned char ciphertext[ciphertextsize];
size_t ciphertextlen = 0;

atchops_aesctr_encrypt(key, ATCHOPS_AES_256, iv, plaintext, plaintextlen, ciphertext, ciphertextsize, &ciphertextlen);

Issues with Code above

1. ciphertextlen is a completely useless variable because it will ALWAYS equal ciphertextsize as long as atchops_aesctr_ciphertext_size is used to calculate ciphertextsize.
2. if the output is calculatable, then why are we letting the programmer manage the memory? the function should be able to handle this for you if the allocation space is predictable and the function can be optimal in allocating memory for you.

My Proposal

Change atchops_aesctr_encrypt function signature to:

int atchops_aesctr_encrypt(const unsigned char *key, const enum atchops atchops_aes_size keybits, unsigned char *iv,
    const unsigned char plaintext, const size_t plaintextlen,
    unsigned char **ciphertext);

Documentation of this function would mention a couple of important notes:

1. "ciphertext is allocated by the function and freed by the caller of the function."
2. the length of ciphertext is always plaintextlen rounded up the next 16 bytes.

[JeremyTubongbanua]

As I wrote this, I decided it isn't worth doing for two reasons (I had wrote everything out already anyways so thought I would just post it):

1. when dealing with bytes, it might be beneficial to have the programmer manage the memory themselves. Since we are returning bytes, we don't null-terminate the return. But if the caller wanted it to be null-terminated (for whatever reason) they would have it implemented like this:

   
   const size_t ciphertextsize = atchops_aesctr_encrypt_size(plaintextlen) + 1
   unsigned char ciphertext[ciphertextsize];
   size_t ciphertextlen = 0;

// ...

atchops_aesctr_encrypt(key, ATCHOPS_AES_256, iv, plaintext, plaintextlen, ciphertext, ciphertextsize, &ciphertextlen);

// At this point in code, ciphertextlen == ciphertextsize - 1

2. If the plaintextlen was 4 bytes, then it is padded to 16 bytes, then the ciphertext would also be 16 bytes (by definition of AES) . The caller could easily make the mistake and expect `ciphertext` to be of length `4` without knowledge of how AES works or may skip reading the documentation altogether causing in making a mistake like this.

[JeremyTubongbanua]

3. Another good reason is when doing a decryption:

ciphertextlen would be something like 16 bytes (but actually be 4 bytes, where the other 12 bytes are padding)

Then we can simply return length as ciphertextlen == 4 and not have to touch or do anything to the 12 pad bytes.