RFID financial transaction

Question

I have this code below which establishes connection to server (from device). Basically assembles some buffer, sends it to server, gets response, parses and prints it. I would appreciate if someone can review it critically - see potential bugs, undefined behaviour, etc.

First some declarations:

typedef int s32;
typedef unsigned char u8;

#define SDK_KEY_MASK_ENTER 1
#define SDK_KEY_MASK_ESC 1

int sdkAccessFile(char*);
void postOperationsMenu(void);
int parseAndUpdateTerminalInfoFile(u8*,u8*);
void sdkPrintStr(u8*,int,int,int,int);
int inputAmountFloat(int*);
int ReadIDAny(u8*,int*);
int readTerminalName(u8*);
void sdkDispMsgBox(u8*,u8*, int, int);
int readTerminalKey(u8*);
int sdkMD5(u8*,u8*,int);
void DemoDisplayInfo(int,u8*,u8*);
s32 sdkCommCreateLink(void);
int sdkCommSendData(u8*, int,int);
int sdkCommRecvData(u8*,int,int,u8*);
int readTerminalInfo(u8*,u8*,u8*,u8*);
void sdkPrintStart(void);
char* serverErrorDesc(int);
void sdkPrintInit(void);
void sdkDispClearScreen(void);
int sdkReadFile(u8*,u8*,int,int*);

Code:

