Click to Rate and Give Feedback
Related Articles

In this column, the author lays out some guiding principles that you should follow when working with the ASP.NET MVC framework.

Matt Ellis

MSDN Magazine July 2009

...

Read more!

We use the new Asynchronous Agents Library in Visual C++ 2010 to solve the classic Dining Philosophers concurrency problem.

Rick Molloy

MSDN Magazine June 2009

...

Read more!

Memory usage can have a direct impact on how fast an application executes and thus is important to optimize. In this article we discuss the basics of memory optimization for .NET programs.

Subramanian Ramaswamy and Vance Morrison

MSDN Magazine June 2009

...

Read more!

Jeremy Miller continues his discussion of persistence patterns by reviewing the Unit of Work design pattern and examining the issues around persistence ignorance.

Jeremy Miller

MSDN Magazine June 2009

...

Read more!

In this article, we show you how to integrate a Windows Services-based solution with SharePoint. The results enable you to provision, start, stop, and remove service instances through SharePoint 3.0 Central Administration.

Pav Cherny

MSDN Magazine April 2009

...

Read more!

Also by this Author

Learn the numerous ways in which you can rewrite URLs to defend against common Web vulnerabilities.

Bryan Sullivan

MSDN Magazine March 2009

...

Read more!

Listen in on a chat between a developer and security pro that delves into some of the major Security Development Lifecycle (SDL) requirements we impose on product teams here at Microsoft

Michael Howard

MSDN Magazine May 2009

...

Read more!

There are many ways to get into trouble when it comes to security. You can trust all code that runs on your network, give any user access to important files, and never bother to check that code on your machine has not changed. You can run without virus protection software, not build security into your own code, and give too many privileges to too many accounts. You can even use a number of built-in functions carelessly enough to allow break-ins, and you can leave server ports open and unmonitored. Obviously, the list continues to grow. What are ...

Read more!

In this installment we introduce you to new Web-oriented security guidance and tools straight from the Security Development Lifecycle (SDL) team at Microsoft.

Bryan Sullivan

MSDN Magazine September 2008

...

Read more!

Five years ago, Bill Gates issued a directive to enhance security across the board. Since then, many valuable lessons have been learned about building more secure software.

Michael Howard

MSDN Magazine November 2007

...

Read more!

Popular Articles

The MVP pattern helps you separate your logic and keep your UI layer free of clutter. This month learn how.

Jean-Paul Boodhoo

MSDN Magazine August 2006

...

Read more!

Chris Tavares explains how the ASP.NET MVC Framework's Model View Controller pattern helps you build flexible, easily tested Web applications.

Chris Tavares

MSDN Magazine March 2008

...

Read more!

C# allows developers to embed XML comments into their source files-a useful facility, especially when more than one programmer is working on the same code. The C# parser can expand these XML tags to provide additional information and export them to an external document for further processing. This article shows how to use XML comments and explains the relevant tags. The author demonstrates how to set up your project to export your XML comments into convenient documentation for the benefit of other developers. He also shows how to use comments ...

Read more!

Here we present techniques for programmatic and declarative data binding and display with Windows Presentation Foundation.

Josh Smith

MSDN Magazine July 2008

...

Read more!

When incorporating the ASP.NET DataGrid control into your Web apps, common operations such as paging, sorting, editing, and deleting data require more effort than you might like to expend. But all that is about to change. The GridView control--the successor to the DataGrid-- extends the DataGrid's functionality it in a number of ways. First, it fully supports data source components and can automatically handle data operations, such as paging, sorting, and editing, as long as its bound data source object supports these capabilities. In addition, ...

Read more!

Security Quiz
Test Your Security IQ
Michael Howard and Bryan Sullivan

