Tag Archive for 'Bugs'

December 2009 PDF Vulnerability

All file formats follow the same evolution.

  1. They start by grouping together some static content, with some nifty features for presenting and editing that data. Think text files, bitmaps, RTF documents… The file format is reasonably easy to understand, and the reader/writer is so simple that it would take a bad programmer to create vulnerabilities.
  2. Then, they start including plug-ins that let them handle more and more types of contents. This lets you include an image inside an HTML page or an Excel spreadsheet in a Word document. This relies on many plugins for getting things right. It sometimes happens that a given plugin contains a security fault that can then be exploited, for instance Internet Explorer had an issue with images in PNG format. The user would visit a page, that page would display an image, and the computer would be contaminated.
  3. Finally, they need to become interactive, so they include a scripting language of some sort. Excel has a macro system that uses Visual Basic, HTML includes Javascript…

The PDF format followed the same process to end up where it is now. In addition to any static document data (text, vector and raster images) and extended content (flash animations, videos, reader extension signatures) a PDF also contains short JavaScript that let authors create interactive documents. This means a PDF document on your desktop can:

  • Accept user input (such as checkboxes or text fields). The input can be saved to the disk if the reader supports it and allows it (Acrobat Reader, used by the vast majority of computer users, only allows saving a file if its author purchased a reader extensions license and signed the PDF file with it).
  • Change its layout at will, for instance displaying a “spouse” page only if the “spouse” checkbox was ticked.
  • Be cryptographically signed, and display information about who signed it. This kind of signature is actually accepted as valid legal proof in many countries.
  • Compute a scannable bar code from user input, so that it can be printed, then scanned on the other side with reduced error rates.
  • Send data over the internet. It can even send itself as an attachment to an email.

Needless to say, with all these features, there are inevitably going to be some exploitable security issues in the mix. Being a popular program, like Acrobat Reader, only increases the number of black hat hackers looking for vulnerabilities. One of these is the recent CVE-2009-4324 from December 2009. There are many types of vulnerabilities, their common feature being that they end up executing arbitrary operations on the computer (as opposed to the safe operations Acrobat Reader normally allows). These operations are usually to download or install trojans, so that the attacker can gain complete control over the computer.

CVE-2009-4324 is of the use-after-free kind. In short:

  • it creates a resource (which uses some memory),
  • it frees (destroys) the resource to recycle its memory,
  • it writes something to that memory,
  • it attempts to use the resource

Normally, the program should stop at step four and say “you can’t use the resource, it’s been destroyed”. A bug can cause it to believe that the resource is still there. The programmer probably assumed that the memory still contained a valid resource and did defend against the memory containing something else… and accessing that as if it were a valid resource executes some code that the attacker wanted to execute. Bingo.

In the case of CVE-2009-4324, this happens as part of the Doc.media.newPlayer method which, for performance reasons, was not completely implemented in Javascript—a bug in some Javascript code can cause the document to misbehave, but it cannot do anything that the Javascript couldn’t do on its own. Those parts that were written in a lower-level language, with access to the computer, contained the exploited bug.

The bug causes the processor to start executing code at a different memory location. In an ideal hacker world, that location would be precisely where some nasty code is present. Buffer overflows, when used to rewrite pieces of the stack, do allow such deterministic jumps. However, CVE-2009-4324 only allows a jump to an undetermined location.

The hacker solution is to use heap spray. The basic idea is that you have a short piece of code you want to execute (the payload). You create a block from that payload by adding no-ops (machine instructions that say “skip me”) before the payload. Then, you create lots of these blocks in memory, and trigger the exploit.

The exploit causes the computer to jump to an undetermined memory location. If it falls within the no-op section of any of the blocks you’ve created, you win: the computer skips over the no-ops, reaches the payload and executes it. If not, the program will crash. Too bad…

Can references be null?

I often hear people insist that C++ references can be null.

You can easily create a null reference like this:

int *ptr = 0;
int &ref = *ptr;
// ref is a null reference

First, there is no such thing as a null reference in standard C++.

