Constructive criticism, suggestions etc

**BobS0327** · 01-07-2006

I have attached code to a conversion routine. It just seems to me that the conversion can be written
more efficiently. It just doesn't look like the best way to do the conversion. So, I have a few questions:
1. Even though the code functions properly, is there another more "efficient" way to accomplish this type of conversion?
2. If not, can the code itself be more "efficiently" coded?
3. Is there any way to do the conversion without the need for a 16 bit array?
4. Can the 36 byte customer key be compressed to something more reasonable such as 10 or 12 bytes?
5. Am I totally ANSI C compliant on the code?
Info about the routine:
Input to Rijndael encryption is 09142006 (An 8 byte text string, MMDDYYYY)
Output from Rijndael encryption ( 16 byte hex values, essentially gibberish):
HEX
ffffffaa
42
ffffffd6
51
13
35
ffffffb7
ffffffe8
fffffff4
ffffffcd
47
ffffffa8
4b
ffffffb0
ffffff9c
ffffffa2

Convert the above encryption output to a 36 byte readable string
to be used as a key. Generated key from Rijndael encryption would be:
56422a51133549180c3347584b50645ec5eb (key to be given to customer)
First 32 bytes are the encryption data, last 4 bytes is the
bit array identifying which bytes are NEGATIVE or POSITIVE
from the above encryption data. In this example c5eb is the bit array
which is a 16 bit array.

Code:

#include <windows.h>
#include <stdio.h>
#include <string.h>
#include "Rijndael.h"

extern char *RijndaelEncrypt( char *input );
extern char *RijndaelDecrypt( char *input );
extern int GetBit( unsigned char *sp, size_t bitpos );
extern int SetBit( unsigned char *sp, size_t bitpos, int value );

#define NEGATIVE 1
#define POSITIVE 0
#define BUFFEROVERFLOW -1

char *CreateHexString( char *input )
{
    static char output[48] = {0};
    char temp[4] = {0};
    unsigned char array[4] = {0};
    int x;
    int y = 0;
    memset(output, 0, sizeof output);
    for(x = 0; x < 16; x++)
    {
        if(input[x] < 0 )
        {
            if(SetBit( array, x, NEGATIVE ) == BUFFEROVERFLOW)
                return NULL;
            sprintf(temp, "%02x",abs(input[x]));
            memcpy(output+y,(void *)temp,2);
            y += 2; 
        }
        else
        {
            if(SetBit( array, x, POSITIVE) == BUFFEROVERFLOW )
                return NULL;
            sprintf(temp, "%02x",input[x]);
            memcpy(output+y,(void *)temp,2);
            y += 2;  
        }
    }
    sprintf(temp, "%02x",array[0]);
    memcpy(output+y,( void * )temp,2);
    sprintf(temp, "%02x",array[1]);
    y += 2;
    memcpy(output+y,( void * )temp,2);
    return ( char * ) output;
}

char *ConvertHexString( char *input )
{
    static char output[48] = {0};
    unsigned char array[2] = {0};
    int accum = 0;
    int x;
    int y = 0;
    int z = 0;
    memset(output, 0, sizeof output);
    if(toupper(input[32]) > 64  && toupper(input[32]) < 71)
        array[0] = (toupper(input[32]) - 55) * 16;
    else
        array[0] = (input[32] - 48) *16;
    if(toupper(input[33]) > 64  && toupper(input[33]) < 71)
        array[0] += (toupper(input[32]) - 55);
    else
        array[0] += input[33] - 48;
    if(toupper(input[34]) > 64  && toupper(input[34]) < 71)
        array[1] = (toupper(input[34]) - 55) *16;
    else
        array[1] = (input[34] - 48) *16;
    if(toupper(input[35]) > 64  && toupper(input[35]) < 71)
        array[1] += (toupper(input[35]) - 55);
    else
        array[1] += input[35] - 48;
    accum = 0;
    for(x = 0; x < 32; x+= 2)
    {
        if (GetBit( array, z ) == NEGATIVE)
        {
            if(toupper(input[x]) > 64  && toupper(input[x]) < 71)
                accum = (toupper(input[x]) - 55) * 16;
            else
                accum = (input[x] - 48) * 16;
            if(toupper(input[x+1]) > 64  && toupper(input[x+1]) < 71)
                accum += (toupper(input[x+1]) - 55);
            else
                accum += input[x+1] - 48;
            accum = accum / -1;
            output[y] = accum;
            ++y; 
            accum = 0;
        }
        else
        {
            if(toupper(input[x]) > 64  && toupper(input[x]) < 71)
                accum = (toupper(input[x]) - 55) * 16;
            else
                accum = (input[x] - 48) *16;

            if(toupper(input[x+1]) > 64  && toupper(input[x+1]) < 71)
                accum += (toupper(input[x+1]) - 55);
            else
                accum += input[x+1] - 48;
            output[y] = accum;
            ++y;     
            accum = 0;
        }
        ++z;
    }
    return ( char * ) output;
}


