Re: matching password problem

From: Jeff 'japhy' Pinyan (japhy_at_perlmonk.org)
Date: 03/14/05 

Date: Mon, 14 Mar 2005 10:44:23 -0500 (EST)
To: Jame Brooke <muarlove@yahoo.com.hk>

On Mar 14, Jame Brooke said:

> Program password.pl

This program is rather poorly written, and appears to have been "inspired"
by some very old Perl programs lying about on your server.

> #!/usr/local/bin/perl
>
> #windows environment
>
> #Read in Parameter passed
>
> require "cgi-lib.pl";
>
> &ReadParse (*input);

Eww. EWW. Use the standard CGI module, and turn on warnings. I'm also
going to suggest turning on strictures, which will require you to write
safer code. In this case, it primarily means declaring your variables
with my():

   #!/usr/bin/perl

   use CGI;
   use strict;
   use warnings;

   my $input = CGI->new; # create a new CGI object from the form data

> $name_status =$input{'name'}; #Read User ID from HTML form
> $pass_status =$input{'pass'}; #Read User Password from HTML form

   my $name = $input->param('name');
   my $pass = $input->param('pass');

> open(PASSWORD,"password.dat");
>
> $pass_count=0;
>
> while (!eof(PASSWORD)) {
>
> $name[$pass_count] = &get(PASSWORD);
> $pass[$pass_count] = &get(PASSWORD);
> $pass_count++;
>
> }
>
> close (PASSWORD);

First, I'm going to clean this code up, and then I'm going to explain why
it's the wrong way to do things. Let's look at the get() function:

> sub get {
> local ($handle)= $_[0];
> local ($str)="";
> local ($ch);
>
> while ($ch ne "\n") {
> $ch=getc($handle);
> $str=$str.$ch
> }
>
> return $str;
> }

What is this, C? This function is a waste of time to write, and a waste
of time to use. Instead of called get(PASSWORD), do <PASSWORD> instead.

   my (@names, @passwords);

   # when you try opening a file, give an error if you fail!
   open PASSWORD, "< password.dat" or die "can't read password.dat: $!";
   while (my $n = <PASSWORD>) {
     my $p = <PASSWORD>;
     chomp ($n, $p); # remove the newlines from the end
     push @names, $n;
     push @passwords, $p;
   }
   close PASSWORD;

> #Check Whether the user name and user password match with password file or not
>
> print "Content-type: text/html\n\n";
>
> print "
>
> <html>

You should use a here-doc, it's nicer-looking:

   print << "END_OF_HTML";
Content-type: text/html

<html>
...
<table>
END_OF_HTML

> for($i=0; $i<$pass_count;$i++) {

This is a C-style loop over an array, and it's rather ugly.

> if ($name[i] = $name_status) {
> if ($pass[i] = $pass_status) {

Here's your big problem. You're missing the $ on $i, and you're using '='
instead of 'eq'. You need to compare strings, not set a variable to a
certain value.

   if ($names[$i] eq $name) {
     if ($passwords[$i] eq $pass) {
       # print success message
       last; # <-- THIS will exit the for loop
     }
   }

But here's how your code should really look. You don't need to store the
usernames and passwords in arrays in your code, because you only need to
look at ONE username and ONE password at a time. And if you DID want to
store them, you should use a hash, not an array.

Here's how I'd write the code:

   #!/usr/bin/perl

   use CGI;
   use strict;
   use warnings;

   my $query = CGI->new;
   my $name = $query->param('name');
   my $pass = $query->param('pass');

   # ...

   open PASSWORD, "< password.dat" or die "can't read password.dat: $!";
   while (my $n = <PASSWORD>) {
     my $p = <PASSWORD>;
     chomp ($n, $p);

     if ($n eq $name) {
       if ($pass eq $p) {
         # the names match and the passwords match
       }
       else {
         # the names match but the passwords don't match
       }

       last; # stop looping over the file
     }
   }
   close PASSWORD;

Another problem I see here is that password.dat lives in the same location
as your CGI program (uh oh) and stores passwords in PLAIN TEXT (UH OH!). 

-- 
Jeff "japhy" Pinyan         %  How can we ever be the sold short or
RPI Acacia Brother #734     %  the cheated, we who for every service
http://japhy.perlmonk.org/  %  have long ago been overpaid?
http://www.perlmonks.org/   %    -- Meister Eckhart

Relevant Pages