Both of us enjoy reviewing code for security bugs. We would even go as far as to say we're pretty good at it. We don't pretend we're the best, but we can generally find plenty of bugs pretty quickly. Can you?
Would you know a security bug if you saw one? Find out by taking this quiz. Each code sample has at least one security vulnerability. Try to spot the bugs and see how you rate. Following the code is a summary of any vulnerabilities, some commentary, and, where appropriate, how the Security Development Lifecycle (SDL) can help find these bugs. Thanks to Peter Torr and Eric Lippert for providing input and code samples.
Bug #1 (C or C++)
void func(char *s1, char *s2) {
  char d[32];
  strncpy(d,s1,sizeof d - 1);
  strncat(d,s2,sizeof d - 1);
  ...
}
Answer We thought we'd start off pretty easy on you with a good ol' buffer overrun. To many people, the code is fine and secure because the code uses the bounded strncpy and strncat functions. However, these functions are only secure if the buffer sizes are correct, and in this example the buffer sizes are wrong. Dead wrong.
Technically, the first call is secure, but the second call is wrong. The last argument of the strncpy and strncat functions is the amount of space left in the buffer, and you just exhausted some or all of that space with the call to strncpy. Buffer overflow. Michael blogged about this exact bug type in 2004.
In Visual C++ 2005 and later, warning C4996 tells you to replace the bad function call with a safer call, and the /analyze option would issue a C6053 warning indicating that strncat might not zero terminate the string.
To be perfectly honest, strncpy and strncat (and their "n" cousins) are worse than strcpy and strcat (and their ilk) for a couple of reasons. First, the return value is just silly—it's a pointer to a buffer, a buffer that might be valid or might not. You have no way of knowing! Second, it's really tough to get the destination buffer size right. If you spotted the bug, give yourself one point.

Bug #2 (C or C++)
int main(int argc, char* argv[]) {
  char d[32];
  if (argc==2) 
    strcpy(d,argv[1]);

  ...
  return 0;
}
Answer This is an evil bug. We have seen this bug used so many times as an example of a buffer overrun, and most of the time it's impossible to determine if there is a security bug in the code or not. It all depends on how the code is used.
If this is a standard Win32 EXE, then there is no security bug here because the worst you can do is attack yourself and run code as yourself, which isn't a security bug.
Now, if this code were located in ServiceMain of a Windows service running as SYSTEM, for example, or the main function of a Linux application setuid root, then the code becomes a bona fide security bug.
Let's say the code is a Linux application marked setuid root. When the application is started by a normal user, the application actually runs as root, which means this is a local privilege elevation vulnerability.
As in the code example given in Bug #1, C4996 warnings are issued for the call to strcpy, and /analyze would issue a C6204 indicating a potential buffer overrun. If you answered, "Huh! I need much more context," then give yourself two points; otherwise, no points for you.

Bug #3 (could be any language—example is in C#)
byte[] GetKey(UInt32 keySize) {
  byte[] key = null;

  try {
    key = new byte[keySize];
    RNGCryptoServiceProvider.Create().GetBytes(key);
  }
  catch (Exception e) {
    Random r = new Random();
    r.NextBytes(key);
  }

  return key;
}
Answer There are two bugs in this lousy key-generation code. The first is pretty obvious: if the call to the cryptographically sound random number generator fails, the code catches the exception and then calls a truly lousy and predictable random generator. If you spotted this, give yourself a point. It's an SDL requirement to use cryptographically random numbers to generate keys.
But there's another bug: the code catches all exceptions. Other than in rare instances, catching all exceptions—whether they are C++ exceptions, Microsoft .NET Framework exceptions or structured exception handling on Windows—hides real errors. So don't do it.
A structured exception handler in C or C++ that catches all exceptions (including access-protection faults such as buffer overruns) will yield a C6320 warning when compiled with /analyze. It was this kind of design that let attackers reattempt their attacks against the animated cursor bug MS07-017. If you spotted the exception-handling bug, give yourself one more point.

Bug #4
void func(const char *s) {
  if (!s) return;
  char t[3];
  memcpy(t,s,3);
  t[3] = 0;
  ...
}
Answer We found a bug like this one a few years ago in Windows Vista while it was still in the process of being developed. But is this a security bug? Obviously it's a coding bug because the code writes to the fourth array element, and the array is only three elements long. Remember, arrays start at zero, not one. I would contend that this is not a security bug because the attacker has no control whatsoever.
If the bug looked like this where the attacker controls i, however, then that would mean the attacker could write a zero anywhere in memory. And that's a card-carrying security bug:
void func(const char *s, int i) {
  if (!s) return;
  char t[3];
  memcpy(t,s,3);
  t[i] = 0;
  ...
}
This code yields a C6201 "out of valid index range" warning when compiled with /analyze. If you said, "Not a security bug," give yourself one point.

Bug #5
public class Barrel {
  // By default, a barrel contains one rhesus monkey.  
  private static Monkey[] defaultMonkeys = 
    new[] { new RhesusMonkey() };
  // backing store for property.
  private IEnumerable<Monkey> monkeys = null; 

