The inadequacy of removing . from @INC

Perl’s @INC might find code that you don’t want. That array is list of directories that use, require, and do search to find modules and libraries. By default, the last entry is ., which represents the current working directory.

J.D. Lightsey and Todd Rinaldo from cPanel (a top-shelf Perl Foundation Core Maintenance Fund sponsor) first reported the issue as CVE-2016-1238. If someone puts a module in the current working directory, they don’t have to modify @INC to load it. However, this also means that someone else might put a module in the current working directory, and, in the case that it’s not already installed somewhere else in @INC, the application loads that one. If you’re running from a shared directory, this might affect you. You don’t even need to use the module in some cases as other modules might helpfully try to use it for you, as in a plugin or Storable’s object inflation bug.

This could be a problem; It could also not be a problem. However, Perls v5.22.3 and v5.24.1 (not yet released as I write this), modify several modules and programs in the perl source tree to remove the dot from @INC.

These patches don’t actually solve the entire issue, but they are a good start. They solve a very small corner issue. It’s similar to the parts of Mastering Perl that cover taint checking and the directories in the PATH environment variable. A small fix doesn’t magically protect us from everything else that impacts the issue. We still have to think through the problem based on everything else that is going on.

Consider the patch to my module App::Cpan. I loaded some modules conditionally:

my $log4perl_loaded = eval "require Log::Log4perl; 1";

A patch to fix this @INC problem creates a subroutine that “safely” loads a module:

sub _safe_load_module {
	my $name = shift;

	local @INC = @INC;
	pop @INC if $INC[-1] eq '.';
	eval "require $name; 1";
}

This wraps my previous calls:

_safe_load_module("Log::Log4perl");

But, there are many problems with this way of doing things. If the problem is loading modules from directories writeable by other users, why would you limit yourself to looking at a single entry in @INC and only removing it if it has a hard-coded entry in only the last position?

I disagree that this is merely a program issue. It’s a risk of using a multi-user computer with every utility, application, or programming language. It’s a consequence of the way things work in these systems. This means the system provides ways, by design, to circumvent this very limited protection.

If this is truly a problem, we should examine every entry to @INC to check permissions. Most of the entries are unlikely to be writeable by other users since they are likely owned by a superuser (or other elevated-priveleges account).

Should perl track a list of trusted users? Some of the directories might be group writeable. I maintain some systems where many people are able to affect system directories. I typically have a perl group for the people allowed to administer the perl installations. Am I going to trust all of those? I think I trust them, but what if someone changed the group to add a user that I don’t trust?

What if one of the entries is a symlink? Maybe someone futzed with the filesytem so that one of the system directories points to a directory that we shouldn’t trust?

Now, suppose we have a foolproof way to check that everything about a directory is acceptable. Between the time we do that and the time we try to load the module, does that situation change? Do we need to lock everything?

So, there’s all of that. Some filesystems make it so that we can’t trust anything in @INC.

But, let’s look at the fix a bit more. There are several statements in _safe_load_module. If I can load a perl debugger here (and there are many ways to do that), I get a chance to run arbitrary code after each statement. After you change @INC I can change it right back. But, I don’t need to change it back. I can append an entry so . is not the last one. Or, what if I had already appended to @INC in such a way that there were two entries for the current directory? Did you know that you can have a code reference in @INC?

I don’t even have to trick you into loading a module you don’t know about. If you’re using CPAN, you’re taking far more risk than loading modules from a writeable directory. How many of you actually review the code in the modules you use? I like Mojolicious, I trust it, and I’ve read a lot of the code, but I haven’t read all of it. When I upgrade, I don’t review the code changes.

What if they decide to play around with @INC? Every time I install a module from CPAN I take the slight risk that someone has tricked me into installing malicious code, the slightly greater risk of installing exploitable code, and the certainty of installing buggy code.

There are some many ways to get to the same effect that deciding to not trust one particular string in one particular position ignores the actual problem. Maybe someone feels safer, but that plays into the hands of the malicious people who know what they are doing and realize this patch won’t stop them.

The removal of . is a small, but good step to the much larger problem. We still need to consider what we allow.