Beefy Boxes and Bandwidth Generously Provided by pair Networks
go ahead... be a heretic
 
PerlMonks  

is this script secured enough from internet attacks

by tercoz (Acolyte)
on Jun 10, 2011 at 12:11 UTC ( #909099=perlquestion: print w/ replies, xml ) Need Help??
tercoz has asked for the wisdom of the Perl Monks concerning the following question:

Hello, my n_name is tercoz, Russia. I am very very new in perl and I want to ask a question. I have written a code to secure all data which is gotten from web forms. Is this code valid? It's working but, maybe i lost something in particular, maybe something is not as secure as i planned. Thank you.

#!/usr/bin/perl -T use strict; use warnings; use CGI qw/param/; use CGI::Carp qw/fatalsToBrowser/; #__No uploads any more__ $CGI::DISABLE_UPLOADS=1; #__100kb in case of flooding__ $CGI::POST_MAX=102_400; #__in case for something to be executed__ $ENV{'PATH'} = '/bin:/usr/bin:/usr/local/bin'; #__prints header__# sub header{ print "Content-type: text/html \n\n"; print qq( <html> <head> <title>Get all keys and values</title> </head> <body bgcolor="#ABBFC9" link="#FFFF00" vlink="#FFFF00" alink="#FF0000" + text="#000000"> ) } #__prints footer__# sub footer{ print qq( </body> </html> <b><center><hr>END OF DOCUMENT<hr></b></center> ) } #__prints warnings__# sub Warning_One{ print '<span style="color:#ff0000;">KEY OR VALUE IS NOT VALID!</span +>'; exit } #__helps alot from unwanted SQL queries__ sub antiInjection{ my @badList = qw(select from where union order char drop alter desc +show set insert); chomp(my $param = shift); $param=~m/$_/i and print "<span style='color:#ff0000;'>WORD $_ IS BA +NNED FROM USE!</span>" and exit for (@badList); } ########## MAIN PROGRAM ######### my $query = new CGI; my $script_name = "secured.pl"; #__html header__ &header; #__create test dir__ my $Test_Dir = 'TEST'; unless (-d $Test_Dir){ mkdir ("$Test_Dir", 0755) || die "<b>Error 675 couldn't create <br>< +b>$Test_Dir </b><br>$!<hr>"; } #__get timestamp__ my $Time_Stamp = time; #__name final file with timestamp value__ my $Results_File = "$Time_Stamp.txt"; my $Results_Path = "$Test_Dir/$Time_Stamp.txt"; #__print date and time to browser__ my $timeToShow = localtime($Time_Stamp); print"<br><br> On <u>$timeToShow</u> we saved results to file: ($Resul +ts_File)<hr>"; #__use timestamped file to add some data__ open (RESULTS, ">>$Results_Path") || die "Error 5643: can't print to < +br><b>$Results_Path</b><br>$!<hr>"; flock (RESULTS, 2); #__obtain all fields__ my @fields = $query->param; #__list of values for each parameter__ my @vals; for my $key (@fields){ #__check keys__ chomp($key); #__warning if forbidden characters to be involved__ &Warning_One if $key=~m/[^\w.-]+/; #__it's secured and untainted now__ $key = $1 if $key=~m/([\w.-]+)/; #__to make sure the digit is digit__ $key += 0 unless $key =~ m/\D+/; &antiInjection($key); #__obtain all values__ @vals = $query->param($key); for my $value (@vals){ ######## START REGEX ############ chomp($key,$value); &Warning_One if $value=~m/[^\w.-]+/; $value=$1 if $value=~m/([\w.-]+)/; $value+=0 unless $value=~m/\D+/; &antiInjection($value); ######## END REGEX ############## ######## PRINT TO FILE ########## print RESULTS "Key ($key) Value ($value)\n"; ######## PRINT TO THE BROWSER ########## print"<b>Key ($key) value ($value)<br>"; } } ################### ### CLOSE FILE ################### flock (RESULTS, 8); close (RESULTS); chmod (0666, "$Results_Path") || die "Error 5641: can't chmod to <br>< +b>$Results_File)</b><br>$!<hr>"; &footer;

Comment on is this script secured enough from internet attacks
Download Code
Re: is this script secured enough from internet attacks
by moritz (Cardinal) on Jun 10, 2011 at 12:23 UTC
    The antiInjection sub is bullshit. The real way to prevent SQL injections is to use prepared statements and placeholders, instead of disallowing use of some SQL keywords in data.

    The checks for "forbidden characters" in the params depends on what you want to do with them eventually, so there's no way for us to assess if it's secure for your use case.

      Thank you, I shall remove that sub. As I am new here I don't know how to give points, I want to thank you and other people for helping me(points), which way can i do so7

        Prepared statements and placeholders don't always work as expected (for example FreeTDS has problems there).

        unpack could cover all injection attempts:

        'INSERT INTO foo(bar) VALUES(0x'.unpack('H*',$value).')'

Re: is this script secured enough from internet attacks
by Anonymous Monk on Jun 10, 2011 at 12:49 UTC

    Your use of flock is classic race condition, you're not supposed to unlock, simply close the filehandle. See Order of flock and open, File Locking Tricks and Traps

    A function called Warning_One should probably not call exit :) thats like a warning shot to the brain :) oops :)

    For maintainability, small style suggestion, instead of a comment ########## MAIN PROGRAM #########, make it a function called Main, see template at (tye)Re: Stupid question (and one discussion of that template Re^2: RFC: Creating unicursal stars)

    In the same spirit (of reusability), lexical filehandles are prefered to the global ones (like RESULTS). See the book Modern Perl Released as Free ePub for many such suggestions, it is a loose description of how experienced and effective Perl 5 programmers work....You can learn this too.

      i shall review "flock" part of my script. I will also remove exit. And will be careful with comments. Thank you very much for your tips.