int main( void )
{
    char test[48] = {0};
    char input[48] ={0}, output[48] = {0}, output1[48] = {0}, output2[48] = {0};
    unsigned long x;
    strcpy(input, "09142006"); // Test input
    strcpy(output , RijndaelEncrypt( input ));
    strcpy(output1, CreateHexString( output )); // Create Customer key
    if(output1 == NULL)
        return BUFFEROVERFLOW;  
    printf("Trial key is %s\n", output1); // Print readable key
    strcpy(output2, ConvertHexString( output1 ));   // Convert customer key back
                                                    // to Rijndael encryption string
    strcpy(test , RijndaelDecrypt(output2));    // Decrypt the Rijndael string
    printf("Unencrypted Date out: %s\n", test);
    return 0;
}

**Dave_Sinkula** · 01-07-2006

This certainly seems primed for pointers (and perhaps character literals):

Code:

    if(toupper(input[32]) > 64  && toupper(input[32]) < 71)
        array[0] = (toupper(input[32]) - 55) * 16;
    else
        array[0] = (input[32] - 48) *16;
    if(toupper(input[33]) > 64  && toupper(input[33]) < 71)
        array[0] += (toupper(input[32]) - 55);
    else
        array[0] += input[33] - 48;
    if(toupper(input[34]) > 64  && toupper(input[34]) < 71)
        array[1] = (toupper(input[34]) - 55) *16;
    else
        array[1] = (input[34] - 48) *16;
    if(toupper(input[35]) > 64  && toupper(input[35]) < 71)
        array[1] += (toupper(input[35]) - 55);
    else
        array[1] += input[35] - 48;

I don't suppose you could share the header with us?

**anonytmouse** · 01-07-2006

You are overcomplicating this. The encryption function outputs a 16 byte number that you wish to convert to human readable number. There is no need to worry about positive or negative, just treat it as an unsigned byte array.

Code:

/*
 * Takes as input a 16 byte number and provides as output a 32 character
 * (plus nul terminator) hexadecimal string.
 */
void CreateHexString( const unsigned char* input, char out[33] )
{
    size_t x;

    for (x = 0; x < 16; x++)
    {
        sprintf(&out[x * 2], "%02x", input[x]);
    }
}


/*
 * Takes as input a 32 character hexadecimal string and provides as output
 * a 16 byte number.
 */
void ConvertHexString( const char *input, unsigned char out[16] )
{
    size_t       x;
    unsigned int temp;

    if (strlen(input) < 32)
    {
        memset(out, 0, 16);
    }
    else
    {
        for (x = 0; x < 16; x++)
        {
            sscanf(&input[x * 2], "%2x", &temp);
            out[x] = (unsigned char) temp;
        }
    }
}

4. Can the 36 byte customer key be compressed to something more reasonable such as 10 or 12 bytes?

Well, the output number is 16 bytes so it is unlikely you can compress it to less than that. Obviously, you can use a number encoding scheme that is more efficient than hexadecimal but hexadecimal seems to be the standard for this sort of thing.

**BobS0327** · 01-08-2006

There is no need to worry about positive or negative, just treat it as an unsigned byte array.

Oops, forgot to mention that once the user has the human readable string, he/she must enter that string key into an edit control to enable the application. The application must convert the human readable string back into a format for decryption to determine the expiration date, which is the reason for preserving the sign. Sorry about my omission.

What I'm really concerned about are the two functions CreateHexString and ConvertHexString. They're just downright ugly. Especially the ConvertHexString function. I'd like to make them a little more efficient and understandable. Thus, if I have to revisit this code 6 months down the road, I'll be able to understand what I'm doing with these functions.
Header file:

Code:

enum {DEFAULT_BLOCK_SIZE=16};
enum {MAX_BLOCK_SIZE=32, MAX_ROUNDS=14, MAX_KC=8, MAX_BC=8};
enum {ECB=0, CBC=1, CFB=2};


typedef struct
{
    int Ke[MAX_ROUNDS+1][MAX_BC];
    int Kd[MAX_ROUNDS+1][MAX_BC];
    int keylength;
    int blockSize;
    int iROUNDS;
    char chain0[MAX_BLOCK_SIZE];
    char chain[MAX_BLOCK_SIZE];
    int tk[MAX_KC];
    int a[MAX_BC];
    int t[MAX_BC];
} aes_type;

/* Function prototypes */

void CreateRijndaelKey(aes_type *aes, char const *key, char const *chain, int keylength,
    int blockSize);
void RijndaelEncryptBlock(aes_type *aes, char const *in, char *result);
void RijndaelDecryptBlock(aes_type *aes, char const *in, char *result);
void Encrypt(aes_type *aes, char const *in, char *result, int n, int iMode);
void Decrypt(aes_type *aes, char const *in, char *result, int n, int iMode);

Thread: Constructive criticism, suggestions etc

Thread Tools

Search Thread

Display

Constructive criticism, suggestions etc

Similar Threads

Free Book Suggestions!

Looking for constructive criticism

Constructive criticism highly appreciated!

Suggestions on Organizing Files

Need Suggestions