void addPoints()
{

   u8 mifareCardNumber[20] = {0};
   s32 idLength = 0;
   //s32 keyPress = 0;
   s32 amount   = 0;

   // Let operator enter amount first
   if(false == inputAmountFloat(&amount))
    return;

   // Read ID
   if(0 == ReadIDAny(mifareCardNumber, &idLength))
    return;


   //
   // Sending data to server part ..
   //


   u8 sendBuffer[255] = {0};
   s32 siRet = 0;

   // Read terminal name which should be 10 bytes
   u8 tName[20] = {0};
   if(false == readTerminalName(tName))
   {
        sdkDispMsgBox("Status", "Error reading \rterminal name", 0, SDK_KEY_MASK_ESC | SDK_KEY_MASK_ENTER);
        return;
   }

   // Read authentication key
   u8 key[32] = {0};
   if(false == readTerminalKey(key))
   {
        sdkDispMsgBox("Status", "Error reading key", 0, SDK_KEY_MASK_ESC | SDK_KEY_MASK_ENTER);
        return;
   }

   //
   // Assemble buffer to send to server
   //
   sendBuffer[0] = 1;                                   // Operation code; byte 1
   memcpy(&sendBuffer[1],  tName, 10);                  // Append terminal name; next 10 bytes
   memcpy(&sendBuffer[11], &amount, 4);                 // Append amount to be added; next 4 bytes, integer
   memcpy(&sendBuffer[15], mifareCardNumber, idLength); // Append card UID; next 4 or 7 bytes

   u8 whatToHash[255] = {0};
   u8 hashOutput[16]  = {0};

   //
   // Compute signature over the packet
   //
   memcpy(whatToHash, key, 16);
   memcpy(&whatToHash[16], sendBuffer, 1 + 10 + 4 + idLength);
   if(SDK_OK != sdkMD5(hashOutput, whatToHash, 16 + 1 + 10 + 4 + idLength))
   {
      sdkDispMsgBox("Status", "Error calculating \rhash", 0, SDK_KEY_MASK_ENTER | SDK_KEY_MASK_ESC);
      return;
   }

   // Append hash to buffer too
   memcpy(&sendBuffer[15 + idLength], hashOutput, 16);

   //
   // Actual sending of data ...
   //

   // Create Link
   DemoDisplayInfo(SDK_DISP_LINE3, "Connecting", NULL);
   siRet = sdkCommCreateLink();
   if (SDK_OK != siRet)
   {
        sdkDispMsgBox("Status", "Error create link", 0, SDK_KEY_MASK_ENTER | SDK_KEY_MASK_ESC);
        return;
   }

   // Send the data
   siRet = sdkCommSendData(sendBuffer, 1 + 10 + 4 + idLength + 16, 0);
   if (SDK_OK != siRet)
   {
        sdkDispMsgBox("Status", "Error send data", 0, SDK_KEY_MASK_ENTER | SDK_KEY_MASK_ESC);
        return;
   }

   // Receive date from server
   u8 revtmp[512] = {0};
   s32 len = sdkCommRecvData(revtmp, 512 /* max amount to receive */, 10000 , NULL);
   if (len <= 0)
   {
        sdkDispMsgBox("Status", "Error receive data", 0, SDK_KEY_MASK_ENTER | SDK_KEY_MASK_ESC);
        return;
   }

   //
   // Parse server response, which comes in form "param1;param2;param3;..."
   //

   // Note: check if these lengths will be sufficient also for UTF8 string, also compare with revtmp size if it will fit
   u8 * pch           = NULL;
   u8 name[255]       = {0};
   u8 userAmount[255] = {0};
   u8 date[50]        = {0};
   u8 tranID[50]      = {0};
   u8 cashback[50]    = {0};
   int counter        = 0;

   //
   // Extract operation status from the server response first. First byte
   // indicates if it is success, error, or information update.
   //
   pch = strtok(revtmp,";"); // extract it
   int respCode = atoi(pch); // convert to integer
   if(respCode != 1)
   {

       if(respCode == 7) // 7 is information update
       {
            parseAndUpdateTerminalInfoFile(pch, tName);
            return;
       }
       else
       {
            // Otherwise error
            sdkDispMsgBox("Status", serverErrorDesc(atoi(pch)), 0, SDK_KEY_MASK_ENTER | SDK_KEY_MASK_ESC);
            return;
       }
    }

   // Now, extract parameters from server response
   // Note: If the response format is different we may get unexpected result.
   // Note: Pay attention that these variables have fixed length here, to avoid oveflow

   while (pch != NULL)
   {
    if(counter == 1)
    {
        // Get card owner name
        strcpy(name, pch);
    }else if(counter == 2)
    {

        strcpy(userAmount, pch);
    }else if(counter == 3)
    {
        // Get transaction ID
        strcpy(tranID, pch);
    }
    else if (counter == 4)
    {
        // Get date
        strcpy(date, pch);
    }else if (counter == 5)
    {
        // Get cashback value
        strcpy(cashback, pch);
    }
        pch = strtok (NULL, ";");
        counter++;
    }

    sdkDispClearScreen();

    // We parsed data, first show card owner and balance on the screen
    u8 finalResult[255] = {0};
    sprintf(finalResult, "Owner: %s \rBalance: %.2f", name,  (double)strtol(userAmount,NULL,10)/(double)100.0);
    sdkDispMsgBox("Transaction OK", finalResult, 0, SDK_KEY_MASK_ESC | SDK_KEY_MASK_ENTER);

    // Read terminal information
    // Note: notice fixed size maximum lengths
    u8 terminalName[10 + 1]       = {0};
    u8 terminalAddress[100 + 1]   = {0};
    u8 companyName[50 + 1]        = {0};
    u8 hotlineNumber[10 + 1 + 10] = {0};
    if(false == readTerminalInfo(terminalName, companyName, terminalAddress, hotlineNumber))
    {
        sdkDispMsgBox("Status", "Error reading file", 0, SDK_KEY_MASK_ESC | SDK_KEY_MASK_ENTER);
        return;
    }

    //
    // Printing ..
    //

    sdkPrintInit();
    u8 printBuffer[200]={0};

    memset(printBuffer, 0, 200);
    sprintf(printBuffer,"%s", companyName);
    sdkPrintStr(printBuffer, 1, 1, 0, 3);

    memset(printBuffer, 0, 200);
    sprintf(printBuffer," ");
    sdkPrintStr(printBuffer,1, 1, 0, 3);

    memset(printBuffer, 0, 200);
    sprintf(printBuffer,"Terminal: %s", terminalName);
    sdkPrintStr(printBuffer, 1, 1, 0, 3);

    memset(printBuffer, 0, 200);
    sprintf(printBuffer,"Address: %s", terminalAddress);
    sdkPrintStr(printBuffer, 1, 1, 0, 3);

    memset(printBuffer, 0, 200);
    sprintf(printBuffer, "Date: %s", date);
    sdkPrintStr(printBuffer, 1, 1, 0, 3);

    memset(printBuffer, 0, 200);
    sprintf(printBuffer, "Transaction ID: %s", tranID);
    sdkPrintStr(printBuffer, 1, 1, 0, 3);

    memset(printBuffer, 0, 200);
    sprintf(printBuffer, "Amount transferred: %.2f", (double)amount/(double)100.0);
    sdkPrintStr(printBuffer, 1, 1, 0, 3);

    memset(printBuffer, 0, 200);
    sprintf(printBuffer, "Cashback: %.2f",(double)strtol(cashback,NULL,10)/(double)100.0);
    sdkPrintStr(printBuffer, 1, 1, 0, 3);

    memset(printBuffer, 0, 200);
    sprintf(printBuffer, "Current balance: %.2f", (double)strtol(userAmount,NULL,10)/(double)100.0);
    sdkPrintStr(printBuffer, 1, 1, 0, 3);

    memset(printBuffer, 0, 200);
    sprintf(printBuffer, "Card owner: %s", name);
    sdkPrintStr(printBuffer, 1, 1, 0, 3);

    memset(printBuffer, 0, 200);
    sprintf(printBuffer," ");
    sdkPrintStr(printBuffer, 1,1, 0, 3);

    memset(printBuffer, 0, 200);
    sprintf(printBuffer,"Hotline: %s", hotlineNumber);
    sdkPrintStr(printBuffer, 1, 1, 0, 3);

    sdkPrintStart();

    return;

}

