We Perl programmers are a fairly snooty lot when it comes to security. With our garbage collection, deep and excellent taint support, DBI placeholders and CGI libraries, it's rare to see exploits in Perl applications.
So when something like the 2009 CWE/SANS Top 25 Most Dangerous Programming Errors report comes out, it's easy to just skip over yet another document decrying buffer overflows, SQL injects and other things that Perl programs don't suffer from.
I've had this report in my "research to look at later" bookmark list since I saw the announcement, but with Padre suffering it's first security exploit I figured I should probably look at it now.
The Padre exploit, for the record, is the same exploit I discovered in Komodo during PPI development, and that has remained in it (and probably other editors) for a few years now.
The exploit relies on the fact that in Perl, you can execute arbitrary code at compile-time. This makes "perl -c script.pl" just as dangerous as "perl script.pl". All an attacker needs to do is wrap code in a BEGIN { } block to convert a run-time attack to a compile-time attack.
# You really don't want to perl -c this
BEGIN { system('rm -rf ~') }
In fact, I've been tempted from time to time to write an Acme::ExploitPAUSE module with something similar to this to prove it's safe.
$VERSION = do { system('rm -rf ~'); return '0.01'; };
Lingering doubts about whether or not this would actually blow up PAUSE keeps me from doing so.
As a result of the compile-time = run-time feature of Perl, any editor which tries to "lint" your Perl code AUTOMATICALLY at load-time by compiling it is in effect executing arbitrary code.
If you also associate a .pl extension with your editor, and then combine that with URL redirection, you can end up with a complete pipeline from URL -> arbitrary-code-execution.
Suffice it to say, we've squashed that bug. Padre requires you to take a specific user action to enable compiler-based syntax checking. This is inconvenient, but less inconvenient than losing your home directory. (We may end up implementing some kind of per-project thing later to make things a bit less tedious though)
Wondering what OTHER exploits Padre might be exposed I had a quick read through the CWE Top 25 report.
Ignoring teh buffer overflow and param-checking sort of thing, there ARE a couple of exploits that are probably quite common in Perl and CPAN.
The standout exploit that vast swathes of CPAN code is probably vulnerable to is CWE-732: Insecure Permission Assignment for Critical Resource.
The short description of this bug is that, when reading a critical file (config files, modules, etc) you should always check the file permissions to be sure that nobody could possibly have modified it.
Which is to say, you should explicitly check that files could not possibly have been written to by users other than those you trust. The cheap version of this is to make sure all loaded files have 644 or better permissions. Anything with world-write permissions cannot be read, neither can group-write files if you don't trust all the members of the group.
When can this impact Perl code?
Lots of Perl program use temp files or temp directories to store data and just trust that nobody else has write permissions. Lots of Perl programs read config files without checking permissions.
And LOTS of Perl programs use Module::Pluggable, or have Plugin support of some kind (including Padre), which is an exploit waiting to happen if we aren't checking the permissions of the modules that we load.
I'm not sure if we already have something like this, but it would be interesting to see things like Module::Pluggable coming with CWE-732 support by default (presumably with an option to disable it).
The same could perhaps be done with Perl's "use" and "require" functions, which could validate the providence of files before they are loaded into memory.
I'm pondering if tainting support might be able to do this intrinsically, so that we could have certainty that programs were safe from attack.
when reading a critical file (config files, modules, etc) you should always check the file permissions to be sure that nobody could possibly have modified it.
How does that help? If an attacker has permission to change the contents of a config file then they may well have permission to chmod it back to 644 afterwards, surely?
Further, checking the permissions with stat() and then reading the file introduces a race condition, and so does reading it first and then statting.
Some programs like OpenSSH or Apache do check the permissions of private key files,
Now there are many security holes associated with
Re:Checking permissions is silly
Alias on 2009-01-20T14:24:36
> How does that help? If an attacker has permission to change the contents of a config file then they may well have permission to chmod it back to 644 afterwards, surely?
No, they can't.
(Unless the attacker has root in which case the security of your program is irrelevant anyway)
adam@svn:~$ sudo vi tmp.pl
adam@svn:~$ sudo chmod 666 tmp.pl
adam@svn:~$ vi tmp.pl
adam@svn:~$ chmod 644 tmp.pl
chmod: changing permissions of `tmp.pl': Operation not permitted> Further, checking the permissions with stat() and then reading the file introduces a race condition
No, it doesn't. (mostly)
If the file is secure and not world-writable at the time you stat, and then you read it, the only way it could be vulnerable is if you (or you in another program, or root) changed the permissions to world-writable between the time you did the stat and when you read it.
There may be other race conditions of course, but assuming none of the programs you might be running would delete the file and create it again fresh, and none will intentionally make the file insecure, it's reasonable to expect the file to be secure.
/tmp is, of course, a whole different story. Re:Checking permissions is silly
Ed Avis on 2009-01-20T16:52:12
No, they can't.
(Unless the attacker has root in which case the security of your program is irrelevant anyway)Well, quite. To change files in
/etc you need to be root, and if you are root then you can make it appear that the files have not been altered. There are conceivable cases where by some freak event a config file was set up as o+w but the directory was not writable, but I have a hard time believing it's the application's job to check for that, any more than I think applications should be constantly scanning and rescanning allocated memory to check for bit flips caused by cosmic rays. If the file is secure and not world-writable at the time you stat, and then you read it, the only way it could be vulnerable is if you (or you in another program, or root) changed the permissions to world-writable between the time you did the stat and when you read it.
Well exactly! And isn't that just the kind of thing that the 'guideline' is checking for?
Similarly: if the file is secure and not world-writable at the time the program is installed, and then you read it, the only way it could be vulnerable is if you (or you in another program, or root) changed the permissions and/or file contents between the time you installed the program and when you ran it. So why is that case any more worth checking for? The difference is only one of timespan, not principle.
Re:Checking permissions is silly
Alias on 2009-01-20T17:35:32
> So why is that case any more worth checking for?
We don't always only read files that were created at the time a program was installed?
Plugins appear afterwards, config files get backed up and restored. Who's to say the program hasn't been upgraded since you last ran?
There's a range of cases where this problem could appear from and not all of them would be a problem during the race window. But I agree some might, so it probably does makes sense to check both initially and then on the opened file handle.
Re:Checking permissions is silly
Arador on 2009-01-20T16:19:53
> Further, checking the permissions with stat() and then reading the file introduces a race condition, and so does reading it first and then statting.You can open a file, and then stat the file handle...
Re:Checking permissions is silly
Ed Avis on 2009-01-20T16:55:47
You can open a file, and then stat the file handle...
Doesn't help. The permissions can be changed after you stat and while you still have the file open.
Re:Checking permissions is silly
spinclad on 2009-01-20T21:19:06
It closes, not the case of its permissions changing, but of statting and reading different files, because the file (in a writable directory, like/tmp) got renamed in the interim. A tighter permission check, reasonably cheap: fstat, read, fstat again and check nothing but access time has changed.
Well, the thing that always irked me, is how templates can be "compiled" (= converted to perl code which, when executed, produces the desired output from the template) and stored to disk, for simply reloading and running later. The compiled templates must be writable by the program that compiles and uses them, in order to be able to recompile them on the fly, if necessary; and which may running under the same under-privileged user as all other users are using; for example, when running as a CGI script.
That way, all those other users can modify the code that will simply be executed by the other program.
And that's just one scenario, I'm sure you can come up with plenty of others.
Re:Template Toolkit
ChrisDolan on 2009-01-21T01:41:51
Very good point.
I just skip the write-to-disk step. If your process is long-lived enough, then the compile time of the template is trivial. The biggest cost in that case is if you have too many templates to comfortably fit in RAM.
If I understand Template::Provider correctly, then COMPILE_DIR defaults to disabled anyway, so this isn't a problem unless you turn it on.
Re:Template Toolkit
Alias on 2009-01-21T17:09:07
I've found it quite useful to just compile every possible loadable template in advance
:) http://search.cpan.org/user/adamk/Template-Provider-Preload-0.05/lib/Template/Provi
d er/Preload.pm