Like all programming languages, C++ is purely a construction of the human mind, and the concepts of the language have been given arbitrary but meaningful names as part of the standardization process so that the users of the language could understand each other. These could be existing words such as class, aligned, statement, pointer or new ones like rvalue or cv-qualified.

In particular, the C++ standard does define a null pointer, but it does not define a null reference because that concept is not part of the language.

This means that instead of having a standard definition (like a null pointer), null references have a commonly accepted definition that follows a similar schema: a null reference is a reference r such that &r is a null pointer. For the rest of this article, I will follow this definition.

Second, a null reference cannot exist in a well-behaved program.

Since the concept does not appear in the standard, it’s pretty obvious that no place in the standard explicitly mentions an operation resulting in a null reference being constructed. In fact, the description of how references are constructed explains that references are constructed from other values, and by definition values cannot have a null pointer for an address.

In particular, the typical construction of a null reference by dereferencing a null pointer is explicitly mentioned in the standard, and described as undefined behavior.

In short, while many modern implementations will let you create a null reference, this will necessarily involve some form of undefined behavior along the way.

I tend to evaluate programmers on a “would I hire this person?” basis. So, what does the “C++ references can be null” statement tell me about someone ?

  • They might be confusing C++ references with C# or Java references, which can be null. Certainly a NO HIRE for a senior C++ programmer.
  • They might have misread their textbook and genuinely think references can be null in the same way as pointers (except, well, the lack of syntactic sugar for checking if a reference is null or creating a null reference strikes me as odd if that were the case). Again, NO HIRE for a senior C++ programmer.
  • They know it’s not standard, but they do not care about writing standard programs. This is an immediate NO HIRE even for junior C++ programmer.

Then, there are those who know about it, and would not use it because they care about writing standard programs. The convention when discussing C++ is to assume that the program is well-behaved, so most people would parse “C++ references can be null” as “C++ references can be null in a well-behaved program”, the latter being obviously incorrect. I don’t really mind a small misunderstanding, the subject being as complex as it is, as long as it’s corrected quickly.

A person who does not is either someone without much experience discussing C++ with others, or a troll. A definite NO HIRE on a team.

Now we know why…

…the stock market crashed.

failcac

AJAX is Hard

Seen from the outside, AJAX has become an easy technology:

$('#container').load('http://domain/path/to/page');

Even if you’re doing smarter things, like updating server-side values with asynchronous POST requests, it’s still easy:

$.post(
  'http://domain/path/to/action',
  { user_id       : $('#user').val(),
    new_user_name : $('#name').val() }
);

And, of course, it’s also easy to make mistakes in AJAX.

Not taking errors into account

In an ideal world, the AJAX request is sent to the server, completed successfully, and the response is propagated back and applied.

In the real world, the AJAX request might never reach the server because the network cable was pulled, or it could carry stale data that cannot be processed, or the user session might have expired, or something else altogether.

This “request cannot be completed successfully” issue has been solved for years in the traditional HTTP world by both servers and browsers: when you try to get to a page and that page can’t be reached, you will either get an error message from your browser or be redirected to another page by the server.

In the AJAX world, a failed request times out silently without anything happening. You have to actually implement that small “Your session has expired, click here to log in again” message box yourself, just like so many other websites did. And, of course, you need to take into account into all of your workflows that the user may be logged out of their session at any point.

Don’t forget to include cable-plugging as part of your testing protocols!

Forgetting to refresh parts

When you post some modifications to the server asynchronously, you need to refresh some parts according to the new state of the server. Which parts do you refresh?

While the answer might seen easy in every single specific case (I’m updating this list/object/grid, so I’ll just refresh it), the general answer is not so simple: your server-side modification might have an impact on other parts of your system.

Consider a typical Facebook-like interface: you have a menu with an inbox, and to the right of that inbox there’s the number of unread messages. On the inbox page, you have a list of messages with a little cross on each message that deletes it through AJAX. The naive thing to do is have that cross update the list of messages, but then deleting an unread message wouldn’t update the menu.

