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

Hey all,
I've rewritten my DB interface, this time using the CGI and Apache::Htpasswd modules, hopefully making it entirely secure.
Thanks to all who gave me advice in the past, and in advance to those who comment on this one :)

Here's my code, copied and pasted this time. 

#!/usr/bin/perl -w
$ENV{'PATH'}='/var/www/cgi-bin/:/bin/:/usr/bin/';
$update="was created by ";
use CGI;
use Apache::Htpasswd;
$query=new CGI;
print $query -> header;
$login_used=`id`;
$date=`date`;
chomp($login_used);
chomp($date);
&heredocs;
sub heredocs{
$illegal=<<ILLEGAL;
    <h2>Illegal login</h2>
    <br>
    <p>
    Some information you submitted, (either your name, the login you w
+ant to create, or the password for this login), contains illegal char
+acters, which for security are not accepted.&nbsp;&nbsp;Please <a hre
+f="../passwords.htm" OnMouseOver="window.status='Go back to the RADB 
+admin page'; return true" OnMouseOut="window.status='';">Go back to t
+he RADB admin page</a>&nbsp; and try again, using only alphanumeric c
+haracters.<p></p>
ILLEGAL
$copy= <<COPY;
    <br>
    <br>
    <br>
RADB created by and © Pine Tree Internet Solutions.  Unauthorized use 
+is prohibited.
    <br>
    <br>
    <a href="/passwords.htm" onMouseover="window.status='Go back to th
+e RADB Admin page'; return true" OnMouseOut="window.status=''">Go bac
+k to the RADB admin page.</a>
COPY
$former=<<FORMERCHGPW;
    <br>
    <h2>Add another user:</h2><br><br>
    <form name="jim" method="post" action="../cgi-bin/passwords.cgi">
      <input type="hidden" name="chp" value="on">
      <input type="hidden" name="chpw" value="off">

Your name:&nbsp;&nbsp;
      <input type="text" name="name">
      <br>
Login you want to create/edit:&nbsp;&nbsp;
      <input type="text" name="login">
&nbsp;&nbsp;&nbsp;&nbsp;Desired password for this login:&nbsp;&nbsp;
      <input type="password" name="pw">&nbsp;&nbsp;&nbsp;&nbsp;
      <br>
If changing an existing user's password, please check the &#8220;Chang
+e password&#8221; box below and enter that user's <b><u>old</u></b> p
+assword in this box:&nbsp;&nbsp;<input type="password" name="oldpw" d
+isabled>
      <br>
This user already exists, I'm changing the password associated with th
+is login.&nbsp;&nbsp;<input type="checkbox" name="changepw" onClick="
+javascript:pwchanger()">
      <br>
      <br>
      <input type="button" value="Submit changes" OnClick="javascript:
+submitter()">
    </form>
    <br>
    <br>
FORMERCHGPW
$pwr=<<PWR;
    <h2>Password rejected!</h2>
    <br>
    <br>
    The password associated with user $untainted_formlogin could not b
+e changed, as it could not be verified that you possess the authority
+ to perform this action.  Perhaps you forgot your password?

PWR
$footer = <<FOOTER;
RADB V1.0 created by:<br>
    <a href="http://www.pinetreenet.com/" onMouseOver="window.status='
+Pine Tree Internet Solutions homepage'; return true" onMouseOut="wind
+ow.status=''">
    <img src="../ptis_title_small.jpg" alt="Pine Tree Internet Solutio
+ns" border=0></a>
    <br>
    <font size="-2">RADB v1.0 &copy; 2003, Pine Tree Internet Solution
+s.&nbsp;&nbsp;
      No part of the RADB may be reproduced without explicit permissio
+n from an authorized Pine Tree Internet Solutions representative.
    </font>
    <br>
    <br>
    <img src="../poweredby.png" alt="Powered by Apache, running on Red
+ Hat Linux 7.3 (Valhalla)">
  </body>
</html>
FOOTER
$rej=<<REJECTED;
    <h3>Login rejected</h3>
 <br>
    <br>
    <p>Login $untainted_formlogin is already in use, or is too similar
+ to another existing login.  Please try again.<br><br><p>
    <br>
    <br>
    <br>
    <A HREF="javascript:window.history.back(1)" onMouseover="window.st
+atus='Go back to the RADB Admin page'; return true" OnMouseOut="windo
+w.status=''">Go back to the RADB admin page.</a>
    $former;
    <br>
    <br>
    $footer;
REJECTED
$copy= <<COPY;
    <br>
    <br>
    <br>
RADB created by and © Pine Tree Internet Solutions.  Unauthorized use 
+is prohibited.
    <br>
    <br>
    <a href="/passwords.htm" onMouseover="window.status='Go back to th
+e RADB Admin page'; return true" OnMouseOut="window.status=''">Go bac
+k to the RADB admin page.</a>
    <br>
    <br>
COPY
}
&script;
sub script{
print <<SCRIPTER;
<html>
  <head>
    <title>RADB v1.0 modification results</title>
    <meta http-equiv="pragma" content="no-cache">
    <meta http-equiv="Cache-Control" content="no-cache">
    <meta http-equiv="Expires" content="0">
    <script language="javascript">

      function submitter(){
        if(document.jim.name.value==""){
          alert("Please fill in your name.");
        }
        else if((document.jim.login.value=="") || (document.jim.login.
+value.length < 2)){
          alert("Please provide the login you\'d like to edit.  Logins
+ must be two characters or greater in length.");
        }
        else if(document.jim.pw.value==""){
          alert("Please provide a password for the user you'd like to 
+add to the database.");
         }
         else if ((document.jim.pw.value.length > 0)&& (document.jim.p
+w.value.length < 6)){
           if(confirm("For security, passwords should be six character
+s or more in length.\\nYour password is " + document.jim.pw.value.len
+gth + " characters in length.\\nDo you still want to use this passwor
+d?")){
              alert("Password is too short, but has been accepted");
              document.jim.submit();
              alert("Your changes to the database have been saved, and
+ logged for record.");
          }
          else
          {
            document.jim.pw.focus();
          }
          }
          else
          {
            if(document.jim.changepw.checked=="true"){
              alert("User " + document.DBEdit.login.value + "'s passwo
+rd has been changed.  Please update your records.");
          }
          document.jim.submit();
          alert("Your changes to the database have been saved, and log
+ged for record.");
          }
          }
    function logger(){
      document.jim.viewdb.value="on";
      document.jim.submit();
      }
    function DBviewer(){
      document.jim.viewlog.value="on";
      document.jim.submit();
      }
    function pwchanger(){
      if(document.jim.chp.value=="on"){
        document.jim.chp.value="off";
        }
      else
        {
        document.jim.chp.value="on";
        }

              if(document.jim.chp.value=="on"){
       document.jim.chpw.value="off";
       document.jim.oldpw.disabled=true;
       }
       else
       {
       document.jim.chpw.value="on";
       document.jim.oldpw.disabled=false;
       }

       }
    function logdel(){
      if(document.jim.name.value==""){
        alert("Please enter your name.");
      }
      else
      {
        if(confirm("Are you sure you want to delete " + document.jim.l
+ogins.options[document.jim.logins.selectedIndex].value + "?")){
          alert("User " + document.jim.logins.options[document.jim.log
+ins.selectedIndex].value + " has been deleted");
          document.jim.del.value="yes";
          document.jim.submit();
          }
          else
          {
          alert("No changes have been made to the database.");
          }
          }
          }

    </script>
  </head>
  <body>
</script>
  </head>
  <body>
SCRIPTER
}
$filepath=$ENV{'DOCUMENT_ROOT'};
$logpath="$filepath/.secure";
$htp=new Apache::Htpasswd("$logpath/.htpasswd");
$query=CGI->new();
                                                      @names=$query->p
+aram;
$count=0;
foreach $name(@names){
@values[$count]=$query->param($name);
if ($name eq "name"){
$nameofuser=$values[$count];
}
elsif ($name eq "login"){
$formlogin=$values[$count];
}
elsif ($name eq "pw"){
$formpw=$values[$count];
if(($formlogin =~ /%3A/) || ($formlogin =~ /%2F/) || ($formlogin =~ /%
+3B/) || ($formpw =~ /%3A/) || ($formpw =~ /%3B/) || ($formpw =~/%2F/)
+ || ($nameofuser =~ /%3A/) || ($nameofuser =~ /%3B/) || ($nameofuser 
+=~ /%2F/)){
print $illegal;
die("Data contained malicious characters");
}
}
elsif ($name eq "logins"){
$delvalue="yes";
$deluservalue=$values[$count];
}
elsif ($name eq "viewlog"){
$viewlogvalue=$values[$count];
}
elsif ($name eq "viewdb"){
$viewdbvalue=$values[$count];
}
elsif ($name eq "chpw"){
$chpw=$values[$count];
}
elsif ($name eq "oldpw"){
$oldpw=$values[$count];
}
else
{
$array[$count]=$value;
$count++;
}
$count++;
}
$nameofuser=~/([\w]+)/;
$untainted_name=$1;
                                                                      
+    $formlogin=~/([\w]+)/;
$untainted_formlogin=$1;
$formpw=~/([\w]+)/;
$untainted_formpw=$1;
$oldpw=~/([\w]+)/;
$untainted_oldpw=$1;
sub checker{
open(DB, ">>$logpath/.htpasswd")||die("Could not open DB file for view
+.");
@checker=<DB>;
foreach $checker(@checker){
($username, $pws)=split(/:/, $checker);
if(($username eq $untainted_formlogin) || ("\L\b$username\E" eq "\L\b$
+untainted_formlogin\E") || ("\U\b$username\E" eq "\U\b$untainted_form
+login\E")){
print "$rej $former <br> <br> $footer";
close(DB);
die("User already exists");
}
}
}
if ($viewdbvalue eq "on"){
&db_viewer;
}
elsif ($viewlogvalue eq "on"){
&log_viewer;
}
else
{
&log;
}
sub log{
if (-e ($logpath)){
&eraser;
}
else
{
mkdir("$logpath", 0755);
&eraser;
}
}
sub eraser{
  if ($delvalue eq "yes"){
    $htp->htDelete($deluservalue);
  print "Login $deluservalue been permanently removed from the personn
+el database by $untainted_name.";
                                                                      
+                  print "<br>";
  print "To undo this action, you must <a href=\"/passwords.htm\" onMo
+useover=\"window.status='Go back to the database admin page'; return 
+true\" OnMouseout=\"window.status=\'\'\">";
  print "go back</a> to the RADB admin page and re-enter the employee 
+mentioned above.";
  print "<br>";
  open(LOG, ">>$logpath/.log") || die("Could not open log file to add 
+\"delete\" entry.");
  $logupdate="User deleted";
  chomp($deluservalue);
  chomp($nameofuser);
  chomp($logupdate);
  print LOG "$deluservalue&$untainted_name,$login_used-$date¬$logupdat
+e\n";
  close(LOG);
  print $copy;
  print $footer;

}
else
{
&mainprog;
}
}

sub mainprog{
if (-e ("$logpath/.log")){
open(LOG, ">>$logpath/.log") || die("Could not open log file to add en
+try.");
}
else
{
open(LOG, ">$logpath/.log")|| die("Could not create log file at $logpa
+th/.log");
}
&usercreate;
sub usercreate{
unless(($viewlogvalue eq "on") || ($viewdbvalue eq "on")){
if($chpw eq "off"){
&checker;
}
else
{
$update="'s password was changed by ";
}
if($update eq "'s password was changed by "){
$logupdate="Password changed";
}
                                                    
else
{
$logupdate="Login created";
}
unless ($chpw eq "on"){
print "Login $untainted_formlogin $update $untainted_name,\nunder the 
+username $login_used,\non $date.\n";

print LOG "$untainted_formlogin&$untainted_name,$login_used-$date¬$log
+update\n";
$htp->htpasswd("$untainted_formlogin", "$untainted_formpw");
print $former;
}
if ($chpw eq "on"){
&passwordchanger;
}
else
{
&contin;
}
sub passwordchanger{
$htp->htpasswd("$untainted_formlogin", "$untainted_formpw", "$untainte
+d_oldpw");
if($htp->error() eq "Apache::Htpasswd::htpasswd - Password not changed
+."){
print "$pwr\n$copy\n$footer";
}
else
{
$htp->htpasswd("$untainted_formlogin", "$untainted_formpw", "$untainte
+d_oldpw");
print "Password for user $untainted_formlogin was successfully changed
+.  Please update your records.";
print $former;
print $footer;
print LOG "$untainted_formlogin&$untainted_name,$login_used-$date¬$log
+update\n";
}
close(LOG);
}
}

}
sub db_viewer{
unless ($viewdbvalue eq "off"){
open (DB, "$logpath/.htpasswd") || die ("Could not open DB file to vie
+w.");
@dbarray=<DB>;
$counter=0;
print "<form name=\"jim\" action=\"passwords.cgi\">";
                                                                      
+     print "<input type=\"hidden\" name=\"del\" value=\"no\">";
print "Your name:&nbsp;&nbsp;<input type=\"text\" name=\"name\">";
foreach $line(@dbarray){
($usern, $crypw)=split(/:/, $line);
while ($counter==0){
print "<select name=\"logins\">";
print "<option selected>Select a user:</option>";
$counter++;
}
print "<option name=\"fred\" value=\"$usern\">$usern</option>";
}
}
print "</select>";
close(DB);
print "<input type=\"button\" onClick=\"javascript:logdel()\" value=\"
+Delete user\">";
print "<br>";
print "</form>";
if ($viewlogvalue eq "on"){
&log_viewer;
}
else
{
&contin;
}
}
sub log_viewer{
$logupdater=$logupdate;
unless ($viewlogvalue eq "off"){
        print "<h2 align=\"center\">Users in database</h2>";
        print "<br>";
        print "<table border=\"6\" align=\"bottom\" bordercolor=\"#FF0
+000\">";
open (DB, "$logpath/.log") || die("Could not open $logpath/.log for ta
+bling");
@dbarray=<DB>;
print "<tr><td><b>Login</b></td><td><b>Added or modified?</b></td><td>
+<b>by user</b></td><td><b>Creator logged in as</b></td><td><b>Date</b
+></td></tr>";
foreach $entry(@dbarray){
($usen, $creatr)=split(/&/, $entry);
($creatir, $loggedinas)=split(/,/, $creatr);
($loggedas, $day)=split(/-/, $loggedinas);
($daychanged, $action)=split(/¬/, $day);
($creatr)=$creatir;
($loggedinas)=$loggedas;
print "<tr><td>$usen</td><td>$action</td><td>$creatr</td><td>$loggedin
+as</td><td>$daychanged</td></tr>";
                                                                      
+      }
print "</table><br><br><br><br>";

close(DB);
&contin;
}
}

sub contin{
print $footer;
}
}

[download]
That's it.
As I said, I hope it's secure this time, and I welcome any feedback.
Thanks,
-Jonathan

edited: Mon Jun 2 13:44:20 2003 by jeffa - readmore tag

Replies are listed 'Best First'.
Re: CGI DB interface, secured (hopefully)
by tedrek (Pilgrim) on Jun 02, 2003 at 02:56 UTC

    Go back and reread sauoq's suggestions from Re: CGI DB interface, have you done *all* of them. strict and Taint are very important, they help you from making mistakes in the first place. :)

    now for my suggestions,

    • Get some formatting, that is a bloody *pain* to read. if you want to get help at least make it easy to read.
    • throw *some* comments in there, *why?* are you doing things. I can tell *what* you are doing from the code. You'll thank yourself later.
    • &contin isn't very clear. something like show_footer() is, and you'll understand it much faster when you need to change something.
    • what's the point of a sub you only call once? eg. heredocs. That's all static data.
    • all your subs should be defined together, not when you need them, it's easier to read and understand.
    • Don't define subs inside other subs unless you have a really good reason.
    • here docs are great but once you get past a certain size it makes everything much nicer looking if you seperate code from content. Take a look at templating systems, I like HTML::Template, it's really easy to use.
    • why bother looking at every parameter? just grab the ones you want directly. much clearer and shorter too.
    • don't use global parameters unless you *need* to.

    I didn't notice any security problems there but I didn't look to closely either as it's so hard to read.

    Update: I reformatted the original script and found that the script itself does no authentication, I'm assuming that you protect it some other way. Anyway I reformatted it and added a bunch of comments, No it doesn't run now but it does give a number of pointers. Update beyond the readmore.

    Read more... (26 kB)
[reply]
[d/l]

Back to Seekers of Perl Wisdom