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

Hello, recently I recieved a lot of mocking for my 'unsafe' mailform combined with some poor CGI script. Now I am doing some experiments and excercises figuring out what is secure content, and the so-called 'injections'. I`ve put a very basic module free code performing very simple tasks so I need some revisions, and if it`s the right way. 

#!/usr/bin/perl -wT
use strict;
use utf8;
sub pass_env {
    my (@pass1) = split("&", $_[0]);
    my ($pass2) = join("=", @pass1);
    my (%pass3) = split("=", $pass2);
    return %pass3;
}
sub check_mail {
        #use Email::Valid instead
    if ( (my @valid = split("@", $_[0]) )== 2 ) {
        #continue with regex later...
                return 1;
    }
    else {
        return 0;
    }
}
sub check_vars {
        #use htmlspecialchars() based check
    foreach (@_) {
        if  ( /[><;():]+/ ) {
            return 0;
        }
    }
    return 1;
}
##
my $str = "sub=hello&mail=sometext&text=beeee mooo "; # $ENV{'QUERY_ST
+RING} simulation
my %hash = &pass_env($str);


if ( &check_vars($hash{'sub'}, $hash{'mail'}, $hash{'text'})) { 
    if ( &check_mail($hash{'mail'}) ) {
    print $hash{'sub'},"\n";
    print $hash{'mail'}, "\n";
    print $hash{'text'}, "\n";
    }
    else {
        print "Invalid email.\n";
    }
}
else {
    print "Please, fill all forms.\n";
}

[download]

so I need some opinions good or bad, no matter.

Replies are listed 'Best First'.
Re: Is that a decent concept?
by Corion (Patriarch) on Mar 31, 2012 at 14:41 UTC

    Your follow-up is commendable. It is still not great because of two things. One is minor - you are hand-parsing the parameters instead of using (say) CGI or CGI::Lite. The other is much more critical. You are still trying to eliminate "unwanted" input instead of letting through only what you want to attempt. I recommend using a simplicistic matcher for email addresses and text, maybe even as simplicistic as /^[-_\w]+\@([-_\w]+\.)+\w+$/ (for the email). This will reject some valid email addresses, but when piping stuff to sendmail (or, as recommended, MIME::Lite), that is preferrable to letting your mail server become blacklisted because of spamming.

    This approach will eliminate least one class of problematic input, bad whitespace in the subject, mail body and recipient.

    Again, the rule is to be very specific in what you let through, instead of only eliminating what you know is bad.

[reply]
[d/l]
[select]

      brian_d_foy mentions in Mastering Perl that your suggestion fits the Prussian Stance, whereas the OP's method represents the American Stance. Apparently those terms originated in a talk by Mark Jason Dominus, though I can't seem to find the original talk anywhere online. It may be mentioned in HOP somewhere as well, but I can't recall where to find it.

      In short:

      • Prussian Stance = Allow-listing = Specifically allowing those characters we know to be safe.
      • American Stance = Block-listing = Specifically disallowing those characters we know to be unsafe.

      The disadvantage (as I see it) to blocklisting is that we must know all possible "bad" characters, whereas with the allowlist, we must only know those good characters we care about. The penalty for omitting an item from a block list is possible security breech. The penalty for omitting an item from a allowlist is potential user frustration, but not a security breech.

      The disadvantage (as I see it) to allowlisting is that, particularly in an era of Unicode, the list of acceptable characters either becomes enormous (in which case the potential for accidentally allowing something we shouldn't increases), or becomes limiting in ways that may impede a pleasant user experience.


      Dave

[reply]
[reply]

      Thank you. As for the MIME::Lite, I`ll use it for sure as well Email::Valid instead of my check. That was just a scratch pad for the very basic concepts of what I have to stress to learn and how to make the algorithm for the safe content mail form/client. I`ll look more into it because I am still learning.

[reply]
Re: Is that a decent concept?
by ww (Archbishop) on Mar 31, 2012 at 16:07 UTC
    The statement "I recieved a lot of mocking" is false (as well as misspelled).

    You received a lot of good advice at no charge.... but seem to have misunderstood or ignored most of it.

    If this is merely an exercise, kudos for the improvements -- especially your decision to use a module to test addresses Note 1 -- but please don't take offense at explanations of your exercise's deficiencies. You're in the position of a small child poking a knife into an electric outlet, if you actually intend to use this "out in the wild." Rolling your own, even with the copious good advice you've received, is NOT an exercise for the beginner.

    Please note that Email::Valid concerns itself chiefly with the validity of the form of the address -- testing it against RFC822 and, optionally, for a valid TLD (top level domain); for the presence of spaces and correcting them, if you specify that with fudge(); and for the existence of an MX or A record. But it's not checking subject, date or body for nasties, nor making any attempt to ensure that the mail actually originates (I'm sure there's a better term for that) at your server.

    Just reading the code and pod for NMSFormMail (or, better yet, TFMail -- the .zip is available at http://nms-cgi.sourceforge.net/scripts.shtml) will illustrate some of the other aspects of making user input (somewhat) safe.

    Note 1 Email::Valid's author assigns a version number of less than one -- typically, a signal that a module is a work-in-progress rather than something believed complete and stable.

[reply]
[d/l]
[select]

      Actually I used 'mocking' in a wrong content, for which I beg forgiveness. As a not a native english speaker I used mocked as the meaning of disagreement but since I appears to be some synonim of joking I take my word back, I am grateful for the advices, because they`ve made me learn some new stuff and reconsider the code before writing.

[reply]
Re: Is that a decent concept?
by marto (Cardinal) on Mar 31, 2012 at 12:38 UTC
[reply]

Back to Seekers of Perl Wisdom