Re: is this script secured enough from internet attacks
by afoken (Parson) on Jun 10, 2011 at 12:58 UTC

    In addition to what moritz said:

    • Perl 4 is dead. An ampersand in front of the function name does something very different in Perl 5 than it did in Perl 4, and usually you do not want that. Get rid of the ampersands.
    • Your code lacks a lot of error checks.
    • Why all that line noise in the comments? It makes the code only harder to read.
    • "close file" before/after a close(HANDLE) is an excellent example for a stupid, useless comment. And there are more examples of this in the code. Comment what is not obvious to the reader, don't repeat the code in english in comments.
    • What's the purpose of the chmod after closing the file? Neither open nor close should mess with the file's mode.
    • use CGI::Carp qw/fatalsToBrowser/; exposes information about your script and your environment to an attacker. Remove that from production code.
    • die "Error 5643: can't print to <br><b>$Results_Path</b><br>$!<hr>"; should not contain HTML.
    • The hardcoded error number in there is just nonsense, drop it.
    • The same applies to the other error messages.
    • Warning_One is a good example for a stupid function name. Does it warn you of a big cat-killing digit 1? Nope! So, name the function according to what it does.
    • Are you aware that Warning_One exits your script before the emitted HTML is complete? It does not even close or unlock the working file.
    • In the error message, you expose the name of the file written to an attacker.
    • The emitted HTML lacks a charset declaration.
    • The emitted HTML does not end after </html>
    • The emitted HTML uses obsolete constructs like the attributes in <body>, inline CSS, the B element. Use an external stylesheet and semantic HTML.
    • You mix code and HTML. Consider using a template instead.
    • You don't use the three-argument form of open.
    • You interpolate uselessly, e.g. in chmod (0666, "$Results_Path")
    • There are more problems with your code, but this list is already too long.

    Alexander

    --
    Today I will gladly share my knowledge and experience, for there are no sweeter words than "I told you so". ;-)
      Perl 4 is dead. An ampersand in front of the function name does something very different in Perl 5 than it did in Perl 4, and usually you do not want that. Get rid of the ampersands.

      Could you explain you comment about ampersands?

      I have never coded in perl 4, but have used perl5 since 1997 on AIX (perl5.00404).

      I distinguish between the subroutines that I have written and those that are part of a "use Module" from core or CPAN.

      For example ( without all the code needed to actually work ):

      use BerkeleyDB; # CPAN module use Common; # My module my $cursor = $db->db_cursor(); # BerkeleyDB function my $var = &BD_Read($FH,$db,\$key,\$data); # my subroutine BD_Read

      I use this to help me distinguish between the source of the code.

      Is there something wrong with my personal preference?

      "Well done is better than well said." - Benjamin Franklin

        Is there something wrong with my personal preference?

        Its like wearing a tutu when you're not a ballerina ? :)

        Is there something wrong with my personal preference?

        Yes. You said you would add a "&" to $db->db_cursor() if it was your own module, but it's impossible to do so.

        flexvault,
        Read perlsub.
        NAME(LIST); # & is optional with parentheses. NAME LIST; # Parentheses optional if predeclared/imported. &NAME(LIST); # Circumvent prototypes. &NAME; # Makes current @_ visible to called subroutine.

        Unless I am creating a reference to a sub (my $subref = \&somesub;), I almost never even think about using & with a sub name purely out of habit. The rare exception is when I am using goto &somesub; which @_ gets inherited.

        Cheers - L~R

        My experience is that you will mostly find rather strong reactions against using & when calling subroutines. However, the only problem with doing so beside over-zealous style-related reactions is that doing so also disables function prototypes. But, if you use function prototypes, you are also likely to get over-zealous reactions against doing such. So the real problems with using & are minimal or none.

        On the other side, what are the arguments in favor of using & ? There is the style argument that you gave. The & provides a visual cue that the function being invoked is certainly not a Perl built-in (it can't be). You can also make the style choice of using & on functions local to the current package and then not using & on functions imported from other modules. It is funny that people who love Perl, including the ugly sigils it pollutes the code with seem to hate the red-headed step-child Perl sigil of &, despite it offering the same style / visual benefits.

        But is there a benefit to using & other than style? Actually, there is.

        #!/usr/bin/perl -w use strict; sub dump { require Data::Dumper; my $d= Data::Dumper->new( \@_ ) # The defaults are "all wrong": ->Indent(1)->Useqq(1)->Terse(1)->Sortkeys(1); return $d->Dump(); } print &dump( @ARGV );

        Since I didn't use a function prototype, surely I can just drop that "ugly" & and nothing changes.

        Note that the above problem doesn't happen for:

        use A::Module qw< dump >; print dump( @ARGV );

        actually reinforcing the above style choice of only using & on subs local to (defined in, not imported to) the current package.

        I solve that problem by capitalizing all of my subroutine names. Of course, that just leads to me learning that there are quite a few people with rather violent style-based reactions against the dreaded and ugly CamelCase. But at least I get to pick which group grumbles at my code. ;)

        - tye        

      The emitted HTML uses obsolete constructs like the attributes in <body>, inline CSS, the B element. Use an external stylesheet and semantic HTML.

      While I agree that you've stated the current trend, the existing standards are HTML 4.01 and CSS 2.1 which permit all of the above. Heck, the only thing that's even deprecated is the use of attributes in the body tag.

      You're just flat wrong about inline CSS and B. The proposed HTML 5 standard includes both the style attribute and B element.

      I started to work with another website and I shall look at those remarks you gave me and apply your tips to the new project, thank you