Inevitably, a developer working on an AJAX feature will forget to take into account that some other part of the page that needs to be updated. Or a developer will add some information to every page and forget that some pages need to update that information.

Repeating yourself

Javascript does not benefit from the same clean separation of features into classes, files, packages and namespaces. Also, IDE quality is lacking when compared to other languages. This makes it hard to refactor JS code when duplicate functionality starts to appear.

Let’s consider the asynchronous post situation. In order for that code to work, you need to have fields with identifiers ‘user’ and ‘name’ and some element to initiate the post through an event. This is not encapsulated: if another page needs similar “post user name” functionality, the code will have to be rewritten. In fact, when the code is that small, it’s actually faster to rewrite it than it is to find and call an existing function (not to mention writing that function in the first place).

No refactoring means the code repeats itself. Having two user-name-change pieces of javascript on two different pages means twice as much work to do when you eventually change how that part really works.

Allowing complex behavior to be written in two or three lines is no excuse for letting your code get out of hand: stand firm by the “once, twice, refactor” motto and do not hesitate to turn a three-liner into a ten-line reusable function with appropriate documentation.

PHP Type Checking

PHP does not enforce types at compile-time (if anything, because there isn’t a compile time) and runtime checking only happens at the leaves of your source code tree, when you use a PHP function and that function notices one of its arguments is incorrect.

There are of course ways of introducing additional type safety into PHP code, both through development practices and through hints. For instance, you can hard-code checks into function prologues:

function SetUsername($username, $usr_id)
{
  assert (is_string($username));
  assert (is_int($usr_id));
  // ...
}

And, if using class types, you can also use the type hint mechanism in PHP 5 to get automatic warnings:

function FitToWindow(Image $img, Window $window)
{
  // ...
}

There remains the issue of member variables, which are modified and read in many different places. This means a “check the object is in a valid state” function is an useful addition to a class, to be used as a validity check during development to catch any errors as soon as they occur.

I sometimes use the following for my checks:

class Type
{
 public static function Is($value, $type)
 {
   if (func_num_args() > 2) {
     $args = func_get_args();
     array_shift($args);
     return self::Is($value, $args);
   }

   if (is_string($type))
     return self::Is($value, array_filter(explode(' ', $type)));

   if (empty($type))
     return true;

   $first = array_shift($type);

   if ($first == 'null')
     return $value === null || self::Is($value, $type);

   if ($first == 'array') {
     if (!is_array($value))
       return false;
     $next = 0;
     foreach ($value as $key => $val) {
       if ($key != $next++)
         return false;
       if (!self::Is($val, $type))
         return false;
     }
     return true;
   }

   if ($first == 'time')
     return is_int($value) && $value >= 0;

   if ($first == 'hash') {
     if (!is_array($value))
       return false;
     foreach ($value as $val)
       if (!is($val, $type))
         return false;
     return true;
   }

   if (is_callable($first))
     return call_user_func($first, $value) && self::Is($value, $type);

   if (is_callable('is_' . $first))
     return call_user_func('is_' . $first, $value) && self::Is($value, $type);

   if (class_exists($first))
     return($value instanceof $first);

   return false;
 }

 public function checkTypes()
 {
   self::check($this);
 }

 public static function check($obj)
 {
   $class = get_class($obj);
   foreach (get_class_vars($class) as $var => $value)
     if ($var{0} != '_')
       if (!is($obj->$var, $value))
         throw new Exception("Type error: `$class::$var` is not of type `$value`");
  }
}

The typical use is to define a new class, then assign a default value to all type-checked variables: that default value is a type string (or array) that is parsed and verified by the check functions. For instance:

class User
{
  var $id = 'int';
  var $name = 'null string';
  var $media = 'array Media';
  var $friends = 'positive int';
  var $_hash;
}

