Hot answers tagged

30

I will go through your code line by line and give feedback. We will skip the general advice on don't use CGI as it's actually suited for what you are trying to do here. I wrote this answer in two stages, so some of the things here might have been said by other people. I'm attempting to provide full feedback, and I'm not copying from others. If several of us ...


11

#!/usr/bin/perl -T use strict; use warnings; use CGI qw/ :all /; my $basepath = '/some/path/to'; my $f = param('f') || ''; # No interpolation, no double quotes. die if $f eq ''; # Don't use && for flow control. my $path = "$basepath/$f.pdf"; open my $PATH, '<', $path or die; # Lexical ...


7

Your param('f') is user-provided and may therefore open up security concerns. In particular, it may be used to navigate to $basepath/../../some-secret-file.pdf. The impact is reduced because you can only read files ending in .pdf, but it fundamentally still exists. As a mitigation, you should limit the allowed file names and match for those. I.e. explicitly ...


5

Since your script has no content to return, a status code of 204 No Content would be more desirable than 200 Success. For that, you should echo "Status: 204 No Content" (RFC 3875 Sec 6.3.3). Also consider returning using status code 405 Method Not Allowed for anything other than a POST request. $TMPOUT is a misnomer. The file is not temporary at all — $...


5

Standard practice is to scope variables using my. Since Perl 5.10, the preferred way to write this is to use the logical defined-or operator to provide a default value when undef is encountered. my $param_page = int(param('page') // 1);


3

It's a couple of decades (!) since I did any CGI, but I'll cast a quick eye over this. I hope other reviewers will pitch in and fill the gaps (or correct my errors). It's an interesting choice to use the same TITLE for success and failure. It might be better to write a small function: header() { cat <<EOT Content-type: text/html; charset=us-ascii &...


3

One big flaw with this that hasn't been mentioned so far is that the way your script is written, it requires giving the web-server uid NOPASSWD sudo access to chpasswd. This means that if there happens to be any other exploitable code on your web server that allows a script-kiddie to run arbitrary commands, they get unrestricted use of sudo chpasswd. ...


3

I haven't gone through the logic / design of the whole thing, just a few local things I saw. Another answer that looked at the overall design might find different things to say. I guess you're running this script as root? I thought you were running it as the user that was changing their password. Since you're already root, you should just use builtin ...


3

Your script is doing a number of unnecessary file copies and scans. I tried to streamline the process a chunk, and came up with the following to replace the line-stripping. Your code does: TMPOUT=hello cat >$TMPOUT # Get the line count LINES=$(wc -l $TMPOUT | cut -d ' ' -f 1) # Remove the first four lines tail -$((LINES - 4)) $TMPOUT >$TMPOUT.1 # ...


2

I don't really know what moses or RPC2 are, and so I've not tested your code, or changed it too much. However, there are still some things you can do to make your code much easier to understand and maintain. You want to use a lot more functions, and I would use a couple of classes. There are two main classes that I'd make Moses and Translater. The former ...


2

A few things I've noticed from a quick skim: The assert is kinda useless. write_h and write_b?? Documentation isn't an excuse for giving your functions useless names. Name them write_header and write_body or something. Globals? Just no. Especially when they're defined AFTER the functions. Took me like 5 minutes to find them. You've copied if os.getloadavg()[...


2

The only real problem I see is that if the browser is really "last century" it probably won't support a long enough query string to contain the full image. So your returned image might be incomplete. Another thing is that this would waste a lot of bandwidth. I see two options To suppress those error messages you could just handle those URLs like you and ...


2

Safety Is this code secure from parameter injection attacks? I think it's safe: Is any part of the user input executed (for example in eval, exec, or other)? -> no Is any part of the user input passed to another command that may execute it? -> no. Although parts of the input are passed to echo, sed, python, they don't execute any part of that. Is any ...


1

Use a raw header, to combine "inline" with a filename: print $q->header( -type => "application/pdf", -target => $file, '-Content-Disposition' => "inline; filename=\"$file\"", );


Only top voted, non community-wiki answers of a minimum length are eligible