Re: is this script secured enough from internet attacks
by ww (Bishop) on Jun 10, 2011 at 13:00 UTC
    If not quite insecure in and of itself, use CGI::Carp qw/fatalsToBrowser/; in production code is potentially helpful to an attacker.

    In production use, you don't need to give away the information about why the iteration failed. Log it, but don't tell the attacker something that may provide insight into your security efforts.

      indeed, I saw many examples when I could get fields of database, because they were shown in CGI:fatalsToBrowser message, got to be careful next time
Re: is this script secured enough from internet attacks
by MidLifeXis (Prior) on Jun 10, 2011 at 13:09 UTC

    Kudos on using taint, strict, warnings, and so on. I also like that you are approaching security levels as a relative value, not an absolute one.

    ... here comes the "but" ...

    Security is not achieved by one single component, but by a sum of the component parts. The system should be evaluated as a whole. Also, without knowing what this is a part of, it is very difficult to assess the level of security. If I were to rate this application on just its own merits, I would give it below average marks, but not the worst I have seen. Not something I would allow on my systems, but better than Matt's Script Archive.

    I have a few specific (not complete) critiques. Do not take this as a complete list.

    • I am not convinced that your untainting code does what it is supposed to without knowing your data requirements. The cost of the effort of security needs to be compared against the value of the data, loss of data1, loss of reputation, and so on.
    • fatalsToBrowser should not be set on a public-facing application. The errors should be logged and reported to the production maintenance group, and the user should get a generic "Oops" message indicating that an error occurred and what they need to do (possibly nothing). I understand why you did it (so die would send stuff to the user directly), but errors that are outside of what you test for will dump information to an attacker that should not be given.
    • You don't check the status of your flock calls, so you still have a race condition. Since the resolution on your filename is 1 second, you only need two clients to connect at the same time with sufficiently large parameter lists to be able to see this. In production, this is not a matter of if, but when this will happen.
    • Your "antiInjection" routine appears to be rejecting SQL keywords. Are you planning on hooking this up to a database? There are a plethora of issues when hooking up to a database.
    • Not really security, but makes me ask if appropriate attention was given elsewhere (including security): your HTML will not validate (nesting on the END OF DOCUMENT line, stuff outside of html container, etc.
    • Additionally, by using die to send error messages directly to your browser, you are bypassing the output from footer. There are ways to use die, but it would involve using eval in tandem with it in a try/catch type construct.
    • Why chmod 0666 the output file? This relates to my comment about the entire system. Can you trust that the data is the same as when your application saved it? A different approach could modify the file after your application was finished with it.
    • Predictable file names. Unless you have a reason to have predictable file names, don't use them from your web server. Bad Guy (the evil brother of Good Guy, not to be confused with the homebound Family Guy), given an alternate means of access to the storage area, can create a denial of service attack by filling your file system with files matching the name pattern for the foreseeable future, set them to mode 000, and leave them there. The clients will not be able to open the file.

    1 - See The theft of business innovation: an ACM-BCS roundtable on threats to global competitiveness for a view of how "low value" targets are now being attacked.

    --MidLifeXis

Re: is this script secured enough from internet attacks
by bluescreen (Friar) on Jun 10, 2011 at 13:23 UTC

    All being said here is really good. One extra tip, usually you don't want to make filters for things you don't want like: antiInjection, antiXSS, antiXXXXX, since attackers usually find new ways to attack your site.

    So my recommendation is to take the inverse approach, that means check that each field has what you expect it to have. If is a numeric field validate that only numbers comes in it, if is an alphanumeric id validate it only contains letters and numbers, and so forth. This will give you much better specially when same filters apply to all fields you end up making them less strict to allow different variation work

    Also even though your application filters SQL inejection the attacker can still take your site down if the query is heavy, so make sure you limit your slow queries and use the proper caching

      Hear, hear! Whitelisting is far more secure than blacklisting ... are you positive that you know all the bad strings?

      Everything you said is valuable, and yes I shudder when I think about those various ways to ruin my website, and the greatest fear is 100000 queries of my cgi per second, cgi always opens and close database, which is time consuming, if there are too many queries, server may be out of service, I don't really know how does it work but heard of it alot
Re: is this script secured enough from internet attacks
by Anonymous Monk on Jun 13, 2011 at 12:44 UTC
    You might do better to look at the various "frameworks" that are available in Perl rather than "coding directly to CGI yourself." Many of these frameworks ... and some of them are very small and simple ... are already built to defend against the most common mistakes. And they're a lot faster to build with, too.
      What's the content of doing so? And it needs time to learn those engines which I don't have, but I shall look onto catalyst someday, thank you

Log In?
Username:
Password:

What's my password?
Create A New User
Node Status?
node history
Node Type: perlquestion [id://909099]
Approved by moritz
Front-paged by MidLifeXis
help
Chatterbox?
and the web crawler heard nothing...

How do I use this? | Other CB clients
Other Users?
Others browsing the Monastery: (8)
As of 2014-07-30 06:50 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    My favorite superfluous repetitious redundant duplicative phrase is:









    Results (229 votes), past polls