CGI script review

Anonymous Monk has asked for the wisdom of the Perl Monks concerning the following question:

Hi All

Can someone take a look at my code below. It accepts input from a form, and passes the data to another script. I know my code is sloppy, and would very much appreciate it if someone could perfect it for me.
I havn't been using perl for very long, and am sure some of my regexps and other things are broken (or possibly insecure)

Thanks

#!/usr/bin/perl -wT

use CGI;
use strict;

#####################
# Set Env Variables #
#####################
my $w          = new CGI;
my $redir      = new CGI;
my $time       = localtime;
my $ipaddr     = $ENV{'REMOTE_ADDR'};
my $message    = "";
my $found_err  = "";
$ENV{'PATH'}   = '/usr/sbin:/etc/adduser';
$ENV{'ENV'}    = '/usr/sbin:/etc/adduser';

########################
# Grab Input From Form #
########################
my $fullname   = $w->param('fullname');
my $username   = $w->param('username');
my $password   = $w->param('password');
my $mothers    = $w->param('mothers');
my $email      = $w->param('email');
my $referral   = $w->param('referral');
my $conditions = $w->param('conditions');

#############################
# Perform User Input Checks #
#############################
if ($fullname !~ /^[-.\w\s]{3,30}$/) {
    $message = $message.'<p>Full Name Must Be Between 3-30 Chars With 
+No Symbols.</p>';
    $found_err = 1; }
if ($username !~ /^[a-z][a-z0-9-._]{2,29}$/) {
    $message = $message.'<p>User Name Must Be Between 3-30 Chars With 
+No Symbols Or Upper-Case.</p>';
    $found_err = 1; }
if(defined(getpwnam($username))) {
    $message = $message.'<p>The Chosen Username Already Exists.</p>';
    $found_err = 1; }
if ($username =~ m/^administrator|accounts|support|postmaster|webmaste
+r|
    |spam-admin|technical|billing|sales|purchase|buy|misuse|
    |assistance|mail|virus-admin|manager|usenet|hostmaster$/) {
    $message = $message.'<p>The Chosen Username Already Exists.</p>';
    $found_err = 1; }
if ($password !~ /^[-.\w\s\d]{6,30}$/) {
    $message = $message.'<p>Password Must Be Between 6-30 Chars With N
+o Symbols.</p>';
    $found_err = 1; }
if ($mothers !~ /^[-.\w\s]{3,30}$/) {
    $message = $message.'<p>Mothers Maiden Name Must Be Between 3-30 C
+hars With No Symbols</p>';
    $found_err = 1; }
if ($email !~ /^([a-zA-Z0-9_\.\-])+\@(([a-zA-Z0-9\-])+\.)+([a-zA-Z0-9]
+)+$/i) {
    $message = $message.'<p>Please Enter A Valid E-Mail Address.</p>';
    $found_err = 1; }
if ($referral !~ /^[-.\w\s]{3,30}$/) {
    $message = $message.'<p>Referral Information Must Be Between 3-30 
+Chars With No Symbols.</p>';
    $found_err = 1; }
if ($conditions !~ m/^Yes$/) {
    $message = $message.'<p>Please Accept The Terms And Conditions.</p
+>';
    $found_err = 1; }

if ($found_err) {
        &PrintError; }

###############################
# Encrypt Password Using Salt #
###############################
my $salt      = '$1$' . join '',
               ('.', '/', 0..9, 'A'..'Z', 'a'..'z')
               [ map { int rand 64 } 0 .. 7 ];
my $epassword = crypt($password, $salt);

########################################
# Pass Details To AddUser For Creation #
########################################
!system ('/etc/adduser/adduser.pl', "-fullname=$fullname", "-username=
+$username",
                                    "-password=$epassword") or die;
   
########################################
# Send E-Mails To Support And New User #
########################################
open (MAIL, "|/usr/sbin/sendmail -t") or die;
print MAIL <<"END_OF_MAIL";
To: support\@domain.com
Reply-to: $email
From: $email
Subject: New User $username/$ipaddr

FullName  : $fullname
UserName  : $username
Security  : $mothers
Referral  : $referral
IP Address: $ipaddr

***End Of E-Mail***

END_OF_MAIL
close (MAIL);

open (MAIL, "|/usr/sbin/sendmail -t") or die;
print MAIL <<"END_OF_MAIL";
To: $email
Reply-to: support\@domain.com
From: support\@domain.com
Subject: Welcome To My Service

Dear $fullname,

blah blah blah blah

END_OF_MAIL
close (MAIL);

########################################
# Log User Create And Pass To Complete #
########################################
open  LOGFILE, '>>',"/etc/adduser/adduser.log" or die;
print LOGFILE "$time:$username:$password:$email:$ipaddr\n";
close LOGFILE;
print $redir->redirect('https://www.domain.com/complete.shtml');

###########################
# Display Creation Errors #
###########################
sub PrintError {
print "Content-type: text/html\n\n";
print $message;
exit 0; }
[download]

20030704 Edit by Corion: Added READMORE tag, changed title from "naff coding..."