  public IEnumerable<Monkey> Monkeys {
    get {
      if (monkeys == null) {
        if (MonkeysReady())
          monkeys = PopulateMonkeys();
        else
          monkeys = defaultMonkeys;
      }
      return monkeys;
    }
  }
}
Answer This is a hard one. The author of this class thinks that they are being both safe and efficient. The backing store is private, the property is read-only, and the property type is IEnumerable<T>, so the caller cannot do anything but read the state of the Barrel.
The author has forgotten that a hostile caller can try to cast the return value of the property to Monkey[]. If there are two Barrels and each one has the default Monkey list, then a hostile caller that has one of them can replace the RhesusMonkey in the static default list with any other Monkey, or null, thereby effectively changing the state of the other Barrel.
The solution here is to cache a ReadOnlyCollection<T> or some other truly read-only storage that protects the underlying array from mutation by a hostile or buggy caller. If you got this, give yourself two points.

Bug #6 (C#)
protected void Page_Load(object sender, EventArgs e) {
  string lastLogin = Request["LastLogin"];
  if (String.IsNullOrEmpty(lastLogin)) {
    HttpCookie lastLoginCookie = new HttpCookie("LastLogin",
      DateTime.Now.ToShortDateString());
    lastLoginCookie.Expires = DateTime.Now.AddYears(1);
    Response.Cookies.Add(lastLoginCookie);
  }
  else {
    Response.Write("Welcome back! You last logged in on " + lastLogin);
    Response.Cookies["LastLogin"].Value = 
      DateTime.Now.ToShortDateString();
  }
}
Answer This is a straightforward, cross-site scripting vulnerability, the most common vulnerability on the Web. Although the code seems to imply that the lastLogin value always comes from a cookie, in fact, the HttpRequest.Item property will prefer a value from the query string over a value from a cookie.
To put this another way, no matter what the value of the lastLogin cookie happens to be set to, if an attacker adds the name/value pair lastLogin=<script>alert('0wned!')</script> to the query string, the application will choose the malicious script input for the value of the lastLogin variable. If you answered XSS, give yourself one point.

Bug #7 (C#)
private decimal? lookupPrice(XmlDocument doc) {
  XmlNode node = doc.SelectSingleNode(
    @"//products/product[id/text()='" + 
    Request["itemId"] + "']/price");
  if (node == null)
    return null;
  else
    return (Convert.ToDecimal(node.InnerText));
}
Answer If you said XPath injection, give yourself one point. XPath injection works on exactly the same principle as its much more (in)famous cousin SQL injection. By creating a query that combines XPath code with unvalidated and unescaped user input, this code is vulnerable to injection attacks. Any application that manipulates text that is then used to perform some form of operation is subject to injection vulnerabilities.

Bug #8 (C#)
public class CustomSessionIDManager : System.Web.Session    State.SessionIDManager
{
    private static object lockObject = new object();

    public override string CreateSessionID(HttpContext context)
    {
        lock (lockObject)
        {
            Int32? lastSessionId = (int?)context.Application                ["LastSessionId"];
            if (lastSessionId == null)
                lastSessionId = 1;
            else
                lastSessionId++;
            context.Application["LastSessionId"] = lastSessionId;
            return lastSessionId.ToString();
        }
    }
}
Answer There are two main problems here. While the code correctly applies a lock around the app logic to ensure that two threads don't create the same session ID at the same time, it's still not safe to deploy in a server farm. Application state as referenced by the HttpContext.Application object is not shared across servers. If this application was deployed in a server farm, it could lead to session ID collisions. If you caught this bug, give yourself one point.
Another serious problem is that the session IDs generated by this class are easily guessed sequential integers. If a user looks at his session token and notices that he has session ID 100, he could use a simple browser utility to change the session ID to 99 or 98 or any other lower value to hijack those users' sessions.
A much better option for the developer in this case would be to use a GUID or other large, random, string-combining letters and numbers. If you realized that sequential integers are poor choices for session ID tokens, you score a point.

