-
Notifications
You must be signed in to change notification settings - Fork 567
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Flaws in Perl code due to unsafe module load path #15263
Comments
From @lightseyHi there, I need some guidance from the Perl and Debian security teams on how to Todd raised this issue on the p5p list in 2012 when he was updating I started putting together a conference talk to summarize all of the The major problem with this behavior is that it unexpectedly puts a For example, if a user on a shared system creates the file The most severe example I've found on Debian is that apt-get will load mkdir -p /tmp/Log A very simplistic analysis of my personal Debian laptop shows that over grep '^#!/usr/bin/perl' /usr/*bin/* | cut -d ':' -f 1 | xargs -n1 Or to test an individual script you can do something like this: strace -f /usr/bin/perldoc perldoc 2>&1 | grep 'stat("\..*\.pm"' This kind of analysis definitely underestimates the number of affected So...with all that explanation said... My question is, which codebase 1) The Perl interpreter for placing the '.' in @INC. 2) Any module that fails to remove the '.' from @INC before attempting 3) Any script that loads any modules that end up exhibiting this 4) End users must avoid running Perl scripts in directories that other At cPanel we started off with approach #3, then ended up leaning I haven't contacted Debian's Perl team about this problem. Please CC |
From @fweimer* John Lightsey:
'.' in @INC is certainly handy for development, so that you can access Maybe it could make sense to extract the full path to the executable This would still put at risk applications which write temporary |
From @demerphqMaybe we should handle this with a command line operation. So perl -Q script.pl would not add ".". People that want to write scripts that dont use the cwd could add the I hate to make security opt-in tho. Yves On 5 April 2016 at 05:23, John Lightsey <perl5-security-report@perl.org> wrote:
-- |
The RT System itself - Status changed from 'new' to 'open' |
From carnil@debian.orgHi, On Tue, Apr 05, 2016 at 08:59:46AM +0200, Florian Weimer wrote:
One addition/context: Just recently #588017 got activity: Regards, |
From @iabynOn Mon, Apr 04, 2016 at 08:23:27PM -0700, John Lightsey wrote:
My personal feeling is that perl should *not* include '.' in @INC by I think we are in the same situation where UNIXy OSes gradually stopped In the same way, if we change the default, the good thing is that it's The converse is not true - its not easy to spot that a script isn't Very few people writing a boring everyday admin script are going to be What I would like to see is: 1) Todd's changes to cpan/ modules to make them dot-agnostic get pushed as -- |
From @rgarciaOn 5 April 2016 at 10:07, Dave Mitchell <davem@iabyn.com> wrote:
I'd like to add that -T also removes . from @INC, so anything that would (I was under the impression that running perl as root was also removing |
From @timbunceOn Tue, Apr 05, 2016 at 09:07:24AM +0100, Dave Mitchell wrote:
+1 Tim. |
From @jhiOn Tuesday-201604-05 4:48, Tim Bunce wrote:
+1 |
From @lightseyOn Tue, 2016-04-05 at 09:37 +0200, Salvatore Bonaccorso wrote:
Right, that bug report basically shows the full public lifespan of the Some of the responses to my email weren't sent to all of the parties I I agree that its a great idea going forward, but it still leaves my If fixing Perl 5.26+ is the only solution, it will result in a very For instance, the behavior in the apt-get tools is a local root exploit I would expect that leaving this vulnerability in place for several It's also difficult to keep fixing flaws like this in isolation without It would be very helpful to know which of the various pieces of code |
From zefram@fysh.orgJohn Lightsey wrote:
Patching older interpreters would change their behaviour in a breaking Modules are the wrong place to do this. It is not for a module to That leaves the script, i.e., the top-level program. This is the We should recommend that (all?) perl programs begin with an invocation BEGIN { pop @INC if $INC[-1] eq "."; } This invocation is portable back to very old perls, and will be safe to -zefram |
From @lightseyOn Wed, 2016-04-06 at 10:33 -0700, Zefram via RT wrote:
One compelling reason to avoid this approach is just the numbers. There For example, by default my Debian box has 136 perl scripts that do a If I fix the two most common modules that cause this (Storable loading It's much simpler to change 2 modules than 124 scripts as a security |
From @tonycozI've merged the extra tickets created into the main ticket. Please make sure [perl #127834] is in the subject when you reply to this ticket. Tony |
From @tonycozOn Tue Apr 05 01:07:50 2016, davem wrote:
This. I don't think Todd's PERL_USE_UNSAFE_INC environment variable should be included - if someone wants . in @INC they can set PERL5LIB, or add -I. to the #! line. The behaviour isn't exactly the same, but I expect it's close enough for most of the cases that expect . in @INC. Tony |
From @LeontOn Tue, Apr 5, 2016 at 5:23 AM, John Lightsey <
I've never liked this behavior TBH. I've recommended using taint mode on It's a mess :-( Leon |
From @toddr
So this exchange gets to the heart of the problem Zefram argues the module shouldn't be manipulating @INC. But as JD points out if we apply this simple fix, all sorts of code is magically secure: Storable does: if (eval { local $SIG{__DIE__}; require Log::Agent; 1 }) { In this case you could change it to this and fix Storable's issue: if (eval { local @INC= grep {$_ ne "." } @INC; local $SIG{__DIE__}; require Log::Agent; 1 }) { |
From zefram@fysh.orgTodd Rinaldo wrote:
This kind of patch, like the formulation that I proposed for top-level -zefram |
From @craigberryOn Sat, Apr 9, 2016 at 11:12 AM, Zefram <zefram@fysh.org> wrote:
FWIW, the @INC entry for current working directory is '.' on both VMS |
From @jhiOn Saturday-201604-09 12:53, Craig A. Berry wrote:
I asked Andy Broad and the UNIX emulation layer used in AmigaOS does Though to quote Andy: "Securitywise, on the amiga the horse bolted |
From zefram@fysh.orgJarkko Hietaniemi wrote:
Consistency of behaviour between systems is to be preferred, even when -zefram |
From @jhiOn Saturday-201604-09 20:38, Zefram wrote:
Sure. Nobody was arguing the opposite.
|
From @toddr
Based on the replies already, . in @INC is the same on all supported systems, right? So this is a possible fix? Todd |
From @tonycozOn Mon Apr 11 15:56:51 2016, toddr@cpanel.net wrote:
Considering your last patch set, I think: a) we should hide the unsafe_inc options a little deeper (require -Accflags=-D...), or not have it at all. b) we shouldn't have the environment variable, adjusting PERL5LIB is suitable for most purposes. Tony |
From @LeontOn Tue, Apr 12, 2016 at 3:02 AM, Tony Cook via RT <
I'd go for hiding. Making it impossible seems unperlish, but we do want to
PERL5LIB added to the front of @INC, while . is currently added to the Leon |
From @lightseyOn Tue, 2016-04-12 at 01:27 -0700, Leon Timmermans via RT wrote:
The downside to this approach is that the code using it must be aware Using a new environmental variable that returned the original @INC |
From @toddr
IMO This whole conversation about what to do for 5.26 needs to move to RT 127810 so it will do better as a public discussion. This thread seems like a better place for us to discuss the proper way to fix 5.24 and below and how to coordinate the security disclosure that's going to have to come with it, right? Todd |
From @lightseyOn Tue, 2016-04-12 at 09:33 -0700, Todd Rinaldo via RT wrote:
The discussion about what to do with 5.24 and earlier seems to have It's probably unrealistic to attempt coordinating simultaneous fixes I would think of this more like XXE attacks or the RLIM_NPROC setuid() I'm guessing, that the Debian security team does want to fix all of the Does the Perl security team want to fix the scripts it maintains that I'd really like to present this stuff publicly to make Perl developers What's a reasonable timeframe before publicly discussing attacks |
From @demerphqOn 15 April 2016 at 21:25, John Lightsey <lightsey@debian.org> wrote:
I think we are still figuring things out. Ric, our point man for these
I have been thinking about this. I guess it means we are vulnerable in Is that right?
I am just thinking that a lot of these bugs can be fixed by
I think we do.
I think this is already being half discussed in public, although I I worry that discussing the fix for 5.26 in a public thread is a good
IMO we would definitely like more time before this happens. Cheers, -- |
From @lightseyOn Sat, 2016-04-16 at 15:57 +0200, demerphq wrote:
More or less, yes... I'd think of it this way: - All Perl scripts start off with the risk of loading a module from the - The Perl script can eliminate this risk entirely by turning on taint - If the Perl script doesn't eliminate the risk directly, then the risk - Flawed scripts are widespread now because certain core modules try to In terms of ease of deployment and overall effect, you get much better Filtering @INC in Storable before it tries to load Log::Agent and in
Me too. I'd much prefer that the immediate issues with Perl scripts in It's difficult to explain why the @INC changes for 5.26 are important You'll have to forgive me and Todd for these two discussions running The main goal in looking for ways to abuse the @INC behavior in 5.24
|
From @fweimer* John Lightsey:
It's also an option that has reduced risk of breaking legitimate So from a distribution point-of-view, it is more work than a single |
From @xsawyerxOn Fri, Dec 23, 2016 at 8:25 PM, Karl Williamson
I would prefer having them in 5.24.3, making both 5.24.1 and 5.24.2 |
From @craigberryOn Fri, Dec 23, 2016 at 2:56 AM, Sawyer X <xsawyerx@gmail.com> wrote:
Thanks to those who have continued to work on this zombie ticket that How do we communicate the current state of affairs in a way that lets The base.pm changes committed to maint-5.24 two months ago, four I assume there is a category of CPAN modules that will break with all |
From @xsawyerxOn Fri, Dec 30, 2016 at 4:33 AM, Craig A. Berry <craig.a.berry@gmail.com> wrote:
I think this recent decision on 5.24.1 maintains a simple-to-follow The reason is simple: We kept improving this patch but we realize this The base.pm patch itself is also simpler to explain now: We originally Is that better? What I am very sorry about is the limbo situation of both 5.24.1 and
I think we should. However, this depends greatly on which patch is
I think we are still moving forward protecting against this risk, but Steve is working on RC5 any day now, which will simply not include the I wanted to write about this to p5p at large. If you believe this |
From @steve-m-hayOn 30 December 2016 at 10:51, Sawyer X <xsawyerx@gmail.com> wrote:
The changes to revert all the various base.pm changes that had been Those branches are now more or less what I will roll RC5 from in the Please look in particular at the changes I've made to perldelta.pod, |
From @craigberryOn Fri, Dec 30, 2016 at 4:51 AM, Sawyer X <xsawyerx@gmail.com> wrote:
But for now we're planning to ship with no patch at all to base.pm
I think it's important to say explicitly that it's reverting base.pm
That all sounds excellent, though I don't think any apology is
The general spirit is great. As I've indicated, a bit more detail in |
From @toddr
Honestly once 5.24.1 is out (without base.pm altered) I would argue that
+1. IMO we should make it clear that unlike the other changes, base.pm
I would suggest that once 5.24.1 is released and the security case is We should not make the same mistake of promising base.pm Todd |
From @craigberryOn Fri, Dec 30, 2016 at 12:53 PM, Todd Rinaldo <toddr@cpanel.net> wrote:
Do you mean no *active* exploits? I would have thought a demonstrator <https://rt-archive.perl.org/perl5/Ticket/Display.html?id=127834#txn-1416452> Or does it have to be in use in the wild to be considered "known"? |
From @toddr
Yeah we may be hitting a terminology mis-match here. https://youtu.be/dlIg4kQ4nRY?t=2m12s While base.pm arguably has is a vulnerability (though technically it's a problem with require) There aren't really any known exploits what haven't been patched already in what we agreed was the correct place (the script). From what I can tell, the patch we're discussing (although I am also in the dark as to WHICH patch we're even discussing) for base.pm tries to address fixing the unknown unknowns. No matter how we patch base.pm, it will almost certainly cause breakage. To me, this goes down the road of: "well if we're going to fix base.pm, why not require and use?" Todd |
From @ntyniOn Fri, Dec 30, 2016 at 12:31:47PM -0800, Todd Rinaldo via RT wrote:
FWIW Debian has /usr/bin/sbuild ("tool for building Debian binary Debian stable has had the first version (well, one of them I guess) Debian unstable/testing has 5.24.1-RC4 (therefore with the latest base.pm I think Ubuntu is using the same patches/packages but I'm not 100% Having 5.24.1 without any base.pm mitigations seems unfortunate to me, Many thanks to everybody who has worked on this burdensome issue. |
From @xsawyerxI feel like we're drifting off into other smaller parts of the bigger * There is already a strategy to resolve base.pm in a cleaner way. Craig has raised good points on what we need to make sure we Thank you for the comments. I hope we covered everything. |
From @LeontOn Sat, Dec 31, 2016 at 12:05 AM, Niko Tyni <ntyni@debian.org> wrote:
This is Exception::Class' sloppiness; if it would set $INC{$subclass_pm} to I still haven't seen a case where base.pm was truly used to optionally load Leon |
From @demerphqOn 31 December 2016 at 17:37, Leon Timmermans <fawaka@gmail.com> wrote:
Agreed. And I don't think the unknown people who might be doing so I have watched this from afar and I feel like a big part of it is I say that while fully respecting the effort and passions and that But sometimes I think tunnel vision sets in and people lose track of At $work when this came up we set up sitecustomize.pl to strip dot Yves -- |
From @LeontOn Sat, Dec 31, 2016 at 5:37 PM, Leon Timmermans <fawaka@gmail.com> wrote:
The attached patch to Exception::Class should secure sbuild and other users Leon |
From @Leont0001-Set-INC-for-generated-classes.patchFrom 64aaebbb46ae24ebf11754b98f05914769af5939 Mon Sep 17 00:00:00 2001
From: Leon Timmermans <fawaka@gmail.com>
Date: Sun, 1 Jan 2017 19:38:22 +0100
Subject: [PATCH] Set %INC for generated classes
---
lib/Exception/Class.pm | 3 +++
1 file changed, 3 insertions(+)
diff --git a/lib/Exception/Class.pm b/lib/Exception/Class.pm
index 996ce65..c58a5f1 100644
--- a/lib/Exception/Class.pm
+++ b/lib/Exception/Class.pm
@@ -188,6 +188,9 @@ EOPERL
eval $code;
die $@ if $@;
+ (my $filename = "$subclass.pm") =~ s{::}{/}g;
+ $INC{$filename} = __FILE__;
+
$CLASSES{$subclass} = 1;
}
--
2.11.0
|
From @toddr
https://github.com/houseabsolute/Exception-Class/pull/8 <https://github.com/houseabsolute/Exception-Class/pull/8> |
From @LeontOn Sun, Jan 1, 2017 at 8:55 PM, Todd Rinaldo <toddr@cpanel.net> wrote:
Merged :-) Leon |
From @jmdhOn Wed, Jan 04, 2017 at 10:08:45PM +0100, Leon Timmermans wrote:
Thanks all, this fix is now on its way to Debian unstable, and Cheers, |
From @jmdhOn Sat, Dec 31, 2016 at 07:10:45AM -0800, Sawyer X via RT wrote:
Hi Sawyer et al, Could you clarify what you mean here by 'the fix for @INC'? I read it as We can then move forward with enumerating the other tools that must be I know that ideally we would have a full cpantesters run to check the Cheers, |
From @xsawyerxOn Fri, Jan 6, 2017 at 2:17 PM, Dominic Hargreaves <dom@earth.li> wrote:
Hi Dominic, :)
Localized cleanup of @INC in core applications and modules. The set of
That is a related issue, but not the same. RT#130467 is when to flip I agree it fits as a blocker for 5.26.0 and I have just added it as such. |
From @iabynOn Mon, Apr 04, 2016 at 08:23:27PM -0700, John Lightsey wrote: I'm just checking on the status of this (still open) security ticket. Is there anything we still need to do for 5.26.0? -- |
From @xsawyerxOn Fri, Mar 17, 2017 at 12:42 PM, Dave Mitchell <davem@iabyn.com> wrote:
Nope.
base.pm is still not patched. Waiting for Aristotle to fully resolve his patch.
I think we resolved to only open this after the full patched 5.24.2
* 5.24.2 |
From @xsawyerxOn Fri, 17 Mar 2017 06:17:41 -0700, xsawyerx@gmail.com wrote:
Update to this: I have asked the Debian team about opening this ticket before 5.24.2/5.22.4 are out and they agree to have it disclosed. Unless someone objects, I will open the ticket on Monday. |
From @tonycozOn Thu, 18 May 2017 04:48:31 -0700, xsawyerx@cpan.org wrote:
Is this waiting on anything to be marked resolved? Tony |
From @jmdhOn Wed, Jun 14, 2017 at 05:51:13PM -0700, Tony Cook via RT wrote:
Presumably, releases of perl 5.24.x and 5.22.x including the base.pm Dominic. |
From @xsawyerxOn 06/19/2017 06:33 AM, Dominic Hargreaves wrote:
Agreed. |
From @khwilliamsonOn Tue, 20 Jun 2017 11:12:10 -0700, xsawyerx@gmail.com wrote:
Are these now done? -- |
From @steve-m-hayOn Fri, 28 Jul 2017 09:33:51 -0700, khw wrote:
5.22.4 and 5.24.2 have now been released with the base.pm fix, but the changes have also been ported forward to 5.26.x and blead. I'm not sure whether that means the ticket should be kept open until 5.26.1 and even 5.28.0 have been released? Arguably not, since other changes in those streams mean perl is now safe by default; the base.pm fix is only in them for the sake of anybody who disables the default safe mode. |
From @xsawyerxOn Fri, 28 Jul 2017 09:49:18 -0700, shay wrote:
I think we can resolve it now. And I have. |
@xsawyerx - Status changed from 'open' to 'resolved' |
Migrated from rt.perl.org#127834 (status was 'resolved')
Searchable as RT127834$
The text was updated successfully, but these errors were encountered: