Research

Application security testing, analysis, and metrics

Failing to Check Error Conditions Could Get You Sued

The Ontario Lottery and Gaming Corp. is in a bit of hot water after refusing to pay a $42.9 million jackpot:

According to the statement, Kusznirewicz was playing an OLG slot machine called Buccaneer at Georgian Downs in Innisfil, Ont., on Dec. 8 when it showed he had won $42.9 million.

When the machine’s winning lights and sounds were activated, an OLG floor attendant initially told Kusznirewicz to go to the “winners circle” to claim his prize, according to the statement. But other OLG employees immediately arrived and told him that the corporation would not be paying, because there had been a “machine malfunction.”

They offered him a free dinner for four at the casino’s buffet.

In a press release, OLG described the malfunction as follows:

“The single Buccaneer-themed slot machine in question is a two cent per play machine with a base game reward of $300 and an absolute maximum payout of $9,025,” the release states.

“The $42 million figure is not a possible award given this machine’s configuration and pay table settings.”

Of course the lawsuit will probably be thrown out, or OLG will settle with the guy for a lesser amount. But from a technical perspective, it’s amusing to think about what happened to cause this scenario. You can imagine the slot machine software looking something like this:

void do_spin() {
  spin_reels();
  if (winning_combination) {
    unsigned int winnings = calculate_payout_in_cents();
    send_to_display("You've won $%u!n", winnings/100);
    add_to_balance(winnings/100);
  }
}

int calculate_payout_in_cents() {
  int rv;
  if (rv = lookup_payout_amount())
    return rv;
  else
    return -1;
}

For some reason, something caused lookup_payout_amount() to return NULL, which meant calculate_payout_in_cents() returned -1, signifying an error. Then, in addition to implicitly casting the signed result to an unsigned type, do_spin() fails to check for the error condition! It assumes success and announces the payout via the slot machine’s display. In this case, the -1, represented as 0xFFFFFFFF in two’s complement, gets interpreted as an unsigned number, 4294967295, due to the implicit cast, and the display prints “You’ve won $42949672!”

Today’s lesson: remember to check your error conditions!

 

FREE Security Tutorials from Veracode

Cyber Security Threats
Mobile Phone Security
Flash Player Security
SQL Injection Attack
CRLF Injection
 

Veracode Security Solutions

Software Security Testing
Binary Code Analysis
Application Testing
 

Veracode Data Security Resources

Data Breaches
Data Loss Prevention
Data Security

2 Comments »

Not that one should ever intentionally write ‘if (x = y)’ given that someone is likely to come along later and ‘fix’ it to be ‘if (x == y)’; if you must write assignment in a subexpression, do the right thing (and what GCC, at least, will recognize as sign of intent) and parenthesize it; besides, don’t use random integer values as booleans. So it really should be “if ((rv = lookup_payout_amount()) != 0)”. Of course, your example allows lookup_payment_amount to return -1 and have it not go through the error codepath, or -2 or whatever, so even if you eyeballed that and fixed the caller to check for “-1″, you’d get screwed if -2 came up from somewhere else. Also, if lookup_payment_amount is returning ‘NULL’, perhaps you should reconsider treating its return value as an integer. And using pointers as booleans is even worse than using non-pointers as booleans. So really it should be:

int *winamtp;

winamtp = lookup_payment_amount();
if (winamtp != NULL) {
assert(*winamtp >= 0);
return *winamtp;
}
return (-1);

Comment by ju ma — March 30, 2009 @ 1:47 pm

@ju ma:

Yes, the idea was that lookup_payment_amount() is “expected” to return an amount, and that 0 indicates it was unable to do that, in which case calculate_payout_in_cents() returns -1. I should have been clearer on that.

The unchecked error condition I was trying to highlight is in do_spin(), where the value of winnings is not checked.

Agree wholeheartedly that ‘if (x=y())’ is a terrible construct, but unfortunately people use that shortcut all the time.

Comment by Chris Eng — March 30, 2009 @ 2:47 pm

RSS feed for comments on this post. TrackBack URI

Leave a comment


Mobile Security

Sql Injection

cyber security

Categories

Archive

Powered by WordPress