This would check that the identifier is an integer, that the name is a string or null, that media is an array of instances of the Media class, and that friends is an integer such that is_positive($obj->friends) returns true (assuming you define that function somewhere). The hash variable is unchecked because it starts with an underscore. This has some advantages:

  • Type expressions are shorter than the corresponding assert statements.
  • They go deeper as far as checks go (for instance, arrays also check that all members are of a certain type).
  • They document the code, by explaining in the class definition what the types of the variables are, as opposed to staying in a function.
  • They help with automated testing by allowing the creation of classes with arbitrary values of the chosen type.

This also has disadvantages:

  • This prevents setting an actual default value for the variables.
  • It introduces an artificial naming convention for variables starting with or without underscores.
  • Type-checking arrays or large structures takes time.
  • It’s not detected by documentation generators.
  • Does not play well with private variables.

You’re not a person

WEEK 1

In this application, every person belongs to exactly one team.

WEEK 4

We need to manage external contractors. We could use the “person” object.

WEEK 5

Hey, we need to assign a team to every person. Let’s create an “external” team.

WEEK 127

Did you see that newspaper article about our company? They say we have an average of 30 people on every team. Do we even have 30-people teams?

Names are short. They can only convey a very limited amount of information. Even worse, that information tends to be different from its meaning in standard English: by declaring in week one that every person belongs to a team, the project designers separated the Application::Person (always in a team) from the English::Person (might be in zero, one or more teams). By week four, this separation vanished from the minds of most of the team. A developer noticed that “English::Contractor is-a English::Person” and mistakenly translated it to “Application::Contractor is-a Application::Person“.

This was the first mistake. Why didn’t he notice?

A positive property is what you can do with a thing.With the Person object, you can store a name, login, password and phone number!This is exactly we you needed! Those positive properties you need that the object doesn’t provide, you can always add them through inheritance or composition, and that’s still less work than implementing everything or having to refactor the code. A negative property is what you cannot do with an object no matter how hard you try. With the Person object, you cannot remain on your own without a team! But our brains are biased to look for positive properties first, and passively ignore negative properties until it’s too late. Positive properties are about the solution solving the problem. Negative properties are about the solution not being applicable.

The second mistake was, by far, the worst. So they finally noticed that negative property that blasted all their model away. And they went on with it, patching the issue by altering the meaning of Application::Team. It originally a project team within the company, it then represented a named group of people that could be a project team or the group of external contractors. This is refactoring: no matter how you look at it, you change the behavior of an object and let it propagate throughout the project, so you better be careful about where it propagates! In this case, they weren’t careful about propagating the change of meaning to the documentation and user interaction part of the project, who mistakenly kept the old meaning of Application::Team. This led to a naive PR team issuing a statement that included the “external” group as if it were a project team.

It’s always helpful to have an anal-retentive person in a group, preferably in a position of authority that lets them veto such changes, and who is vigilant enough to spot that “external” team early on in the design.

The real mistake was allowing a negative property to slip into the design. Negative properties hinder reuse, by definition. Sure, allowing a person to belong to zero-one-many teams is hard on every piece of code that must work on teams, because the writers have to remember to check whether the person has a team in the first place. But it has to be done. Doing it may even bring to light some issues in the original requirements (“So what happens when a person changes teams between the moment team bonuses are computed and the moment they are paid out?”) that would become annoying later on.

Find the bug!

This code summarizes some text by removing anything past 255 characters. It’s done before inserting things into a database, so that the brutal cut of a VARCHAR(255) doesn’t leave a truncated string but rather three nice dots.

function summarize($text)
{
  assert (is_string($text));
  if (strlen($text) < 255) return $text;
  return substr($text,0,253).'...';
}

Yet, however simple it might be, this function contains a bug. One day, your database will return a weird error when an user tries to save the data. If you’re lucky, it will happen to a lone user who will complain and move on. If you’re unlucky, it will happen during a million-line import and crash everything, or it will display strange things on the screenof every user around.

Can you find why?

Continue reading ‘Find the bug!’

Easier Unit Tests

