/ secure code

How Safe Is This Code?

A Recent code example I found online got me thinking about how explicit should our security procedures be, or in other words that one should still use best security practices even when the framework provides these tests automatically. Below you'll find the example and my reasoning. As always would love to hear your opinion too.

The Code

This time we're dealing with ruby and this following snippet shows how to handle file upload in Sinatra:

post '/save_image' do  
  @filename = params[:file][:filename]
  file = params[:file][:tempfile]

  File.open("./public/#{@filename}", 'wb') do |f|
  erb :show_image

Code from https://gist.github.com/runemadsen/3905593.

Easy to see the file name is not tested for fishy characters (such as .. and /). This was suspicious even for an online example.

Of course sinatra uses Rack which handles the file upload sanitation for you. Rack::Multipart::UploadedFile includes this line:

@original_filename = ::File.basename(path)

But the question remains: Should the original code example also call basename? I think it should and for the following reasons.

General Protection Is Rarely Sufficient

Security best practices of the framework are great in reducing easy hacks, but they're also too good at giving us a false sense of security. The above code may not be vulnerable to directory traversal attacks, but it's far from secure. Users may upload any file type in any size, they may overwrite other users' files and probably a dozen of other nasty things.

Relying on the framework alone means we don't even get a chance to think what could go wrong, and without it no system could be secure.

By explicitly checking the file type (and sanitizing its name), we create a commitment of what we allow or don't allow in the application.

Code that includes explicit sanitization and security checks will be easier to discuss. And easier for one of our team members to step up and notice we should allow this or that file type, or we don't check size.

When all checks are implicit, everyone thinks all checks are somewhere. Where? I dunno, in Rack, in Sinatra, somewhere.

Portability: Code tends to be copied around

And that's a problem for implicit security mechanisms, since the new framework or environment might not have them.

Here's the same code in PHP:

// In PHP versions earlier than 4.1.0, $HTTP_POST_FILES should be used instead
// of $_FILES.

$uploaddir = '/var/www/uploads/';
$uploadfile = $uploaddir . basename($_FILES['userfile']['name']);

echo '<pre>';
if (move_uploaded_file($_FILES['userfile']['tmp_name'], $uploadfile)) {
    echo "File is valid, and was successfully uploaded.\n";
} else {
    echo "Possible file upload attack!\n";

echo 'Here is some more debugging info:';

print "</pre>";

As you can notice PHP has no implicit call to basename and that call should certainly be there. If anyone would just "translate" the ruby code they might forget to call basename even in the PHP version where it's required.

Habits Grow On You

Writing secure code explicitly over and over again reminds us that bad things can happen to our app, and help us talk about the security considerations of our code.

On the other hand relying on the framework / firewall / environment to help us tends to expand too, and makes writing insecure code more common (and eventually even in places where framework protection is missing).

A good habit in writing code would be that the part in code that does the dangerous thing should protect its inputs. Opening a file based on external input without sanitizing the input is something we should not expect to see in examples or real world code. This should be a code smell, an indication that something's wrong.

By always sanitizing input before dangerously using it we increase our security orientation and eventually get used to writing better, more secured code.

What do you think? Any good reasons not to explicitly sanitize inputs (even when you know they're safe) ?