Re^2: prevent perl script running from browser

[afoken]

That looks scary.

I've already explained some of the problems with the first few lines of the code. Let me add some more notes for the remaining part.

• Perl runs in taint mode. That's actually a good idea.
• Indenting is not consistent. Yeah, it's bean counting time. But it shows that this code is not well-maintained; and it makes it unnecessarily hard to read the code.
• use CGI::Carp qw(fatalsToBrowser); - you are exposing any errors to an attacker, a really bad idea. Just let the script die, you will find any errors in the error log. The web server will deliver a "500 Internal Server Error" page. That's sufficient.
• No strict, no warnings. A lot of errors in the code are hidden this way. Have a look:

  >perl -cw -Mstrict -T 1200474.pl
  main::get_date() called too early to check prototype at 1200474.pl lin
  +e 76.
  main::get_date() called too early to check prototype at 1200474.pl lin
  +e 104.
  Global symbol "$to" requires explicit package name at 1200474.pl line 
  +25.
  Global symbol "$from" requires explicit package name at 1200474.pl lin
  +e 26.
  Global symbol "$subject" requires explicit package name at 1200474.pl 
  +line 27.
  Global symbol "$message" requires explicit package name at 1200474.pl 
  +line 28.
  Global symbol "$to" requires explicit package name at 1200474.pl line 
  +33.
  Global symbol "$from" requires explicit package name at 1200474.pl lin
  +e 34.
  Global symbol "$subject" requires explicit package name at 1200474.pl 
  +line 35.
  Global symbol "$message" requires explicit package name at 1200474.pl 
  +line 37.
  Global symbol "$gpg_path" requires explicit package name at 1200474.pl
  + line 70.
  Global symbol "$gpg_options" requires explicit package name at 1200474
  +.pl line 71.
  Global symbol "$gpg_public_key_user_id" requires explicit package name
  + at 1200474.pl line 72.
  Global symbol "$gpg_command" requires explicit package name at 1200474
  +.pl line 83.
  Global symbol "$gpg_path" requires explicit package name at 1200474.pl
  + line 83.
  Global symbol "$gpg_options" requires explicit package name at 1200474
  +.pl line 83.
  Global symbol "$gpg_command" requires explicit package name at 1200474
  +.pl line 84.
  Global symbol "$gpg_public_key_user_id" requires explicit package name
  + at 1200474.pl line 84.
  Global symbol "$gpg_command" requires explicit package name at 1200474
  +.pl line 85.
  Global symbol "$gpg_command" requires explicit package name at 1200474
  +.pl line 87.
  Global symbol "$output_file" requires explicit package name at 1200474
  +.pl line 91.
  Global symbol "$gpg_output" requires explicit package name at 1200474.
  +pl line 94.
  Global symbol "$output_file" requires explicit package name at 1200474
  +.pl line 98.
  1200474.pl had compilation errors.
  
  >
  

  (Note that I commented out use CGI::Carp qw(fatalsToBrowser); to avoid duplicated error messages.)
• Omit the (empty) prototypes for get_date() and get_order_num(), you don't need them. Just continue to ignore @_ in those two routines.
• The mail to the hardcoded $to is not encrypted at all, and contains just the user's email address (or whatever was passed via CGI parameter email).
• Sending that mail uses a pipe open to a hardcoded instance of sendmail with a hardcoded parameter. $ENV{'PATH'} from a few lines above is not used (but because an absolute path is used for sendmail, the junk in $ENV{'PATH'} does not harm).
• No error check for the pipe open to sendmail. Did you ever wonder why you got less emails than orders?
• Bareword file handle for the pipe open to sendmail. It won't hurt, but it is bad style.
• Two-argument open. Same thing. Use three argument open, and check for errors.
• TOCTOU error / race condition in generating the GPG output file: A while loop checks for the existence (-e) of something named "/home/xxxxx/public_html/cgi-bin/encPGP/" + date + a number starting at 1. A lot of CPU cycles later, /usr/bin/gpg is invoked to create that file. During that time, another instance may have generated the SAME output file. Ever wondered why some orders are lost or mixed up? To fix that:
  • Test and create the (temporary) file at the same time. The C API has flags for open to create an exclusive file (O_CREAT | O_EXCL). You don't have to know that, the libc and nice packets like File::Temp wrap that up nicely. Note that many libraries have messed up this very problem.
  • Don't place temp files in your webspace. Put them in /tmp and remove them when you are done. File::Temp can do that automatically for you.
  • Please don't tell me that you keep those files because mailing breaks so often.
  • You don't need a temp file at all, just pipe into STDIN of /usr/bin/gpg and read its STDOUT. See "Bidirectional Communication with Another Process" in perlipc, or use IPC::Run.
