Constructive criticism, suggestions etc

This is a discussion on Constructive criticism, suggestions etc within the C Programming forums, part of the General Programming Boards category; I have attached code to a conversion routine. It just seems to me that the conversion can be written more ...

  1. #1
    Registered User
    Join Date
    Mar 2005
    Location
    Mountaintop, Pa
    Posts
    1,059

    Constructive criticism, suggestions etc

    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;
    }

  2. #2
    Just Lurking Dave_Sinkula's Avatar
    Join Date
    Oct 2002
    Posts
    5,006
    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?
    7. It is easier to write an incorrect program than understand a correct one.
    40. There are two ways to write error-free programs; only the third one works.*

  3. #3
    Yes, my avatar is stolen anonytmouse's Avatar
    Join Date
    Dec 2002
    Posts
    2,544
    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.
    Last edited by anonytmouse; 01-07-2006 at 11:46 PM.

  4. #4
    Registered User
    Join Date
    Mar 2005
    Location
    Mountaintop, Pa
    Posts
    1,059
    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);

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Free Book Suggestions!
    By valaris in forum A Brief History of Cprogramming.com
    Replies: 4
    Last Post: 10-08-2008, 11:25 AM
  2. Looking for constructive criticism
    By wd_kendrick in forum C Programming
    Replies: 16
    Last Post: 05-28-2008, 10:42 AM
  3. Constructive criticism highly appreciated!
    By muggizuggi in forum C++ Programming
    Replies: 17
    Last Post: 08-01-2005, 12:54 PM
  4. Suggestions on Organizing Files
    By jrahhali in forum A Brief History of Cprogramming.com
    Replies: 3
    Last Post: 10-09-2004, 02:15 AM
  5. Need Suggestions
    By Drew in forum C++ Programming
    Replies: 3
    Last Post: 09-18-2003, 06:46 AM

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21