Are you guaranteed that your device has a fixed endianness? And is your server written to decode the device's endianness? — JS1, Jun 9, 2015 at 7:09
@chux: you had some opinions on the reading card ID but you removed it without me fully reading it... maybe you can post it at least as comment. — user75356, Jun 10, 2015 at 15:41

Edward · Accepted Answer · 2015-06-11 18:37:32Z

I see a number of things that may help you improve your code.

Break up the code into smaller functions

The addPoints() function is quite long and does a series of identifiable steps. Rather than having everything in one long function, it would be easier to read and maintain if each discrete step were its own function.

Eliminate "magic numbers"

This code is littered with "magic numbers," that is, unnamed constants such as 20, 200, 50, etc. Generally it's better to avoid that and give such constants meaningful names. That way, if anything ever needs to be changed, you won't have to go hunting through the code for all instances of "20" and then trying to determine if this particular 20 means the length of the card number or some other constant that happens to have the same value.

Be careful with signed versus unsigned

Your code declares pch to be u8 * but then it's assigned a value from strtok which returns a char *. It may be that your code is compiled to always generate unsigned char, even for constant strings, but you should make sure you know which you have.

Check for `NULL`

The first call to strtok could return NULL if the first character in the buffer is '\0', but the code does not seem to check for that possibility. Since we don't have the code for the various other functions, we can't tell if they are OK getting a NULL pointer, but you may wish to verify that.

Use `strncpy` rather than `strcpy`

The code currently gets up to 512 bytes from a server and parses using strtok. It then copies those parsed strings into fixed size buffers on the stack, but does not specify a maximum length. This is a recipe for creating a stack buffer overflow vulnerability. Instead, you should use the safer strncpy function which copies up to a fixed length.

Use `snprintf` rather than `sprintf`

The rationale is the same as for strncpy rather than strcpy. Additionally, you may want to limit the sizes of the strings and numbers you print by using the format specifier.

Initialize variable only when needed

The code currently calls readTerminalInfo with a number of strings that are all declared like this:

u8 terminalName[10 + 1]       = {0};

There seems to be no reason to initialize the first byte (only) to 0 if the call will fill in those values anyway. The same is true for a number of variables.

Consider consolidating the strings

If you ever need to provide a different language for the interface, it's convenient to have all of strings extracted into a list and named. For example:

// these strings should be translated
static const char STATUS_MSG[] = "Status";
static const char STATUS_READNAME_ERR = "Error reading \rterminal name"
static const char STATUS_READKEY_ERR = "Error reading key"
...
// end of translated strings

sdkDispMagBox(STATUS_MSG, STATUS_READKEY_ERR, ...);