• /usr/bin/gpg is once again called using a two-argument, bareword handle open without error checks. Again, use the three-argument form and check for errors.
• The code reading back the output of /usr/bin/gpg has similar problems: no error check, bareword handle, two-argument open.
• The same code could use slurp mode instead of reading line by line. That's more efficient.
• The code reading the output of /usr/bin/gpg tries to read from the undefined variable $output_file. Later, an unlink is attempted using the same variable, but not checked for errors. /usr/bin/gpg actually writes to the filename in $out_file. Using strict would have caught this error. Either this is (apart from anonymizing) not the code that actually runs on the server, or nobody cares about lack of data in the mails sent out.
• Sending out the mail using the SLS bareword handle has the same problems with open as before.
• Did you notice that it uses a different set of parameters for sendmail? The first sendmail (for the MAIL bareword handle) uses only -t, this one adds -i.
• This round of sendmail can pass arbitary data to sendmail. If $email contains a newline, it can inject extra headers. If $email contains two newlines, it can also be used to generate content. There is no code to validate $email.
• The next round of sendmail using the USR bareword handle shares the same problems as the previous one: unchecked two argument open, arbitary data written into a sendmail pipe.
• Arbitary HTML injection via the $total variable. Like $email, it lacks all sanity checks, it is written out without any escaping, and so allows to send arbitary data to the browser.
• get_order_num() uses bareword handles, so if it unexpectedly returns early, the order_cnt.txt file will stay open and locked until the script exits.
• If printing the increased number back to the file fails, you end with an empty file and the next order will be order #0. The sane way is to use two files and a temp file: One file just for locking, it may have any content (including none). A second file contains the current order number. To increment the number, write the incremented number to the temp file, then - while still holding the lock - rename it to the name of the second file. Rename is atomar (except for networked filesystems), so you have either the old or the new count, but never an empty file.
• Luckily, get_order_num() is completely unused. Just remove it.
• The script takes all input from CGI parameters, and it behaves as a CGI. Unmodified, it will be hard not to use it as a CGI. Yes, it could be modified to run as a command-line tool, but that needs work.

I would disable that script NOW. Just remove it from the server, then fix the problems. There are at least three obvious injection problems that need to be addressed, and there is a race condition in encrypting the form data.

Alexander

[snowchild]

hi Alexander Thank you for a very concise run down on my garbage script. I hope you realize that for most of the comments you wrote line by line I have no idea what you are talking about LOL. As I said earlier, someone wrote this script 17 years ago and since then it has had some minor changes made by others from trying to make it work on various hosting servers. I think it would be best as you say, to remove the script ASAP and try to write a better solution with PHP. In the meantime I am stuck with it. It does the job and no I have not noticed any errors in the output or mixing up of emails being sent, but as you say it's got more holes in it than swiss cheese. I cannot disable it until I have something better to replace it with. Such is life. Thanks again

[afoken]

I hope you realize that for most of the comments you wrote line by line I have no idea what you are talking about LOL.

Well, you chould change that. Understand what happens in the script, even in its current, scary state. That's not hard. In fact, the lack of error checks actually makes it easier to follow the program flow. Ask for things that you don't understand, in the code and in my review. We are here to answer.

As I said earlier, someone wrote this script 17 years ago and since then it has had some minor changes made by others from trying to make it work on various hosting servers.

That explains the commented-out statements and the indent.

I think it would be best as you say, to remove the script ASAP and try to write a better solution

Yes.

with PHP.

No.

In the meantime I am stuck with it. It does the job and no I have not noticed any errors in the output or mixing up of emails being sent,

Well, then you are lucky not to have a lot of load on the webserver.

but as you say it's got more holes in it than swiss cheese.

At least, it has a lot of potential to be a spam relay, using your company's name. It can also be used to generate malicious web pages that seem to be hosted on your company's hosted webserver. It should be quite easy to steal cookies and other data from the user's browser.

I cannot disable it until I have something better to replace it with. Such is life. Thanks again

Well, you could. Talk to your boss. Make it urgent to get a sane replacement. It's likely much cheaper than fixing the bad reputation you could get if someone finds this script.

Alexander