CGI is Okay, but Bad Code is Irresponsible

| 8 Comments

Update: IBM dW and the author have improved the code; I've followed up at Politely Suggesting Improvements.

IBM's developerWorks published an article yesterday describing a simple Ajax web login service.

The original code was horribly insecure, Bobby Tables-style "Anyone can log in without knowing a password merely by manipulating the query parameters" insecure. Fortunately, IBM fixed the code. Unfortunately, it's still bad Perl 5:

#!/usr/bin/perl
# BAD CODE DO NOT USE use CGI qw(:all);
# BAD CODE DO NOT USE use DBI;
# BAD CODE DO NOT USE use DBD::mysql;
# BAD CODE DO NOT USE
# BAD CODE DO NOT USE # read the CGI params
# BAD CODE DO NOT USE $CGI = new CGI();
# BAD CODE DO NOT USE $username = $CGI->param("username");
# BAD CODE DO NOT USE $password = $CGI->param("password");
# BAD CODE DO NOT USE
# BAD CODE DO NOT USE # connect to the database
# BAD CODE DO NOT USE $DBH = DBI->connect("DBI:mysql:database=mydb;host=localhost;port=2009",
# BAD CODE DO NOT USE                                         "mydbusername", "mydbpassword")
# BAD CODE DO NOT USE   or die($DBI::errstr);
# BAD CODE DO NOT USE
# BAD CODE DO NOT USE # check the username and password in the database
# BAD CODE DO NOT USE $query = qq{SELECT id FROM users WHERE username=? && password=?};
# BAD CODE DO NOT USE $query = $DBH->prepare($query);
# BAD CODE DO NOT USE $query->execute($username,$password);
# BAD CODE DO NOT USE $ref = $query->fetchrow_hashref() || 0;
# BAD CODE DO NOT USE $userID = $ref->{"id"};
# BAD CODE DO NOT USE
# BAD CODE DO NOT USE # create a JSON string according to the database result
# BAD CODE DO NOT USE $JSON = ($userID) ?
# BAD CODE DO NOT USE         qq{{"success" : "login is successful", "userid" : "$userID"}} :
# BAD CODE DO NOT USE         qq{{"error" : "username or password is wrong"}};
# BAD CODE DO NOT USE
# BAD CODE DO NOT USE # return JSON string
# BAD CODE DO NOT USE print qq{Content-type: application/json; charset=utf8\n\n};
# BAD CODE DO NOT USE print $JSON;

Perhaps "bad" is too strong a word. Here's that code cleaned up and written with a better foundation for writing real code. (After all, is this article not intended to give people a starting point for writing real code?)

#!/usr/bin/perl -T

use strict;
use warnings;

use CGI;
use DBI;

my $q        = CGI->new;
my $username = $q->param("username");
my $password = $q->param("password");

my $dbh = DBI->connect(
    'DBI:mysql:database=mydb;host=localhost;port=2009',
    'mydbusername', 'mydbpassword'
) or die $DBI::errstr;

my $sth = $dbh->prepare(
    'SELECT id FROM users WHERE username=? and password=?;'
);
$sth->execute($username, $password);

my $json;

if (my ($id) = $sth->fetchrow_array)
{
    $json = qq|{"success" : "login is successful", "userid" : "$id"}|;
}
else
{
    $json = qq|{"error" : "username or password is wrong"}|;
}

print $q->header(
    -type    => 'application/json',
    -charset =>  'utf-8',
), $json;

The differences are minor, but the corrected version of the code is a little less clunky. It follows the Perl style guide more closely (using the idioms of both the CGI and DBI modules) and avoids unnecessary work.

Is this nitpicky?

Perhaps. Perl is nothing if not pragmatic... but if you're going to publish example code used as the core of a login system, why wouldn't you write the best code you possibly can? It may be acceptable to disclaim responsibility for even explaining the security problems of storing plaintext passwords in the database, but posting code vulnerable to SQL injections in 2011 is irresponsible.