Automated unit testing has three main advantages:

  1. It forces you to express in detail what you want the unit to do (so that a computer may then test it by checking the results).
  2. For an unit to be testable, its results should be checked as “correct” or “incorrect” on their own or with minimal context. This makes code easier to reuse.
  3. Since it’s automatic, you do not need to test manually every time you make a change: the tests will run hourly or nightly (or for every build) and tell you what went wrong.

The cost, however, is that unit testing code must be written for every unit. That code needs to create an object, manipulate it, then check its return values for validity. And test-driven-development advocates insist that such code should be written before the actual unit (make it compile, make it fail, make it pass).

I do understand their position: if you’re thinking of very specific corner cases that you might forget about while writing the unit, you might as well write tests for these corner cases first. But what about the simple, core functionality of the units? If a programmer sets their minds to testing an unit, they can usually look at the value and decide “it’s correct” or “it’s wrong”, and that process is orders of magnitude faster than writing an assertion that checks whether the value is correct. We’re not talking about complex ten-argument dozen-property function on a deep-inherited object here, it’s more of a “this string goes in, that string goes out” concept.

An example would be a “slugify” function : plug an arbitrary string into it, and a cleaned up lowercase hyphen-separated string comes out. As long as you are mindful of corner cases (weird characters, encodings, empty strings and so on) testing this function manually is quite simple. You certainly lose the benefit of point 1 (no more expressing the details of what the function should do in advance of writing it) but you can keep the benefits of 2 (whether automatic or manual, tests make code reusable) and you can even get the benefits of 3 by saving your code as an unit test.

This is what happens in the console-like scaffold I have been working on (you can click to enlarge):

autotest

You type in code in the text box. Clicking “Run” (or pressing TAB RET) sends the code through AJAX to the server and retrieves the var_dumped result (as well as a highlighted version of the code). The mindful programmer can use this console to preemptively test and debug code. The excellent programmer will even adapt their code to make such testing easier, and being able to use any kind of code in the barren context of this console means that code can be used anywhere.

The real hit there is that little Add Cog icon from the FamFamFam Silk icon set. It’s truly beautiful and adds a touch of antialiased pixel art to an otherwise bland rounded-edges Web 2.0 console. And what it does is even better: when clicking that icon, the corresponding piece of PHP is automatically added to the database of unit tests, as a test that runs the code on the green background and asserts that the output of the code is identical to the text on the grey background.

This makes the “for every bug you find, write an unit test that finds that bug” insanely easy to do: find the bug using the console, correct the code, check with the console that the bug is dealt with, and then add the final “bug corrected” console run to the unit test database. The work spent debugging a program from this console is never lost, it all ends up being an automated test with one single click.

This also makes writing unit tests much more developer-friendly: the console lets you test your code as you write it, without having to write controllers or views or whatever other contraption you need to display the results. An excellent choice for testing-averse developers.

Nice One

void User::DeleteAccount()
{
  // You cannot delete accounts. It's illegal, we must keep a trace of every
  // account on the system. Use User::DisableAccount() instead. 

  // Why is there a DeleteAccount() function then? Because once upon a
  // time, when there was no DeleteAccount() function, a smartass though
  // "hey, they forgot to write a DeleteAccount()" and promptly wrote it
  // himself. 

  // So, this function remained here as a warning for you: you obviously
  // didn't get the "you cannot delete accounts" memo, because you came
  // looking for a function to do just that. Do not try to delete
  // accounts, my friend. Do not stray from the righeous path. Disable
  // the accounts instead.

  assert (false);
}

Expect the Unexpected

When looking at a function declaration, there are several levels of abstraction one can use to describe what that function does.

The actual action of that function is what really happens. This includes any bugs the function may contain and any undocumented behavior that is subject to change in later versions.

The documented action of the function is what the author of the function intended to do with that function. This includes a complete description of what the function should reasonably be expected to do, what conditions may trigger an error, and what external factors may affect the outcome.

The expected action of the function is what the user of the function expects the function to do. This is the action that matters most of the time, since there are often many users for every function.

In an ideal world, all three actions would be identical: the author implemented the function to do exactly what was documented and the documentation covers all behavior and explicitly marks all unspecified elements, the user has read the documentation and understands it completely.