Bug #9 (C#)
bool login(string username, 
           string password, 
           SqlConnection connection, 
           out string errorMessage) {
  SqlCommand selectUserAndPassword = new SqlCommand(
    "SELECT Password FROM UserAccount WHERE Username = @username",
    connection);
  selectUserAndPassword.Parameters.Add(
    new SqlParameter("@username",  username));
  string validPassword = 
    (string)selectUserAndPassword.ExecuteScalar();

  if (validPassword == null) {
    // the user doesn't exist in the database
    errorMessage = "Invalid user name";
    return false;
  }
  else if (validPassword != password) {
    // the given password doesn't match 
    errorMessage = "Incorrect password";
    return false;
  }
  else {
    // success
    errorMessage = String.Empty;
    return true;
  }
}
Answer The biggest problem here is that the application is returning too much information to the user in the case of a failed login. While it's definitely helpful for a user to know whether he just mistyped his password or whether he completely forgot his user name, this information is also useful to an attacker attempting a brute force attack against the application. Although it sounds counterintuitive, in this situation it's better to be unhelpful. Failed logins should display messages such as "Invalid username or password," not "Invalid username" and "Invalid password."
If you caught this, you get a point. And give yourself a bonus point if you also remembered that the application shouldn't be storing passwords in plaintext in the database; instead, it should be storing and comparing salted hashes of the passwords.
So How Do You Rate?
Points Comment
15+ We're hiring.
11-14
Not bad, not bad at all. Now apply your talents to all that code your buddies wrote.
7-10
Hmmm. OK. You clearly have a lot to learn, but you're probably better than 50% of the developers out there writing Web apps today.
4-6
You have a LOT to learn. Go to your favorite book store and buy all the books written by both authors of this article.

0-3 Step away from the editor and compiler, and no one will get hurt.

Bug #10 (Silverlight CLR C#)
bool verifyCode(string discountCode) {
  // We store the hash of the secret code instead of 
  // the plaintext of the secret code.
  // Hash the incoming value and compare it against 
  // the stored hash.
  SHA1Managed hashFunction = new SHA1Managed();
  byte[] codeHash = 
    hashFunction.ComputeHash(
      System.Text.Encoding.Unicode.GetBytes(discountCode));
  byte[] secretCode = new byte[] { 
    116, 46, 130, 122, 36, 234, 158, 125, 163, 122,
    157, 186, 64, 142, 51, 153, 113, 79, 1, 42 };

  if (codeHash.Length != secretCode.Length) {
    // The hash lengths don't match, so the strings don't
    // match this should never happen, but we check anyway
    return false;
  }

  // perform an element-by-element comparison of the arrays
  for (int i = 0; i < codeHash.Length; i++) {
    if (codeHash[i] != secretCode[i])
      return false; // the hashes don't match
  }

  // all the elements match, so the strings match
  // the discount code is valid, inform the server
  WebServiceSoapClient client = new WebServiceSoapClient();
  client.ApplyDiscountCode();

  return true;
}
Answer The developer made a wise decision not to embed the secret code in plaintext in the code. If you only need to test whether a user knows a secret (such as a discount code or a password), it's always better to store a hash of that secret and compare hashes rather than storing the plaintext and comparing strings directly. Unfortunately, the developer chose to use the SHA-1 hash algorithm, which is showing serious signs of weakness and has been subsequently banned by the SDL. A much better choice would have been to use the SHA256Managed class that implements the SDL-approved and recommended SHA-256 hash algorithm. If you caught this, you get a point.
Even worse than choosing SHA-1 over SHA-256 is the fact that the developer neglected to salt the hash value. Unsalted hashes are much more vulnerable to cracking via precomputed hash tables (often called rainbow tables). It would probably take an attacker a short time to determine the original plaintext secret code from the unsalted hash value. (The authors will post congratulations in the SDL blog to acknowledge the first person who responds to us with the plaintext secret code.) Give yourself a point if you caught the unsalted hash.
However, the biggest problem with this code is the fact that it's being executed on the client machine at all! Remember we stated at the beginning that this is Silverlight CLR code that runs in the user's browser. Any code that runs on the client can be manipulated by an attacker. Period. Nothing prevents a determined user from attaching a debugger to the browser instance running the Silverlight code and stepping through the code as it executes.
He could set the codeHash variable to be equal to the secretCode hash so that the comparison logic would always succeed. Or he could skip over the validation logic completely and just jump the current instruction to the Web service call that applies the discount code. Easiest of all, he could avoid the debugger entirely and just call the Web service method ApplyDiscountCode directly!
It's important to realize that even though you may be using C# or Visual Basic to create Silverlight applications just like you would for ASP.NET Web Forms, Silverlight code runs on the client's machine, and Web Forms code runs on the server. Code that runs on the client can be viewed and changed by an attacker. Never embed secrets into client-side code or allow client-side code to make privileged decisions (such as whether a discount code is valid or whether a user is authorized to perform a certain action). If you caught this bug, give yourself a point.

Page view tracker