Now all of the human translatable strings are all together and strings that should not be translated (such as ";" in this code), are separate. That way, if you ever have to produce, say, a French language version, it is more easily done. In fact, one could put each language in its own translation unit and then customization could be as simple as choosing which file to include. Note, too, that this costs literally nothing in terms of ROM or RAM or runtime. The resulting code is the same. The only difference is the organization of it.

Understand numeric conversion

Several places in the code have something like this:

sprintf(printBuffer, "Amount transferred: %.2f", (double)amount/(double)100.0);

However, neither cast is needed and the code could be written:

sprintf(printBuffer, "Amount transferred: %.2f", amount/100.0);

The reason is that the default for floating point literals is to make them double and once one of the operands is a double the other will be promoted. See this explanation of implicit conversions for more details.

Prefer a `switch` to long `if .. else`

The code currently includes a relatively long if .. else construct to parse the buffer received from the server. This, however, would be improved for readability by the use of a switch instead, and for reliability by also providing a default case that could handle the error condition (more than 5 fields).

Use the return value of `sprintf`

Right now there are multiple instances of code like this:

memset(printBuffer, 0, 200);
sprintf(printBuffer, "Card owner: %s", name);
sdkPrintStr(printBuffer, 1, 1, 0, 3);

First, is it really necessary to clear the buffer every time? If not, then it could simply be omitted. If so, however, it could be made slightly more efficient by using the return value of sprintf which, if the call was successful, returns the number of characters printed.

Check return codes

Some of your functions, such as parseAndUpdateTerminalInfoFile return a value. We don't know what that value is, but if it's a status code indicating success or failure, it should probably be checked and handled. Right now the code simply throws the value away.

Fix your code formatting

The indentation and placement of braces is inconsistent. Fixing it would improve readability and make the code easier to maintain.

Thanks, I will go through your recommendations once again- but at this point I was interested if there were some critical errors. I think most relevant in my case was like passing u8* to strcpy, but I have verified and manufacturers of this device use that a lot even in their samples. Other issues I think I have taken care off(might not occur). But I will get back to this like I said — user75356, Jun 9, 2015 at 18:55
(1) about strcpy and max lengths - I am ensuring from the server side the data that always comes doesn't overrun the buffer (2) "The first call to strtok could return NULL if the first character in the buffer is '\0'"- This should also not happen, server should not sent it; but thanks for pointing out — user75356, Jun 10, 2015 at 6:49
It may be worth considering the small but non-zero probability of a communication error, and the potential for malicious attack. — Edward, Jun 10, 2015 at 10:50
Hi Edward - thanks for your last suggestion. But actually this device I think handles communication error - because the server has to send two bytes which are length of this packet - but this 2 bytes is not seen on the code side, I think device just strips it off.. — user75356, Jun 10, 2015 at 11:19
Aside: it can be nice if you can slightly elaborate on multilanguage support. Ideally I don't want to consume more memory than now with new approach nb: i am not sure where those hard coded strings are stored now though — user75356, Jun 10, 2015 at 17:54

Community · Accepted Answer · 2020-06-10 13:24:26Z

Remove hardcoded lengths and magic numbers

Currently, you have a block of code with lots of hardcoded lengths and magic numbers that looks like this:

   //
   // Assemble buffer to send to server
   //
   sendBuffer[0] = 1;                                   // Operation code; byte 1
   memcpy(&sendBuffer[1],  tName, 10);                  // Append terminal name; next 10 bytes
   memcpy(&sendBuffer[11], &amount, 4);                 // Append amount to be added; next 4 bytes, integer
   memcpy(&sendBuffer[15], mifareCardNumber, idLength); // Append card UID; next 4 or 7 bytes

   u8 whatToHash[255] = {0};
   u8 hashOutput[16]  = {0};

   //
   // Compute signature over the packet
   //
   memcpy(whatToHash, key, 16);
   memcpy(&whatToHash[16], sendBuffer, 1 + 10 + 4 + idLength);
   if(SDK_OK != sdkMD5(hashOutput, whatToHash, 16 + 1 + 10 + 4 + idLength))
   {
      sdkDispMsgBox("Status", "Error calculating \rhash", 0, SDK_KEY_MASK_ENTER | SDK_KEY_MASK_ESC);
      return;
   }

   // Append hash to buffer too
   memcpy(&sendBuffer[15 + idLength], hashOutput, 16);

   // Send the data
   siRet = sdkCommSendData(sendBuffer, 1 + 10 + 4 + idLength + 16, 0);

