CodeReview: Tiny Encryption Algorithm for arbitrary sized data

Question

The TEA is a very simple encryption algorithm requiring little time and space - perfect for embedded systems. There are extensions to it, and every version has its flaws (WEP was based on it), but for casual protection it's perfect.

In the vein of this topic on code review, I'm posting my code for critique. Interestingly, when I decided to do this I added more comments and cleaned up the code so I wouldn't be too embarrassed to post it - so just the act of asking for a code review has already yielded improvements I would not otherwise have made, which doubtless will help me down the road (especially as this module is designed for re-use).

I'm posting just the two files of interest, but if you want I'll post the test program as an answer (gives examples of usage) - I just don't want this question to be too long.

Also, the encrypt and decrypt routines are exactly as found from Wikipedia, so I don't expect anyone to check that I'm following the TEA correctly - I'm more interested in the block routines I've added, though if you notice something about the TEA routines it would be good to know.

========== tea.h ==========

#ifndef __TEA.H__
#define __TEA.H__

#include <stdint.h>

void encrypt (uint32_t* v, uint32_t* k);
void decrypt (uint32_t* v, uint32_t* k);
void encryptBlock(uint8_t * data, uint32_t * len, uint32_t * key);
void decryptBlock(uint8_t * data, uint32_t * len, uint32_t * key);

#endif

========== tea.c ==========

#include "tea.h"
/* encryptBlock
 *   Encrypts byte array data of length len with key key using TEA
 * Arguments:
 *   data - pointer to 8 bit data array to be encrypted - SEE NOTES
 *   len - length of array
 *   key - Pointer to four integer array (16 bytes) holding TEA key
 * Returns:
 *   data - encrypted data held here
 *   len - size of the new data array
 * Side effects:
 *   Modifies data and len
 * NOTES:
 * data size must be equal to or larger than ((len + 7) / 8) * 8 + 8
 * TEA encrypts in 8 byte blocks, so it must include enough space to 
 * hold the entire data to pad out to an 8 byte boundary, plus another
 * 8 bytes at the end to give the length to the decrypt algorithm.
 *
 *  - Shortcut - make sure that data is at least len + 15 bytes in size.
 */
void encryptBlock(uint8_t * data, uint32_t * len, uint32_t * key)
{
   uint32_t blocks, i;
   uint32_t * data32;

   // treat the data as 32 bit unsigned integers
   data32 = (uint32_t *) data;

   // Find the number of 8 byte blocks, add one for the length
   blocks = (((*len) + 7) / 8) + 1;

   // Set the last block to the original data length
   data32[(blocks*2) - 1] = *len;

   // Set the encrypted data length
   *len = blocks * 8;

   for(i = 0; i< blocks; i++)
   {
      encrypt(&data32[i*2], key);
   }
}

/* decryptBlock
 *   Decrypts byte array data of length len with key key using TEA
 * Arguments:
 *   data - pointer to 8 bit data array to be decrypted - SEE NOTES
 *   len - length of array
 *   key - Pointer to four integer array (16 bytes) holding TEA key
 * Returns:
 *   data - decrypted data held here
 *   len - size of the new data array
 * Side effects:
 *   Modifies data and len
 * NOTES:
 *   None
 */
void decryptBlock(uint8_t * data, uint32_t * len, uint32_t * key)
{
   uint32_t blocks, i;
   uint32_t * data32;

   // treat the data as 32 bit unsigned integers
   data32 = (uint32_t *) data;

   // Find the number of 8 byte blocks
   blocks = (*len)/8;

   for(i = 0; i< blocks; i++)
   {
      decrypt(&data32[i*2], key);
   }

   // Return the length of the original data
   *len = data32[(blocks*2) - 1];
}

/* encrypt
 *   Encrypt 64 bits with a 128 bit key using TEA
 *   From http://en.wikipedia.org/wiki/Tiny_Encryption_Algorithm
 * Arguments:
 *   v - array of two 32 bit uints to be encoded in place
 *   k - array of four 32 bit uints to act as key
 * Returns:
 *   v - encrypted result
 * Side effects:
 *   None
 */
