Re: [Nms-cgi-devel] ffa script discussion

[Nick C. <ni...@cl...>]

On Tue, Jan 29, 2002 at 11:02:40AM +0100, Lukas Ertl wrote:
> Hi,
> 
> I just looked at and tested ffa.pl and some questions arise:
> 
> *) Should we implement some kind of link normalisation, so that
>    "http://www.bla.com" is converted to "http://www.bla.com/"?
>    This would make it easier to eliminate repeated links.

We could do that, I would rate it as low priority though.

> *) Should we implement some kind of URL checking, so that bogus URLs like
>    "http://bla" are not accepted and get caught by no_url()?

I don't think so, bla could be a genuine hostname on an intranet or
something.

> *) https:// links should be accepted (this would be just a quick hack
>    in the URL checking regex).

Agreed.

> *) We have to redesign the error checking for no_url() and no_title().
>    Currently, no_url() gets called when someone entered a bogus URL, but
>    $title is yet undefined at this point, so you get 1.) an error message
>    in the log files and 2.) after entering the URL into the "bogus URL"
>    form you get a second redirect to "bogus title".

Yes, that's bad.


Here are some other things that need to be fixed.  Most of these
apply to ffa, and to many others as well.  People looking for
things to do take note :)


1) CSS

There are places with code like 

  print qq|<a href="$foo">....|

where $foo has come from CGI inputs.  This allows people to display
arbitrary HTML and opens up cross site scripting holes.  We need to
copy the escape_html function from FormMail.pl and use it everywhere
this happens.  I've done this for guestbook and FormMail.

Have a look at http://httpd.apache.org/info/css-security/ if you'd
like to see an explanation of why this is important.

2) Updating files

Many scripts do the updates by locking the file, reading all the lines
in, truncating the file and then writing them all out again.  This
could cause corruption if the script dies or is killed half way through
the write process, and it scales badly to large files.

What the scripts that process files line by line in this way should do
is: 

  open $file.lck for write

  flock $file.lck

  open $file for input

  open $file.tmp for output

  copy the file line by line, making whatever changes are needed

  close $file.tmp

  rename $file.tmp to $file

  close $file.lck

The script must *not* unlink $file.lck when done or the
locking scheme becomes unsafe.  It stays around as a harmless
zero size file.

3) CGI.pm settings

We don't use file uploads, and we shouldn't need very large POSTs,
so CGI.pm best practice is to set something like:

  $CGI::DISABLE_UPLOADS = 1;
  $CGI::POST_MAX = 1000000;

... just after the 'use CGI' bit.

4) Input filtering

There are a few places where things that have come from CGI
inputs get into places (files, email bodies) without any filtering
at all.  At a minimum, all user input should be run through the
strip_nonprintable function from FormMail, to take out things like
NULLs and delete characters.  I've done this for FormMail only.


--
Nick