Comment on CGI script review Download Code

Replies are listed 'Best First'.
Re: CGI script review by sauoq (Abbot) on Jul 04, 2003 at 09:09 UTC
Without going over the whole thing, I can say right off the bat that using a CGI to add users is not generally a good idea. Also, this will never be secure unless you run it under SSL because, otherwise, the supplied password could be sniffed. (Kudos for using -T though.) -sauoq "My two cents aren't worth a dime.";	[reply]
•Re: CGI script review by merlyn (Sage) on Jul 04, 2003 at 13:54 UTC
I haven't seen anyone else point out yet that you reject perfectly valid and in-use email addresses. Please look in to Email::Valid, and you'll also have to rethink your log and how you generate your confirmation message as a result. -- Randal L. Schwartz, Perl hacker Be sure to read my standard disclaimer if this is a reply.	[reply]
Re: CGI script review by EvdB (Deacon) on Jul 04, 2003 at 15:02 UTC
This code is crying out for separation of logic and presentation. The idea is that you put all the presentational stuff (ie HTML, error messages, email text) and put them in another file to the code. Look at Template for a good module for doing this. At first it will seem like real overkill to do it this way, but it leads to much cleaner and maintainable code. It also means that it is much easier to change the presentational stuff as it is separate. It is also much quicker to code this way. You can create an email message or HTML page to a fictional user (joe bloggs) in a normal editor, and then wher you are happy you change joe into `[% person.first_name %]` or whatever. --`tidiness is the memory loss of environmental mnemonics`	[reply] [d/l] [select]
Re: CGI script review by cLive ;-) (Prior) on Jul 04, 2003 at 09:53 UTC
`a-zA-Z0-9_ == \w` [download] I'd use an array mapped to a hash to check existance of user - easier maintenance `my %sysuser = map { $_ => 1 } qw(administrator accounts support postma +ster webmaster spam-admin technical billing sales purchase buy misus +e assistance mail virus-admin manager usenet hostmaster); if (defined $sysuser{$username}) {` [download] If you're loading CGI, you might as well use it for output: `$w->p('Please Accept The Terms And Conditions.'); print $w->header. $message;` [download] etc. From a style view, (personally), I'd use @error to store errors: `my @errors=(); ... push @errors, 'Password Must Be Between 6-30 Chars With No Symbols.';` [download] then check with `if (@errors) { print $w->header. join '', map { $q->p($_) } @errors; exit(0); } # you only need one cgi object $w->redirect();` [download] Note, most people use $q(uery) or $cgi for the CGI object you might want to do the same, especially when you want to read up CGI tutorials here. It might help :) Note - above is untested for exact syntax, but that's the general idea. And yes, use SSL! But overall, I think you've done a pretty good job if you're new to Perl :) .02 cLive ;-)	[reply] [d/l] [select]
Re: Re: CGI script review by Anonymous Monk on Jul 04, 2003 at 10:10 UTC
this script is stored on an ssl (https) website if thats what you mean?	[reply]
Re: CGI script review by sgifford (Prior) on Jul 04, 2003 at 18:36 UTC
You allow newlines in `$fullname` (they match `\s`), which is probably a really bad idea. You probably want an explicit space there (`TAB` and `CR` characters in your `passwd` file don't sound like any fun either). Fortunately, since you don't allow a colon I couldn't find a way to leverage this into a root login. Ditto for `$passwd`, although that's not so bad from a security standpoint (since you crypt it anyways). Keep in mind that anybody logged into the system running `ps` can see all of those command-line parameters, including the password. Better to pass them on a filehandle. Some part of this system must run as root. Is it your CGI script, or `adduser.pl`? If adduser.pl, how do you prevent other users from running this? Your pattern `[a-z0-9-._]` looks error prone. I would either move the `-` to the end, or else escape it with a backslash. Otherwise small changes could turn it into a range of characters, with possibly unfortunate consequences. Not allowing users to have symbols in their passwords makes them noticably less secure, simply because it reduces the possible characters in the password. All of my passwords have symbols in them, and I get extremely annoyed when I can't use symbols in passwords. There's a race condition; the user could not exist at the time `getpwnam` runs, but exist by the time `adduser.pl` runs. As long as `adduser.pl` handles this, it won't cause serious problems, but you could fix it altogether by returning a specific error code if the user already exists, and detecting this and giving an appropriate error message. All in all, though, a pretty good job. Use of `-T` is very important, along with `-w`, `use strict`, and the multiple-argument form of `system`. Mails are sent with `sendmail` instead of `/bin/mail` which is good---`/bin/mail`has been prone to shell escapes in the past and may still be. As somebody else said, you'll want to run this over SSL, but it looks like you already are.	[reply] [d/l]
Re: CGI script review by cjcollier (Novice) on Jul 04, 2003 at 09:51 UTC
sauoq has a point. I personally don't know how to use -T and so should be shot ;) But here's a diff, anyhow. http://colliertech.org/~cjcollier/perlmonks/newbie.diff	[reply]