Security Quiz

Test Your Security IQ

Michael Howard and Bryan Sullivan

Contents

Bug #1 (C or C++)
Bug #2 (C or C++)
Bug #3 (could be any language—example is in C#)
Bug #4
Bug #5
Bug #6 (C#)
Bug #7 (C#)
Bug #8 (C#)
Bug #9 (C#)
Bug #10 (Silverlight CLR C#)

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.

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.

So How Do You Rate?