In the real world, those actions are all different. The difference between the actual action and the documented action is either a bug (the function does not behave as documented) or the documentation being too vague and leaving things implicitly unspecified. The difference between the expected action and the documented action happens because the user has not read, or understood, all the nuances of the function’s behavior as described in the documentation.

Breaking the Mental Model

The classic example of the latter difference in understanding is the strtolower function:

When we convert the string “integer” to upper and lower case in the Turkish locale, we get some strange characters back:

"INTEGER".ToLower() = "ınteger"
"integer".ToUpper() = "İNTEGER"

The user is not aware that strtolower depends on the current locale, because their mental model of the strtolower function turns every uppercase letter of the occidental latin alphabet into its corresponding lowercase letter in that same alphabet. Of course, this is not what happens, and there is no way of “getting” this fact straight without thoroughly reading and remembering the entire documentation of the strtolower function.

The best we can do, as function authors, is to make it woefully obvious to users of that function when they misunderstand the function.

But, you say, the only way to detect most non-trivial function misuses is through complete testing, and it’s quite probable that the user will not think of the test cases that would break their mental model!

This is correct, and this precisely why I said misunderstand and not misuse. Determining whether or not a function is used correctly is something that the user can do quite easily once they get a correct mental model of that function, so we’ll let them do exactly that. The point here is to make the function as hard to use as possible when you don’t understand it completely.

Consider the strtolower function. If you don’t understand that locale can affect the operation performed by that function, then you are going to get things wrong. A nice way to ensure you understand this is to make the locale a mandatory argument of the function. By telling the user “you need to specify a locale before using this function” you are breaking the mental model of any user that expected the function to be locale-independent, and that is a good thing.

Exceptional Situations

There is an interesting gradient of mental-model-breaking in the handling of exceptional situations:

Handling Method Always When fails
No handling (ASM, C++ undefined behavior No No
Return codes (C APIs) Weak Weak
Exceptions Weak Strong
Java Exceptions Medium Strong
Type System Strong N/A

Here, I’m discussing the ability for a given handling method of breaking an incorrect mental model in two situations : “always” means whenever the function is used, “when fails” means whenever the function is used incorrectly in a fashion that interrupts the normal course of execution.

When the function is used, the existence of exceptional situations is mentioned as weak (only in the documentation), medium (compiler error that is not very specific) or strong (specific, reliable compiler error). When a failure occurs, the result is weak (depends on user action) or strong (independent of user action).

As such, using the type system appears to be the strongest means of describing the existence of exceptional situations. How?

In a functional language, every function returns a result. There is no point in computing a result unless that result is used, which means every function result is used somewhere in the code. As such, having functions that may encounter errors return an “Error or Success” type forces the user of the function to handle the possibility of an error before they get the result.

This is precisely how Objective Caml avoids the very possibility of a “null reference” runtime error : the option type has to be explicitly turned into a value, which means that pattern matching must be used and therefore the null case has to be handled as well:

let frobnicate option =
  match option with
    | Some value -> work_with value
    | None -> work_without_value ()

Dealing with Programmers

The problem is that programmers are humans and humans are lazy. Nobody wants to spend additional time designing the type of a function just to prevent misunderstanding of that function (unless it’s an API, of course) and nobody wants to have to type an additional argument to a function.

In fact, the entire convention over configuration philosophy relies on the idea that programmers should have to make as few decisions as possible. But adding default values for every argument is dangerous if programmers are not aware that those arguments exist—choosing a sane default value implies that such a value exists and is the one most programmers have in their own limited mental models for that behavior.

And if no consensus exists, using a default value is impossible: a programmer would expect strtolower to work in the current locale by default, while another would expect strtolower to work in an invariant locale by default. Choosing a default locale means that one of these two programmers is wrong and leads to bugs. It certainly is the programmer’s fault for not reading the documentation properly, but one could argue that a successful library is one that produces great results even in the hands of less competent programmers.



1170 feed subscribers
(readers who polled a feed this week)