void encrypt (uint32_t* v, uint32_t* k) {
    uint32_t v0=v[0], v1=v[1], sum=0, i;           /* set up */
    uint32_t delta=0x9e3779b9;                     /* a key schedule constant */
    uint32_t k0=k[0], k1=k[1], k2=k[2], k3=k[3];   /* cache key */
    for (i=0; i < 32; i++) {                       /* basic cycle start */
        sum += delta;
        v0 += ((v1<<4) + k0) ^ (v1 + sum) ^ ((v1>>5) + k1);
        v1 += ((v0<<4) + k2) ^ (v0 + sum) ^ ((v0>>5) + k3);  
    }                                              /* end cycle */
    v[0]=v0; v[1]=v1;
}

/* decrypt
 *   Decrypt 64 bits with a 128 bit key using TEA
 *   From http://en.wikipedia.org/wiki/Tiny_Encryption_Algorithm
 * Arguments:
 *   v - array of two 32 bit uints to be decoded in place
 *   k - array of four 32 bit uints to act as key
 * Returns:
 *   v - decrypted result
 * Side effects:
 *   None
 */
void decrypt (uint32_t* v, uint32_t* k) {
    uint32_t v0=v[0], v1=v[1], sum=0xC6EF3720, i;  /* set up */
    uint32_t delta=0x9e3779b9;                     /* a key schedule constant */
    uint32_t k0=k[0], k1=k[1], k2=k[2], k3=k[3];   /* cache key */
    for (i=0; i<32; i++) {                         /* basic cycle start */
        v1 -= ((v0<<4) + k2) ^ (v0 + sum) ^ ((v0>>5) + k3);
        v0 -= ((v1<<4) + k0) ^ (v1 + sum) ^ ((v1>>5) + k1);
        sum -= delta;                                   
    }                                              /* end cycle */
    v[0]=v0; v[1]=v1;
}

Issues so far:

Coding

• Using magic numbers in encrypt and decrypt routines - use #defines instead - Kyle
• If the 64 bit encoding functions aren't used outside this module, their prototypes should be in the code, not header - Simon
• Add sanity checking to input - Rob
• Require that input len is a multiple of 8 bytes - making a requirement we can't enforce or check is a recipe for corruption - Rob

Style

• Using K&R brace style in some places, ANSI in others - consistency - Simon
• The long descriptive comment is more useful for the end user if it's in the header file (Header for usage, code for implementation) - Rob

Answer 1

I'm not familiar with your algorithm, but in glancing over your code, it looks like sum and delta are magic numbers. Since delta is shared between encrypt and decrypt, it might be a good idea to #define them at the top of the file.

edit: I just looked at the Wikipedia article and the magic numbers make an appearance in the reference implementation, which has been copied verbatim into your code.

Answer 2

Without looking too closely, I'm struggling to see any major problems with this code. Here are a couple of trivial issues:

1. Did you mean to use different brace styles? The block functions use (sort of) K&R braces and the 64-bit functions use ANSI braces.
2. Are the 64-bit functions ever to be used outside the module? If not, I would move their prototypes to the .c file.

Answer 3

You should add some sanity checking on the incoming arguments, especially if you intend this to be library code used from multiple projects.

I would recommend you change definition of encryptBlock to require that len is a multiple of 8 bytes. Requiring that the incoming array is sized to a multiple of 8 bytes, but not enforcing that for len is a sure fire way to get memory corruption when someone forgets (e.g. when they are encrypting strings).

Personally I prefer to have the long descriptive comment in the header file rather than the .c file. Anyone using tea.h will find it more useful there.

Answer 4

I'm actually the opposite if of Rob Walker in that I prefer there to be a nice comment block preceding the functions in the code file as opposed to the header. However, as long as you are constant in where you put your comments, most people shouldn't have an issue unless it goes against your company's manual of style. The only other thing I can think of in terms of style would be a comment block at the start of the files with some general information, for example:

/*-----------------------------------------------------------------------------
 * File      - 
 * Author(s) - 
 *
 * Purpose   -
 *
 *----------------------------------------------------------------------------- 
 * Notes     - 
 *-----------------------------------------------------------------------------
 * Revisions - 
 *---------------------------------------------------------------------------*/

Sometimes you will also see copyright information in these blocks as well. This is just an example of what one looks like though.

In terms of the code itself, I have to agree with the others in regards to putting the "magic numbers" off to the side and making sure they are clearly labeled as to what they do. Ditto for the sanity checking if this is going to be used as part of a library.