It's okay to write baby Perl, but it's baffingly irresponsible to publish said baby Perl as an example from which you expect other novices to learn.

It's almost like publishing home chemistry experiments for second graders where half of the instructions are "Mix together household cleaning ingredients. Be careful; some of them might be dangerous if you mix them together, but that's not the point of these instructions, so you are on your own."

With that said, some people have criticized the example for the use of the CGI module and the CGI mechanism for dynamic web programs in 2011—but that's a well-intentioned if wrong-headed criticism.

Certainly Plack offers an easier mechanism of experimentation and example deployment (which scales to production-ready deployment easily), once you understand Plack and have it installed and configured for your application...

... but for didactic purposes, as a baseline execution model which is relatively easy to understand due to its maturity and ubiquity, CGI is a decent choice for deployment.

The Perl world does need more articles and posts and examples of how to use Plack to replace the CGI model in all of its forms, but the irresponsibility in publishing this article rests with the editors who allowed the further propagation of bad Perl cobbled together.

8 Comments

Thank you for having the patience to write this up.

The saddest part of the author's response is where she said she tried generating an error case mithaldu pointed out, but "the compiler didn't complain (unless I used strict)". So she knows about "use strict", but considers its use an unusual situation? And she's trying to teach other people how to write Perl code?

The quality and applicability of the IBM technical articles is often quite good and I really appreciate their site so I hope we can constructively discuss/guide this kind of thing without making the next author whose Perl might not be strong say, "F-that, I'm writing my example in PHP because the Perl community jumps down your throat if you use a semi-colon in the wrong place.

That said, the rewrite seemed great.

I agree! It's important to distinguish between writing baby Perl (which is okay) and giving other people bad examples from which to copy. It's really a simple test, too.

Perhaps I should write a guide to evaluating Perl 5 code examples.

It's better to set RaiseError => 1

I mostly agree, but I wanted to make the minimal responsible changes to the program to improve its modernity. I also prefer to have connection information outside of my programs, but that's also a larger change.

There is still a big security hole in this code: the web server must have as much access to the database as the any user, which is to say that anyone who can execute some sql code can do anything that the most privileged user can do. Furthermore, the web servers database credentials must be stored on the web server (in this case hard coded and well documented). A single bad moment of delivering perl scripts as text instead of executing means you've just published some powerful login credentials. Did you notice they got downloaded? Did you have time to change the password before TempEmployee Ted dirtied his hands in your data?

Additionally, using an application table "users" with fields "id", "username", "password", and possibly some other application specific columns (eg: "role"), means that your application must duplicate authentication and authorization which is almost certainly better implemented in the database server.

A better solution is to use the gathered credentials ("username" and "password") as arguments to $DBI->connect. That way gaining control of the web server doesn't give you full access to the database, and if TempEmployee Ted decides to fool around with SQL injection, he will never be able to access any information he couldn't access with the web application (assuming the database is properly locked down).

Furthermore, using the users credentials to log in rather than using a single full access login for the web application allows for the possibility of a spreadsheet or other desktop application having access to the database using the same credentials with exactly the same permissions.

So either IBM's servers broke and eradicated the comment system (newly posted comments don't show up either) or they deleted all comments on this article. [ http://www.ibm.com/developerworks/webservices/library/ws-simplelogin/index.html ]

Modern Perl: The Book

cover image for Modern Perl: the book

The best Perl Programmers read Modern Perl: The Book.

affiliated with ModernPerl.net

Categories

Pages

About this Entry

This page contains a single entry by chromatic published on January 28, 2011 10:37 AM.

Your EDSL is only Pretty in Stockholm was the previous entry in this blog.

The Method Keyword, take two is the next entry in this blog.

Find recent content on the main index or look in the archives to find all content.


Sponsored by Blender Recipe Reviews and the Trendshare how to invest guide

Powered by the Perl programming language

what is programming?