On November 3, 2002 at 01:53, Gunnar Hjalmarsson wrote:
Have you possibly checked out my rewritten script for invoking MHonArc?
http://www.mhonarc.org/archive/html/mhonarc-users/2002-10/msg00054.html
Some initial comments about the script (which you are free to
ignore :-):
* The initial BEGIN block is probably not needed. Fatal messages
should be sent a log file to avoid any potential sensitive
information showing up in someone's browser. Error messages the
user see should not be some Perl error message, IMHO.
* Since it is known that many people use scripts as-is without
changing things, I would set the $adminpw variable to a value
that is not supported. I.e. If the script gets a password equal
to the default value provided in the source distribution, it
does not allow access.
* It would be nice to have passwords in a separate file, but then it
would require the user to know how to deny access to those files
via HTTP. You may want document that if a user is able to set
up HTTP authentication via the server, then use that. Therefore,
you want to make sure your script supports an empty password,
or have the ability to run in no password mode.
* I'd sent $pophost to something like "example.com".
* The updatembox() function does not protect against concurrent
access to $mbox.
* The following statements appear to open up a vulnerability:
if (!$ENV{'HTTP_USER_AGENT'} or $ENV{'HTTP_USER_AGENT'} =~ /^libwww-perl/) {
exit (autoupdate()); # if not invoked from a browser
}
What prevents anyone from not causing the autoupdate() routine to
be invoked? It appears no password is checked before this statement
is invoked.
This appears to allow someone to do a DoS against the the popserver
($pophost) or allow arbitrary messages to be posted to the archive.
FYI, User-Agent strings can be spoofed, so be cautious when testing
it for a particular agent type.
* The updatembox() could be written to prevent a large amount of
memory getting used. At a mimimum, store the passed in message
data into a single scalar and a pass a reference to it updatembox().
For example:
my $newmail = '';
local $_;
while (<STDIN>) {
$newmail .= $_;
}
updatembox(\$newmail);
Or better,
updatembox(\*STDIN);
And, updatembox should be written as follows:
sub updatembox {
## XXX: Some kind of file locking should be done here!!
local(*FILE);
open (FILE, ">>$mbox") or die "Couldn't open $mbox\n$!"
my $data = shift;
if (ref($data) eq 'SCALAR') {
# Argument is a reference to scalar string
print FILE $$data;
print FILE "\n" unless $$data =~ /\n\Z/;
} elsif (ref($data) eq 'ARRAY') {
# Argument is a reference to array
local $_
foreach (@$data) {
print FILE $_;
}
} else {
# Treat argument as a filehandle
my $line;
while (defined($line = <$data>)) {
print FILE $line;
}
print FILE "\n" unless $line =~ /\n\Z/;
}
close(FILE);
}
(Routine NOT tested!)
This version supports giving updatembox() a reference to a scalar
string, an array, or a filehandle. This version should support
the existing pop retrieval code with the slight change of passing a
reference to the array to updatembox(). Note, an array containing
each line of a message takes up more memory than a single scalar
containing the entire message text.
* I do not think the crypt password option provides any benefits.
If someone sniffs someones cookie, they can just echo it back
and be authenticated without knowing the real password.
I guess it does provide some protection if the source of the
CGI program was accessed and the password cannot be directly
seen in $adminpw since it is crypted.
* In the shell() routine, you may want to use shellwords.pl. It's
an old library that is still included with perl. I'm not sure if
anyone has bothered to make a module out of it.
I'm asking because if you would find the quality to be acceptable, I
think you should consider linking to mhastart.pl or including it in the
MHonArc distribution.
I do not have a problem with including it in the contrib/ directory
of the distribution. Of course, keep a version available from
your website so you can update independently from MHonArc releases.
I can list a URL in the contrib/ README to tell people were the
latest version is available.
P.S. You didn't forget about making the maxwidth option also affect
fixed paragraphs in flowed messages, did you? ;-)
I just checked it in. It took some time as I discovered a
problem with html_length() being invoked in a substitution portion
of a s/// operation. A subtle behavior issue dealing with
backreferences. It would be nice if Perl supported localizing
backreference variables.
--ewh
---------------------------------------------------------------------
To sign-off this list, send email to majordomo(_at_)mhonarc(_dot_)org with the
message text UNSUBSCRIBE MHONARC-DEV
