|
michael.kitchen has asked for the wisdom of the Perl Monks concerning the following question:
I am trying to use numbers passed from a form element to a script and use it to open a file for writing where the numbers passed are part of the file name.
<input type=hidden name=event value=123456>
I successfully pass the data to the script and can display it on the webpage, but everything goes wrong when I try to use it to open (create) a file for writing. Here is most all the code:
#!/usr/bin/perl -wT
use CGI qw/:standard/;
my $event = param('event');
my $commentUID = time();
my $newCOMMENT = param('newCOMMENT');
if ($newCOMMENT ne "") {
open(AFH, ">> $event.txt");
print AFH "$commentUID\n";
close(AFH);
open(BFH, ">> $commentUID.txt");
print BFH "$newCOMMENT\n";
close(BFH);
}
The $commentUID.txt file is opened and written to just fine, but the form data passed to the script does not work at all. The most I could figure out is that the "." is being left out and 123456txt is trying to be opened...but all I get is an internal error. I have looked for a solution for several hours and cannot figure out how to do it. Please help. |
Re: open file using variable passed by form
by haukex (Archbishop) on Mar 17, 2018 at 10:03 UTC
|
I have a number of comments:
You should Use strict and warnings (also, since you're currently using -w, see What's wrong with -w and $^W). For debugging, you can run CGI.pm scripts from the command line, e.g. perl -T comment.cgi 'event=foo&newCOMMENT=bar' and you can inspect the contents of variables (such as the filename, if you were to store it in a variable) with modules like Data::Dumper - see also the Basic debugging checklist. When you are getting 500 server errors from CGI scripts, you should check the server's error logs for the exact error message. See also CGI Help Guide and Troubleshooting Perl CGI scripts (I've linked you to these before). There is also CGI::Carp's use CGI::Carp qw/fatalsToBrowser/;, although you should not use this in a production environment as it may reveal too many details of the server to an attacker. You should use the three-argument form of open, lexical filehandles, and always check the return value for errors, for example: open my $fh, '>>', $filename or die "$filename: $!"; Since you can't always be sure what environment CGI scripts will be run in, you should use absolute pathnames. But the biggest issue I see with this script is security. You are taking form input, completely unchecked, and using it directly in a filename. An attacker could inject arbitrary values here. At least you are using Taint mode (-T) - which is currently where your errors are coming from (and I think it's a good thing you're getting errors in this case...). But even so I would be very strict with the values that are allowed for those variables. (Also, what if $newCOMMENT contains newlines?) Using flat files is not necessarily the most efficient or safe way to do this. I see several race conditions here, such as $commentUID (what if there are multiple comments submitted within the same second?), opening two files separately (what if two processes are running concurrently? you may get out-of-order lines), and also no file locking going on. This would be a good place to use a database instead.
Now, having said all that, it's still possible to do something like what you want. Although as I said I wouldn't recommend using flat files, this is just for the sake of showing one way to do it. Note that I am only using a single file - but a warning about this - I'm using flock, which is not supported on all filesystems!
#!/usr/bin/perl -T
use warnings;
use strict;
use CGI qw/param header/;
use Fcntl qw/:flock :seek/;
my $COMMENTFILE = '/path/to/comments.txt'; # absolute pathname
if ( length param('newCOMMENT') ) {
# untaint form input with strict regexes
my ($event) = param('event') =~ /\A(\w+)\z/
or die "bad event";
my ($comment) = param('newCOMMENT') =~ /\A([\w\h]+)\z/
or die "bad comment";
# we use "---" as a marker below, so strip that out
$comment=~s/^---//gm;
open my $fh, '>>', $COMMENTFILE or die "$COMMENTFILE: $!";
# lock the file
flock($fh, LOCK_EX) or die "flock $COMMENTFILE: $!";
# in case someone else has written to the file in the meantime
seek($fh, 0, SEEK_END) or die "seek $COMMENTFILE: $!";
# I've just picked a format for the flat file
print $fh "--- ",time," ",$event,"\n";
# we know $comment doesn't contain "---" because of the regex
print $fh $comment,"\n";
flock($fh, LOCK_UN) or die "un-flock $COMMENTFILE: $!";
close $fh;
}
print header('text/plain');
print "Hello, World\n";
| [reply]
[d/l]
[select] |
|
|
You were right! My problem (part or all, not sure) was -T. I have something that is working (with -T) and it came from a combination of your code and poj's. I couldn't quite follow all of your code and was wondering if you might take a little bit more time and explain a couple of things for me. I truly like to understand what is going on, at least to some small degree. :)
if ( length param('newCOMMENT') ) # deciding if a new comment exists?
# untaint form input with strict regexes - don't need to know exactly
+what is happening
my ($event) = param('event') =~ /\A(\w+)\z/
or die "bad event";
# just wondering why code is different for 'newComment'
my ($comment) = param('newCOMMENT') =~ /\A([\w\h]+)\z/
or die "bad comment";
Again,thanks for taking your time to originally post and for any time spent replying to this one. And thanks for giving me a lot more to think about. FYI though, I now have plans for a better UID, plan on locking files while in use, and changing from textarea to standard text input box. | [reply]
[d/l] |
|
|
length param('newCOMMENT')
This avoids warnings when param returns undef (at least on Perl 5.12 and up).
As for the regexes, have a look at perlrequick and perlretut. Using them, I make sure that param('event') contains only "word" characters (which excludes things like dots, slashes, or backslashes), and I make sure that newCOMMENT includes only "word" characters and horizontal whitespace (which excludes, for example, newlines; see perlrebackslash and perlrecharclass for all the details). I then use capture groups to "untaint" the values (see perlsec). You may find that the regexes are too restrictive, in which case you can add allowed characters, but be very careful with this - adding too many or the wrong ones will open the security holes again. This is another good reason to not use form input for filenames and use a database instead, where these things are not an issue if you use the right tools - see Bobby Tables.
| [reply]
[d/l]
[select] |
Re: open file using variable passed by form
by poj (Abbot) on Mar 17, 2018 at 09:58 UTC
|
use CGI::Carp 'fatalsToBrowser'; # use only while debugging
to your script to see what is causing the internal error
or try this SSCCE which writes to /tmp
#!/usr/bin/perl
use strict;
use warnings;
use CGI qw/:standard/;
use CGI::Carp 'fatalsToBrowser'; # use only while debugging
use Cwd;
my $cwd = getcwd(); # current working directory
my $event = param('event') || 1;
my $newCOMMENT = param('newCOMMENT') || 'test comment';
my $commentUID = time();
my $path = '/tmp';
my $fileA = "$path/$event.txt";
my $fileB = "$path/$commentUID.txt";
if ($newCOMMENT ne "") {
open AFH, '>>', $fileA
or die "Could not open $fileA : $!";
print AFH "$commentUID\n";
close AFH;
open BFH, '>>', $fileB
or die "Could not open $fileB : $!";
print BFH "$newCOMMENT\n";
close BFH ;
}
print header,start_html;
print pre("
cwd : $cwd
event : [$event]
newCOMMENT : [$newCOMMENT]
commentUID : $commentUID");
print end_html;
poj
| [reply]
[d/l]
[select] |
|
|
| [reply]
[d/l] |
|
|
| [reply] |
|
|
Thank you so much for taking time to help. It seems that -T was killing my script and used to code from haukex. Used (and learned from) some of your code too. I have a working script (at this point in time). You may see it again for another reason. :)
For some reason I could not open a file (for writing) in /tmp, but was able to in a new sub-directory of cgi-bin...go figure.
Again, thanks!!!
| [reply] |
|
|
For some reason I could not open a file (for writing) in /tmp, but was able to in a new sub-directory of cgi-bin...go figure.
That's strange, and could be an indication that your script is running
with privileges that are higher than e.g. the nobody user that webservers commonly use to run scripts. That'd be another reason to be incredibly careful with using form input for filenames and potentially other things. Attackers would happily exploit a security hole that allows them to create files to, for example, set up phishing sites under your domain.
Taint mode is a good idea in this case because it forces you to think about certain cases. But it's of course also not a silver bullet - thinking about what you are doing with user input is always a good idea :-)
| [reply]
[d/l] |
|
|
