I've performed many code reviews, and I've often seen developers make the same mistakes. In most cases these defects in the code simply result in a poor user experience, but in some cases they open holes for hackers. There are always the really gnarly situations that take hours to debug, but quiet often the simple mistakes are what come back to create crisis situations.
Security practitioners should understand how developers introduce security vulnerabilities into applications and work to support the developers in improving code quality and security. Encouragement and support for improvement must be a fundamental part of the charter of the security organization. The first step is to understand the kinds of mistakes that contribute to vulnerabilities. Let's review some common, fundamental dos and don'ts for secure coding.
Do validate all user input
Input validation has always been important, even in command line applications (just think of the last time you used a command line tool and mistyped a command line flag).
Never assume that input from HTML forms is valid. Just because you gave the user only hidden fields or pull downs, or you had JavaScript to validate every input, does not guarantee the input will not be tampered with. A hacker with a simple local Web proxy can change anything they want after the JavaScript executes.
Verify that all input from the user contains valid characters and represents a valid value before using that value in your application. Check it early in the processing of the request to avoid accidental use before the validation is done. Also, check it at multiple levels (see defense-in-depth, later). Be restrictive. You can always ease up on the restrictions. It's harder to tighten up the validation rules later, as you may have already stored user data that would no longer be valid.
Do escape input values
Implementing any code that creates file paths, HTML, SQL statements or other strings that anoth...
To continue reading for free, register below or login
To read more you must become a member of SearchSecurity.com
er subsystem parses requires care. User input may contain characters that allow a hacker to cause your application to pass invalid strings to those subsystems that result in unauthorized access.
Unless you disallow all special characters in your input validation, you will need to make sure you properly escape or in some other way, account for special characters in the target subsystem (see the documentation for the subsystem in question for definitive escape requirements). For example,
in inputs used to calculate file paths: /, \. or ..
in strings used to calculate or display HTML: >, <, ", &
in strings used to calculate SQL statements: ', ", \
Many runtime environments already provide functions to escape these inputs.
Also, use techniques like parameter substitutions in the database interface rather than building up query strings using concatenation. Leverage this and other layers of protection that system components provide.
Do fail Safe
When making decisions that affect security, it is important to write code to deny access by default. Only allow access after confirmation that the user has proper authorization to proceed. In the following code snippet, the initial value of badPassword could result in unauthorized access if an exception is caught. Initializing the local variable to true denies access by default.
private static boolean valid (String user, String hashedInput) {
boolean badPassword = false;
try {
if (!userRecord(user).password.equals(hashedInput)) {
badPassword = true;
}
if (userRecord(user).disabled) {
badPassword = true;
}
}
catch (InvalidUserRecord e)
{
LogException(e);
}
return !badPassword;
}
Also, avoid the use of negatives, as in the example above, with badPassword. It is too easy to get the sense of logic wrong. The code above would be clearer if the local variable reflected the same sense as that of the method being implemented. For example, the following code is clearer and easier to maintain. In addition, there's no more ambiguity in the exception path.
private static boolean valid (String user, String hashedPassword) {
boolean validPassword = false;
try {
if (userRecord(user).password.equals(hashedPassword))
&& !userRecord(user).disabled) {
validPassword = true;
}
/* else validPassword is already false */
}
catch (InvalidUserRecord e)
{
/* User record must not exist or identity manager is down. */
LogException(e);
/* If server is down we'll deny access since we can't check. */
return (false);
}
return validPassword;
}
Do treat sensitive security information with care
Application developers should be mindful of the type of information being handled. If the information is sensitive, the developer should take special care in the code than handles it. Breaches that reveal information such as passwords, PINs and personal data can be disastrous.
One goal should be to make sure this information is only stored in appropriate locations. For instance, never write to an application or system log any of the user's personal information (password, SSN, credit card numbers, etc.), as these logs may be readable by operational personnel who should not have access to that personal information. Write only enough information about the user to identify within the application which user made the request.
It is almost always a good idea, and often required, to encrypt the sensitive data stored on mass storage. Storing clear-text passwords in a database, for instance, means that a hacker who simply gains read access may have all the keys to the kingdom.
A common technique for handling passwords is to only store the hashed copy of the password and use this to compare with the hashed user input (these values should also be salted to increase the work required for a dictionary attack).
Inevitably, there are clear-text copies of the user input in memory, often on the heap long after the code runs, and maybe on the paging file of the operating system. One good habit to get into is to keep the length of the code path used to process the clear-text password as short as possible. Also, clear the contents of this local memory storage within the same block as the declaration of that storage. This will help keep the clear-text passwords off the stack and heap.
Do practice defense-in-depth
Too often code relies on the surrounding environment working correctly. For example, in the following code the writer is making it clear that they are counting on request being a valid value.
/* request should be one of STATUS, EMAIL, or LOGOUT */
char * pageTemplate;
if (request == STATUS) pageTemplate = statusPage;
else if (request == LOGOUT) pageTemplate = logoutPage;
else if (request == EMAIL) pageTemplate = emailPage;
displayPage(pageTemplate, pageContent);
Defense-in-depth is a concept that says you should protect your application in multiple ways. Do input validation with the tools the application environment provides. In addition, write your code to assume the input validation might fail.
/* request should be one of STATUS, EMAIL, or LOGOUT */
char * pageTemplate;
if (request == STATUS) pageTemplate = statusPage;
else if (request == LOGOUT) pageTemplate = logoutPage;
else if (request == EMAIL) pageTemplate = emailPage;
else {
log("Invalid request: %d", request);
exit (1);
}
displayPage(pageTemplate, pageContent);
This costs little in terms of code or performance, but makes the code more robust in the face of failure.
Don't provide hints to hackers
How many times have you read articles that tell you not to deliver error messages from the database to the user? This just gives the bad guys more information. Why make it easier for them?
By all means, provide intelligible, useful error messages to your users, but keep the details in the log file.
For instance, this Perl code tells the user that this operation does a database insert and gives them an error message that may include information about the database that you don't want a hacker to have.
$blockid = DAtable::insert(%recordSpec);
if ($DAtable::err) {
print "Insert failed for user $uid. "
. "errstr='$DAtable::errstr'";
}
This gives a nefarious user too much information. They don't need to know about database operations or details of the errors encountered.
A safer approach is to tell the user something went wrong, but only provide the details in the log, as in this example:
$blockid = DAtable::insert(%recordSpec);
# Errors are reflected in the package variable $DAtable::err
# and $DAtable::errstr. There is no other interface to these
# values.
if ($DAtable::err) {
Log "Insert failed for user $uid. "
. "errstr='$DAtable::errstr'";
print 'An error has occurred. '
. 'Your information has not been saved.';
}
Now the user knows that something has failed and not to expect the data they entered or requested to be saved. They don't know it was a database operation that failed. The details of the failure are in the log file for operational personnel to investigate.
Don't add comments telling what the code does
Have you ever gone to read someone else's code and wondered, "Why did they do that?" Good comments in code are a major help in maintaining code – if they help make the code clearer.
Many developers know they should add comments and so they take the easy way out and add comments saying what the code does. The code is right there. Readers can see what it does – these comments are not what they need. Tell them why decisions are being made the way they are. Help the reader understand the code.
For example, in figure 2 above, the comment makes it clear why the code references the package variables err and errstr. Without the comment it is less clear why the code uses these directly rather than some procedural interface to get the error information.
Do study code patterns
When reviewing code, you can often find logic errors that may affect security by watching for patterns in the source code and looking for exceptions to those patterns. For instance, in the following code:
if (result == CASE_1) return(VALUE_1);
else if (result == CASE_2) return(VALUE_2);
else if (result == CASE_3) return(VALUE_3);
else if (result == CASE_4) return(VALUE_4);
else if (result == CASE_5) return(VALUE_2);
/* otherwise we're good to go with the default */
return (VALUE_0);
The next to last return statement looks suspicious. It could be a cut and paste error. It could be a logic error and cause the return value to mislead the caller into providing access to something that should be denied – e.g., a list of employees rather than a list of distributors.
Do make (code) buddies
The concept of code buddies has been around since at least the early 70s. In some development environments they are a requirement. That's because they are a good idea. Find someone else to review your code and offer to reciprocate.
Having someone else read through your code almost always results in them asking you questions. The 'what if?' and 'why that way?' questions make you think about and justify your choices outside the more solitary activity of writing the code. This change in context provides an opportunity to step back and take a fresh look at your work.
Often another set of eyes will find simple problems that you just can't see after staring at the code for weeks on end.
If you are responsible for maintenance on a piece of code, this is also a great way to engage the originator or another developer familiar with the code and learn that code faster through the dialog of the review.
Finally, you get an opportunity to see other styles and learn from other developers' code when you review code for someone else.
Don't just fix defects, study them
When a security-related defect is found in your or another developer's code, try to understand the cause. Study the defect and try to determine how you would have done things differently. When another developer makes the change, understand the changes that they made – maybe there's another way to fix it other than what you chose.
Over time you'll be able to recognize the patterns of these defects and generalize the solutions. Part of what you should strive for is to not make the same mistake. Learn from your mistakes and the mistakes of others.
About the author
Michael Jordan, CISM, has been writing code, managing development and reviewing code security for over 36 years. He worked on classics like Multics and GCOS, as well as porting BSD and System V features to various systems. Mike has managed platform and embedded system development. He was vice president of engineering at companies like 3DO and PlaceWare (now Microsoft LiveMeeting).
