Item9983: Bad practice having @INC setting with current directory in scripts

The following construct is found in all your scripts:

@INC = ( '.' , grep { $_ ne '.' } @INC );

Good security practice would involve not giving the current working directory a way to override to all of your modules. Is there a good reason for this idea?

-- DaveHayes - 09 Nov 2010

It's historical. Can you expand on why it is bad security practice? The scripts in question are cgi-bin scripts, and as such the working dir is well controlled. I struggle to think of a way to exploit this.

-- CrawfordCurrie - 21 Nov 2010

Well as I know good security practices, even if you can't think of a way to exploit this you should err on the side of safety.

As one example, a FastCGI setup may not have a well controlled working directory. I believe this working directory remains set to where you were when you started the process manager. Now I wrote a startup script that places the working directory in a good place, but if I am in the heat of solving a problem and I restart the process manager manually without my script then my working directory could be my home directory. An administrative error to be sure, but one that would have less consequence except for that @INC definition.

The issue with @INC: if someone can trick the server into writing a file (interesting examples would be ./CGI.pm or ./Foswiki.pm) then with the above @INC those modules would be picked up instead of the modules the software actually wanted.

-- DaveHayes - 21 Nov 2010

well. this is busted on my fun setup - FindBin is returning the wrong thing - see https://rt.cpan.org/Ticket/Display.html?id=63220

-- SvenDowideit - 22 Nov 2010

Where are we on this one.

Do we currently have a Release01x01 branch that fails in Sven's setup?

If this is the case the change needs to be reverted. We had code that worked fine and noone has shown a real attack vector that anyone using Foswiki as documented would run into. We cannot fix a very theoretical issue and then create a much larger problem in the process. At least the Release01x01 branch needs to be kept stable.

-- KennethLavrsen - 07 Dec 2010

Hum, I've committed this on both trunk and the release branch 4 weeks ago (11/10/10) and Sven opened a bug report on FindBin, because it's most likely his setup (he has a very convoluted setup I doubt anybody will ever have), but it could be related to userdir and suexec.

I will try to setup such a system, and I know that George's installation which also runs suexec, is working just fine with my change.

As for the attack vector, it's true it would most likely need filesystem access, but you're not sure where, which is the all point. As '.' is in the path, '.' COULD be in some user-writable path, using some mod_userdir or something.

So, action is: We need to review Sven's issue, and see if his problem comes from my code, from FindBin or from his setup, and if it can be easily fixed in any of the 3. I'll try to work on this...

-- OlivierRaginel - 07 Dec 2010

It's very important that theoretical holes be closed with the same intention for security as practical ones. The security holes that bite people the most are the ones they said "no one could ever exploit that". Making sure something can't happen is very important.

-- DaveHayes - 07 Dec 2010

Yes Dave. But I only want holes plugged by fixes that do not create real issues that cause downtime and trouble.

Most Foswiki's are Intranet based and cost combined with the risk of an internal employee attacking a very special installation of Foswiki is far smaller than the cost of doing an upgrade from 1.1.2 to 1.1.3 and find that it takes a full working day to revert things back to 1.1.2 because our plug caused trouble.

So what I am saying is that I do not want a plug checked into 1.1.3 unless we know it is working for everyone.

-- KennethLavrsen - 07 Dec 2010

This change either needs to be reverted, or at least re-written anyway, as Will pointed out, we should not be using FindBin. Gilmar introduced that, and I built on it, but FindBin's documentation recommends not using it in persistent environments. It shouldn't be that bad as we always use it for the same thing (I mean to retrieve the same path), but still.

I think the 'proper' way to do it is one Dave or maybe somebody else (George?) suggested:

• Try to guess the path from the filename
• If that fails, try to use FindBin or the like
• If that fails, fallback to '.'
• If that fails, abort. -- this should never happen, or at least it's what we used to have anyway.

I'll try to fix this asap, but it's Xmas, so... not that much free time

-- OlivierRaginel - 18 Dec 2010

Maybe we should do a revert checkin now and then let this bug item be an enhancement or normal bug to be fixed.

Right now it is a 1.1.3 release blocker not because of the original report but because of the fix already checked in.

-- KennethLavrsen - 20 Dec 2010

Agreed. If I don't manage to "fix" it tomorrow (23 Dec 2010), please feel free to revert it on the Release branch.

-- OlivierRaginel - 20 Dec 2010

It would be nice if you had a separate category that didn't block a release but was more urgent than normal.

-- DaveHayes - 20 Dec 2010

I have reverted the fix for Release branch only.

-- KennethLavrsen - 28 Feb 2011

I guess this has been fixed in Item10717, hasn't it?

-- OlivierRaginel - 14 May 2011

Only on trunk. Release branch still only uses the . in the path. So if trunk is looking stable using the FILE with conditional prepend of ., we can probably sync over the trunk code to 1.1.4 before closing this.

-- GeorgeClark - 14 May 2011

Everyone assumed the previous code that I reverted was stable. And it turned out it was not.

The trunk does not get broad exposure and we cannot be sure. Please leave the 1.1 branch as it is. It works. Nothing is broken.

And let the trunk version pass through a set of beta releases and release candidates when we start working on getting a 1.2 or 2.0 out. That is the safe way to do it.

We have everything to lose and nothing to win by merging this over to 1.1.4.

Also note that the current trunk code uses the FILE but falls back to including ".". How safe is that? Could you fool the server by manipulating FILE and then the server falls back to "." anyway?

-- KennethLavrsen - 16 May 2011

I totally agree with Kenneth. I'd rather not merge that to 1.1.4. As for the fallback, I'm afraid it's either that, or we cannot run under some fancy systems, or at least they would need manual configuration.

Maybe we could improve, but for the time being this is good, and this shall not be merged into 1.1.4.

-- OlivierRaginel - 16 May 2011

is __FILE__ subject to manipulation? According to perldata:

Special Literals
The special literals __FILE__, __LINE__, and __PACKAGE__ represent the current filename, line number, and package name at that point in your program. They may be used only as separate tokens; they will not be interpolated into strings

-- GeorgeClark - 16 May 2011

No, it isn't.

-- CrawfordCurrie - 10 Mar 2014

Commenting is disabled while not logged in