AES Wrap function

Question

This code is supposed to implement RFC 3394 which also described in this PDF at page 11.

Are there any weak places or formatting issues in attached source? Or maybe some places are not explicit enough?

void wrap( const unsigned char *key, const unsigned char *plaintext, unsigned int plaintext_length,
 unsigned char *ciphertext, unsigned int *ciphertext_length, unsigned int semiblocksize)
{
 int i, j, k;
 unsigned int n, t, tt;
 unsigned char *A, B[16], *R;
 int targetlevel = 0, p = semiblocksize;
 while (p >>= 1) ++targetlevel;
 n = plaintext_length >> targetlevel;
 A = B;
 memset(A, 0xa6, semiblocksize);
 memcpy(ciphertext+semiblocksize, plaintext, plaintext_length);
 for(j=0, t=1; j<=5; j++)
 {
 for(i=1, R=ciphertext+semiblocksize; i<=n; i++, t++, R+=semiblocksize)
 {
 memcpy(B+semiblocksize, R, semiblocksize);
 AES128_ECB_encrypt(B, key, B);
 
 for(k=semiblocksize - 1, tt=t; (k>=0) && (tt>0); k--, tt>>=semiblocksize)
 {
 A[k] ^= (unsigned char) (tt & 0xFF);
 }
 memcpy(R, B+semiblocksize, semiblocksize);
 }
 }
 memcpy(ciphertext, A, semiblocksize);
 *ciphertext_length = plaintext_length + semiblocksize;
}

I know, that ECB mode should not be used in production, but this is only for ensuring output correctness by checking coincidence with the RFC.

Answer 1

1. Arrays are best indexed with type size_t rather than unsigned. Notice unnecessary mixed int/unsigned types in i<=n. Both should be size_t.

2. Avoid naked magic numbers. Why 16 in unsigned char B[16]?

3. Formatting: For review purposes, consider limiting to the width to the target display.

   // void wrap( const unsigned char *key, const unsigned char *plaintext, unsigned int plaintext_length,
   // unsigned char *ciphertext, unsigned int *ciphertext_length, unsigned int semiblocksize)
   void wrap(const unsigned char *key, const unsigned char *plaintext, 
    unsigned int plaintext_length, unsigned char *ciphertext, 
    unsigned int *ciphertext_length, unsigned int semiblocksize)
   

4. See little value in the vertical line spacing. Removing a few of these blank line to group common elements together would make more sense.

   unsigned char *A, B[16], *R;
   int targetlevel = 0, p = semiblocksize;
   while (p >>= 1) ++targetlevel;
   n = plaintext_length >> targetlevel;
   A = B;
   memset(A, 0xa6, semiblocksize);
   memcpy(ciphertext+semiblocksize, plaintext, plaintext_length);
   for(j=0, t=1; j<=5; j++)
   

5. for() loops are best coded with for (i=first_value; i<some_N; i += delta). Complex usage of for() loops tend to be code golfing. Indexing from 0 is idiomatic in C and should be followed when reasonable. Following this helps convey meaning and avoids off-by-1 errors.

   // for(j=0, t=1; j<=5; j++) {
    // for(i=1, R=ciphertext+semiblocksize; i<=n; i++, t++, R+=semiblocksize)
   t = 0; 
   for(j=0; j<6; j++) {
    R = ciphertext;
    for(i=0; i<n; i++) { 
    R += semiblocksize;
    t++;
    ...
    }
   }
   

6. Variable names A, B convey little meaning unless standard for the encryption algorithm's notation.

7. Cast serves no purpose here in C

    // A[k] ^= (unsigned char) (tt & 0xFF);
    A[k] ^= tt & 0xFF;