Always Use Braces
| | class BoardComputer { |
| | |
| | CruiseControl cruiseControl; |
| | |
| | void authorize(User user) { |
| | Objects.requireNonNull(user); |
| » | if (user.isUnknown()) |
| | cruiseControl.logUnauthorizedAccessAttempt(); |
| » | if (user.isAstronaut()) |
| | cruiseControl.grantAccess(user); |
| » | if (user.isCommander()) |
| | cruiseControl.grantAccess(user); |
| | cruiseControl.grantAdminAccess(user); |
| | } |
| | } |
Here we’ve translated the switch statement from the previous comparison into separate if statements. There’s one problem, though. The indentation in the snippet is perilously misleading. There are no curly braces after the if, so the condition applies only to the subsequent line. That makes the whole method behave maliciously—the line cruiseControl.grantAdminAccess(user); is always executed and grants admin access to any user! What a mess.
The root of this problem isn’t the indentation. It’s the missing curly braces that define the scoping. Essentially, this is a variant of the switch fallthrough that we’ve seen in Avoid Switch Fallthrough.
If you’re aware of this problem for a switch, you might still overlook it with an if. Even if you indent correctly, there’s no guarantee that anyone would spot the bug during the always hectic crunch times before deadlines. This is why we advise everybody to always use braces.
Take a look at the corrected version.
| | class BoardComputer { |
| | |
| | CruiseControl cruiseControl; |
| | |
| | void authorize(User user) { |
| | Objects.requireNonNull(user); |
| | if (user.isUnknown()) { |
| | cruiseControl.logUnauthorizedAccessAttempt(); |
| | } |
| | if (user.isAstronaut()) { |
| | cruiseControl.grantAccess(user); |
| | } |
| | if (user.isCommander()) { |
| | cruiseControl.grantAccess(user); |
| | } |
| » | cruiseControl.grantAdminAccess(user); // SECURITY THREAT |
| | } |
| | } |
Here we’ve added the curly braces in the way that the compiler treats the code. You can spot the error easily: no condition guards the call of grantAdminAccess(). Thanks to the curly braces, the code is much more readable now. Even if time is short, it’s much more likely that someone will spot this critical bug.
It’s also safer to add new lines of code now. Before, you might have overlooked the fact that there are no curly braces and added a line into one of the blocks.
Misleading indentation caused the bug in our example. That’s dangerous, especially because it was combined with the misplaced thinking that fewer characters mean better code. Remember: Less code is not always better—more readable code is!
You might argue that such an error is unrealistic and would hardly ever happen in practice for security-critical code. History tells otherwise; in 2014, Apple engineers had a very similar bug in the implementation of Apple’s SSL/TLS protocol for iOS.[19] Attackers exploiting that bug could eavesdrop to any secure connection of an Apple device. Scary, isn’t it?
Always using curly braces out of routine is a good defense against fallthrough bugs, as is always the using auto-indentation of your IDE. But the solution to this problem isn’t perfect yet, because it violates Fail Fast and Ensure Code Symmetry. Be sure to read on!