Skip Menu |
Report information
Id: 128845
Status: rejected
Priority: 0/
Queue: perl5

Owner: jkeenan <jkeenan [at] cpan.org>
Requestors: chris.travers [at] gmail.com
Cc:
AdminCc:

Operating System: (no value)
PatchStatus: (no value)
Severity: low
Type: unknown
Perl Version: (no value)
Fixed In: (no value)



Subject: patch attached for perlsec.pod
Download (untitled) / with headers
text/plain 290b
I tried to make sure this would be put in a place that has not changed between versions. It is a brief description of attacks against the current directory and safe coding strategies. Feedback is welcome when you have time and bandwidth to give it (not expecting an immediate discussion).
Subject: perlsec.patch
Download perlsec.patch
text/plain 1.9k
--- a/perlsec.pod 2016-08-05 06:22:29.062358445 +0200 +++ b/perlsec.pod 2016-08-05 06:21:54.754415959 +0200 @@ -445,6 +445,42 @@ L<perlunicode> for details, and L<perlunicode/"Security Implications of Unicode"> for security implications in particular. +=head2 Code Inclusion from the Current Working Directory + +The current directory in the load path are potentially dangerous and +while taint mode provides some protection there are a number of key +problems that can crop up. Particularly when releasing published +software, programs can be attacked in various ways. Because of the +complexity of the dependency tree in many programs, these problems +can be very difficult to notice. + +=over 4 + +=item Falling back to current working directory for optional dependencies + +Current versions of Perl's C<@INC> contain the last entry as the current working +directory. This means that code that requires another module within an eval +statement will eventually look in the current working directory to see if that +file exists, but the file isn't required on the system, so if an attacker can +place an optional dependency of any dependency of your script in the current +working directory then, aside from Taint mode, your script may load and execute +the contents of the script. The safest solution is to use C<no lib '.'> at +the head of your Perl programs. + + +=item Code Preload Injection Attacks + +If a script has the current working directory at a high priority (for example +due to the use of C<use lib '.'> or C<-I.>, then if an attacker can place a +file with the same name as any dependency (optional or not) in the current +working directory or the program, then this file can transparently inject code +into the main program by loading and running code into memory, then unloading +itself and loading the correct module. In general, these are somewhat +dangerous with or without Taint Mode. + +=back + + =head2 Algorithmic Complexity Attacks Certain internal algorithms used in the implementation of Perl can
RT-Send-CC: perl5-porters [...] perl.org
On Thu Aug 04 21:26:04 2016, chris.travers@gmail.com wrote: Show quoted text
> I tried to make sure this would be put in a place that has not changed > between versions. It is a brief description of attacks against the > current directory and safe coding strategies. > > Feedback is welcome when you have time and bandwidth to give it (not > expecting an immediate discussion).
I think this is a good idea. Here are several suggestions for rewording it: Show quoted text
> --- a/perlsec.pod 2016-08-05 06:22:29.062358445 +0200 > +++ b/perlsec.pod 2016-08-05 06:21:54.754415959 +0200 > @@ -445,6 +445,42 @@ > L<perlunicode> for details, and L<perlunicode/"Security Implications > of Unicode"> for security implications in particular. > > +=head2 Code Inclusion from the Current Working Directory > + > +The current directory in the load path are potentially dangerous and
s/are/is; s/and$/and,/; Show quoted text
> +while taint mode provides some protection there are a number of key
s/protection/protection,/; Show quoted text
> +problems that can crop up. Particularly when releasing published > +software, programs can be attacked in various ways. Because of the > +complexity of the dependency tree in many programs, these problems > +can be very difficult to notice. > + > +=over 4 > + > +=item Falling back to current working directory for optional dependencies > + > +Current versions of Perl's C<@INC> contain the last entry as the current working > +directory.
contain the current working directory as the last entry Show quoted text
> This means that code that requires another module within an eval > +statement
‘eval’ is actually an expression. I would drop the word ‘statement,’ but I’m being pedantic. Show quoted text
> will
will, if the required module is not installed, eventually... Show quoted text
> eventually look in the current working directory to see if that > +file exists, but the file isn't required on the system, so if an attacker can
file exists. Since the file isn't required on the system, an attacker can Show quoted text
> +place an optional dependency of any dependency of your script in the current > +working directory then, aside from Taint mode,
s/aside.*mode/regardless of taint mode/ Show quoted text
> your script may load and execute > +the contents of the script. The safest solution is to use C<no lib '.'> at
s/the script/the file/ Show quoted text
> +the head of your Perl programs. > + > + > +=item Code Preload Injection Attacks > + > +If a script has the current working directory at a high priority (for example > +due to the use of C<use lib '.'> or C<-I.>, then if an attacker can place a > +file with the same name as any dependency (optional or not) in the current > +working directory or the program, then this file can transparently inject code
Did you mean ‘of’ by ‘or’? Show quoted text
> +into the main program by loading and running code into memory, then unloading > +itself and loading the correct module. In general, these are somewhat > +dangerous with or without Taint Mode.
s/Taint Mode/\L$&/ Show quoted text
> + > +=back > + > + > =head2 Algorithmic Complexity Attacks > > Certain internal algorithms used in the implementation of Perl can >
-- Father Chrysostomos
From: Sawyer X <xsawyerx [...] gmail.com>
Date: Fri, 5 Aug 2016 12:04:26 +0200
Subject: Re: [perl #128845] patch attached for perlsec.pod
To: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 579b
On 08/05/2016 07:25 AM, Father Chrysostomos via RT wrote: Show quoted text
> On Thu Aug 04 21:26:04 2016, chris.travers@gmail.com wrote:
>> I tried to make sure this would be put in a place that has not changed >> between versions. It is a brief description of attacks against the >> current directory and safe coding strategies. >> >> Feedback is welcome when you have time and bandwidth to give it (not >> expecting an immediate discussion).
> I think this is a good idea. Here are several suggestions for rewording it:
+1 for the documentation patch and for the rewording suggestions.
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.2k
On Fri Aug 05 03:05:00 2016, xsawyerx@gmail.com wrote: Show quoted text
> > +1 for the documentation patch and for the rewording suggestions. >
I replied before finishing my thoughts. First of all, there seems to be an agreement and push to remove '.' from @INC's last entry in the next version, which would make this patch (except for back-porting) incorrect. It should read that the current version of perl does not have it, while previous versions do. We could make this change now, or wait for that patch to be introduced and then alter the text again. This also means we should probably mention there were steps taken in core programs and modules to mitigate this by removing it for their use. I'm not that big on suggesting "no lib '.'" to prevent this. 1. This will no longer be part of perl coming 5.26, but also, since @INC entries can be strings and subroutines, this really does not cover all cases ("./.", "./", ".//", "././" etc.) and can give a false sense of security in both this respect and in other ways to make sure of optional module load paths attack vector and other possible attack vectors relating to this. What I +1 here is making a note about the security considerations of optional module loading *with* relative paths as a possible attack vector, and that this is why it has been removed since perl 5.26.
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 195b
On Fri Aug 05 03:17:09 2016, xsawyerx@cpan.org wrote: Show quoted text
> > [...] > optional module loading *with* relative paths as a possible attack
That's a misnomer, sorry. "relative paths" is what I meant.
To: perlbug-followup [...] perl.org
Subject: Re: [perl #128845] patch attached for perlsec.pod
Date: Fri, 5 Aug 2016 19:05:25 +0200
From: Chris Travers <chris.travers [...] gmail.com>
Download (untitled) / with headers
text/plain 2.1k


On Fri, Aug 5, 2016 at 12:17 PM, Sawyer X via RT <perlbug-followup@perl.org> wrote:
Show quoted text
On Fri Aug 05 03:05:00 2016, xsawyerx@gmail.com wrote:
>
> +1 for the documentation patch and for the rewording suggestions.
>

I replied before finishing my thoughts.

First of all, there seems to be an agreement and push to remove '.' from @INC's last entry in the next version, which would make this patch (except for back-porting) incorrect. It should read that the current version of perl does not have it, while previous versions do. We could make this change now, or wait for that patch to be introduced and then alter the text again. This also means we should probably mention there were steps taken in core programs and modules to mitigate this by removing it for their use.

I'm not that big on suggesting "no lib '.'" to prevent this. 1. This will no longer be part of perl coming 5.26, but also, since @INC entries can be strings and subroutines, this really does not cover all cases ("./.", "./", ".//", "././" etc.) and can give a false sense of security in both this respect and in other ways to make sure of optional module load paths attack vector and other possible attack vectors relating to this.

As I have had more time to think about what is appropriate for *this* piece of documentation I agree.

The problem as I see it is a bit different though.  I think the issue is that the no lib '.' suggestion breaks a common command line switch people use and that is appropriate only when people are willing to do that.  Discussing the tradeoff there is probably outside the scope of the perlsec document.  I am happy to recommend it personally but the medium does matter and this is not a medium where one can responsibly discuss this I think.
Show quoted text

What I +1 here is making a note about the security considerations of optional module loading *with* relative paths as a possible attack vector, and that this is why it has been removed since perl 5.26.


Agreed.

I will supply an updated patch with these suggestions addressed this weekend

--
Best Wishes,
Chris Travers

Efficito:  Hosted Accounting and ERP.  Robust and Flexible.  No vendor lock-in.
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 2.4k
On Sun, 07 Aug 2016 21:47:53 -0700, chris.travers@gmail.com wrote: Show quoted text
> On Fri, Aug 5, 2016 at 12:17 PM, Sawyer X via RT <perlbug- > followup@perl.org> > wrote: >
> > On Fri Aug 05 03:05:00 2016, xsawyerx@gmail.com wrote:
> > > > > > +1 for the documentation patch and for the rewording suggestions. > > >
> > > > I replied before finishing my thoughts. > > > > First of all, there seems to be an agreement and push to remove '.' > > from > > @INC's last entry in the next version, which would make this patch > > (except > > for back-porting) incorrect. It should read that the current version > > of > > perl does not have it, while previous versions do. We could make this > > change now, or wait for that patch to be introduced and then alter > > the text > > again. This also means we should probably mention there were steps > > taken in > > core programs and modules to mitigate this by removing it for their > > use. > > > > I'm not that big on suggesting "no lib '.'" to prevent this. 1. This > > will > > no longer be part of perl coming 5.26, but also, since @INC entries > > can be > > strings and subroutines, this really does not cover all cases ("./.", > > "./", > > ".//", "././" etc.) and can give a false sense of security in both > > this > > respect and in other ways to make sure of optional module load paths > > attack > > vector and other possible attack vectors relating to this. > >
> > As I have had more time to think about what is appropriate for *this* > piece > of documentation I agree. > > The problem as I see it is a bit different though. I think the issue > is > that the no lib '.' suggestion breaks a common command line switch > people > use and that is appropriate only when people are willing to do that. > Discussing the tradeoff there is probably outside the scope of the > perlsec > document. I am happy to recommend it personally but the medium does > matter > and this is not a medium where one can responsibly discuss this I > think. >
> > > > What I +1 here is making a note about the security considerations of > > optional module loading *with* relative paths as a possible attack > > vector, > > and that this is why it has been removed since perl 5.26. > >
> > > Agreed. > > I will supply an updated patch with these suggestions addressed this > weekend
Would it be possible for you to supply this patch in the next week. That is our final deadline before the next official release, and it would be good to get this in. -- Karl Williamson
RT-Send-CC: perl5-porters [...] perl.org
On Sat, 11 Feb 2017 18:36:35 GMT, khw wrote: Show quoted text
> On Sun, 07 Aug 2016 21:47:53 -0700, chris.travers@gmail.com wrote:
> > On Fri, Aug 5, 2016 at 12:17 PM, Sawyer X via RT <perlbug- > > followup@perl.org> > > wrote: > >
> > > On Fri Aug 05 03:05:00 2016, xsawyerx@gmail.com wrote:
> > > > > > > > +1 for the documentation patch and for the rewording suggestions. > > > >
> > > > > > I replied before finishing my thoughts. > > > > > > First of all, there seems to be an agreement and push to remove '.' > > > from > > > @INC's last entry in the next version, which would make this patch > > > (except > > > for back-porting) incorrect. It should read that the current > > > version > > > of > > > perl does not have it, while previous versions do. We could make > > > this > > > change now, or wait for that patch to be introduced and then alter > > > the text > > > again. This also means we should probably mention there were steps > > > taken in > > > core programs and modules to mitigate this by removing it for their > > > use. > > > > > > I'm not that big on suggesting "no lib '.'" to prevent this. 1. > > > This > > > will > > > no longer be part of perl coming 5.26, but also, since @INC entries > > > can be > > > strings and subroutines, this really does not cover all cases > > > ("./.", > > > "./", > > > ".//", "././" etc.) and can give a false sense of security in both > > > this > > > respect and in other ways to make sure of optional module load > > > paths > > > attack > > > vector and other possible attack vectors relating to this. > > >
> > > > As I have had more time to think about what is appropriate for *this* > > piece > > of documentation I agree. > > > > The problem as I see it is a bit different though. I think the issue > > is > > that the no lib '.' suggestion breaks a common command line switch > > people > > use and that is appropriate only when people are willing to do that. > > Discussing the tradeoff there is probably outside the scope of the > > perlsec > > document. I am happy to recommend it personally but the medium does > > matter > > and this is not a medium where one can responsibly discuss this I > > think. > >
> > > > > > What I +1 here is making a note about the security considerations > > > of > > > optional module loading *with* relative paths as a possible attack > > > vector, > > > and that this is why it has been removed since perl 5.26. > > >
> > > > > > Agreed. > > > > I will supply an updated patch with these suggestions addressed this > > weekend
> > Would it be possible for you to supply this patch in the next week. > That is our final deadline before the next official release, and it > would be good to get this in.
Since no updated patch was submitted during the balance of the 5.26 development cycle, and since there was additional documentation related to "no-dot-by-default-in-@INC" subsequent to the discussion in this ticket, I am marking this ticket Rejected. Any new suggestions for documentation updates should be provided in a new RT. Thank you very much. -- James E Keenan (jkeenan@cpan.org)


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

For issues related to this RT instance (aka "perlbug"), please contact perlbug-admin at perl.org