When it comes time to add a new field to the transaction (in the future), you will have a hard time getting the 1 + 10 + 4 + idLength + 16 + ... correct in all the places that you have hardcoded it. What you should do instead is use a variable to keep track of the current length, so you don't have to worry about computing it yourself. Here is how your code could be rewritten:

static void appendBuffer(u8 *buffer, s32 *pPos, const u8 *src, s32 length)
{
   memcpy(&buffer[*pPos], src, length);
   *pPos += length;
}

void addPoints()
{
   s32 sendLen = 0;
   // ...

   //
   // Assemble buffer to send to server
   //
   u8 operationCode = OPERATION_CODE;
   appendBuffer(sendBuffer, &sendLen, &operationCode, 1);          // Operation code; byte 1
   appendBuffer(sendBuffer, &sendLen, tName, TNAME_LEN);           // Append terminal name; next 10 bytes
   appendBuffer(sendBuffer, &sendLen, (u8 *) &amount, 4);          // Append amount to be added; next 4 bytes, integer
   appendBuffer(sendBuffer, &sendLen, mifareCardNumber, idLength); // Append card UID; next 4 or 7 bytes

   u8 whatToHash[MAX_HASH_LEN] = {0};
   u8 hashOutput[HASH_OUT_LEN] = {0};
   s32 hashLen                 = 0;

   //
   // Compute signature over the packet
   //
   appendBuffer(whatToHash, &hashLen, key, KEY_LEN);
   appendBuffer(whatToHash, &hashLen, sendBuffer, sendLen);
   if(SDK_OK != sdkMD5(hashOutput, whatToHash, hashLen))
   {
      sdkDispMsgBox("Status", "Error calculating \rhash", 0, SDK_KEY_MASK_ENTER | SDK_KEY_MASK_ESC);
      return;
   }

   // Append hash to buffer too
   appendBuffer(sendBuffer, &sendLen, hashOutput, HASH_OUT_LEN);

   // Send the data
   siRet = sdkCommSendData(sendBuffer, sendLen, 0);

   // ...
}

chqrlie · Accepted Answer · 2015-06-15 20:59:19Z

1

In addition to the other reviews with great remarks, I want to point out a potential problem with strtok:

If a field is empty in the message returned, strtok will skip it because it considers any chain of ; characters as a single separator. Hence the code will shift all field values.

strtok is a very poor parsing function, it should only be used for space separated values, like scanf, which has many more issues. Both should really be avoided and a hand coded method is preferred.

edited Jun 15, 2015 at 20:59

answered Jun 15, 2015 at 20:51

chqrlie

2034 silver badges9 bronze badges

Add a comment |

Stack Exchange Network

RFID financial transaction

3 Answers 3

Break up the code into smaller functions

Eliminate "magic numbers"

Be careful with signed versus unsigned

Check for `NULL`

Use `strncpy` rather than `strcpy`

Use `snprintf` rather than `sprintf`

Initialize variable only when needed

Consider consolidating the strings

Understand numeric conversion

Prefer a `switch` to long `if .. else`

Use the return value of `sprintf`

Check return codes

Fix your code formatting

Remove hardcoded lengths and magic numbers

Your Answer

Hot Network Questions

RFID financial transaction

3 Answers 3

Break up the code into smaller functions

Eliminate "magic numbers"

Be careful with signed versus unsigned

Check for NULL

Use strncpy rather than strcpy

Use snprintf rather than sprintf

Initialize variable only when needed

Consider consolidating the strings

Understand numeric conversion

Prefer a switch to long if .. else

Use the return value of sprintf

Check return codes

Fix your code formatting

Remove hardcoded lengths and magic numbers

Your Answer

Sign up or log in

Post as a guest

Related

Hot Network Questions

Check for `NULL`

Use `strncpy` rather than `strcpy`

Use `snprintf` rather than `sprintf`

Prefer a `switch` to long `if .. else`

Use the return value of `sprintf`