Skip to content
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

rename wrongly considered insecure in an elsif clause #12415

Open
p5pRT opened this issue Sep 14, 2012 · 5 comments
Open

rename wrongly considered insecure in an elsif clause #12415

p5pRT opened this issue Sep 14, 2012 · 5 comments

Comments

@p5pRT
Copy link

p5pRT commented Sep 14, 2012

Migrated from rt.perl.org#114908 (status was 'open')

Searchable as RT114908$

@p5pRT
Copy link
Author

p5pRT commented Sep 14, 2012

From @mhasch

This is a bug report for perl from mhasch@​cpan.org,
generated with the help of perlbug 1.39 running under perl 5.16.1.


Perl seems to consider "rename" to be an insecure operation in
taint mode if it happens to be called in an elsif clause chained
after an if clause involving something tainted. This seems to
be the case in perl5.8.9, perl5.10.1, perl5.14.2 and perl5.16.1.

A test for this is attached below. I thank Bram on #p5p for
help in boiling down the test case.

-Martin



Flags​:
  category=core
  severity=medium


Site configuration information for perl 5.16.1​:

Configured by ***** at Fri Aug 10 10​:33​:58 CEST 2012.

Summary of my perl5 (revision 5 version 16 subversion 1) configuration​:
 
  Platform​:
  osname=linux, osvers=2.6.32-5-686, archname=i686-linux-64int-ld
  uname='linux ******* 2.6.32-5-686 #1 smp sun may 6 04​:01​:19 utc 2012 i686 gnulinux '
  config_args='-Dprefix=/opt/perl516 -Duse64bitint -Duselongdouble -Dperladmin=none -Dusevfork=false -de'
  hint=recommended, useposix=true, d_sigaction=define
  useithreads=undef, usemultiplicity=undef
  useperlio=define, d_sfio=undef, uselargefiles=define, usesocks=undef
  use64bitint=define, use64bitall=undef, uselongdouble=define
  usemymalloc=n, bincompat5005=undef
  Compiler​:
  cc='cc', ccflags ='-fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64',
  optimize='-O2',
  cppflags='-fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include'
  ccversion='', gccversion='4.3.5', gccosandvers=''
  intsize=4, longsize=4, ptrsize=4, doublesize=8, byteorder=12345678
  d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=12
  ivtype='long long', ivsize=8, nvtype='long double', nvsize=12, Off_t='off_t', lseeksize=8
  alignbytes=4, prototype=define
  Linker and Libraries​:
  ld='cc', ldflags =' -fstack-protector -L/usr/local/lib'
  libpth=/usr/local/lib /lib/../lib /usr/lib/../lib /lib /usr/lib /usr/lib64
  libs=-lnsl -ldb -ldl -lm -lcrypt -lutil -lc
  perllibs=-lnsl -ldl -lm -lcrypt -lutil -lc
  libc=/lib/libc-2.11.3.so, so=so, useshrplib=false, libperl=libperl.a
  gnulibc_version='2.11.3'
  Dynamic Linking​:
  dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E'
  cccdlflags='-fPIC', lddlflags='-shared -O2 -L/usr/local/lib -fstack-protector'

Locally applied patches​:
 


@​INC for perl 5.16.1​:
  /opt/perl516/lib/site_perl/5.16.1/i686-linux-64int-ld
  /opt/perl516/lib/site_perl/5.16.1
  /opt/perl516/lib/5.16.1/i686-linux-64int-ld
  /opt/perl516/lib/5.16.1
  /opt/perl516/lib/site_perl/5.16.0/i686-linux-64int-ld
  /opt/perl516/lib/site_perl/5.16.0
  /opt/perl516/lib/site_perl
  .


Environment for perl 5.16.1​:
  HOME=/home/******
  LANG=C
  LANGUAGE (unset)
  LD_LIBRARY_PATH (unset)
  LOGDIR (unset)
  PATH=/opt/perl516/bin​:/home/******/bin​:/usr/local/bin​:/usr/bin​:/bin​:/usr/games
  PERL_BADLANG (unset)
  SHELL=/bin/bash

@p5pRT
Copy link
Author

p5pRT commented Sep 14, 2012

From @mhasch

#!/usr/bin/perl -T

use strict;
use warnings;
use Test​::More;

my $target_filename = 'elsif_rename_target.txt';
my $source_filename = 'elsif_rename_source.txt';

open my $fh, '>', $source_filename
  or plan skip_all => "$source_filename​: $!";
close $fh;

plan tests => 1;

my $r = eval {
  if ($0 eq '///') {
  # not reached
  }
  elsif (rename $source_filename, $target_filename) {
  # not relevant
  }
  1
};
if (!$r) {
  diag $@​;
}
ok($r, 'rename was allowed');

unlink $target_filename, $source_filename;

__END__
1..1
# Insecure dependency in rename while running with -T switch at elsif_rename.t line 20.
not ok 1 - rename was allowed
# Failed test 'rename was allowed'
# at elsif_rename.t line 28.
# Looks like you failed 1 test of 1.

@p5pRT
Copy link
Author

p5pRT commented May 27, 2013

From @jkeenan

On Fri Sep 14 10​:24​:05 2012, mhasch@​cpan.org wrote​:

This is a bug report for perl from mhasch@​cpan.org,
generated with the help of perlbug 1.39 running under perl 5.16.1.

-----------------------------------------------------------------
Perl seems to consider "rename" to be an insecure operation in
taint mode if it happens to be called in an elsif clause chained
after an if clause involving something tainted. This seems to
be the case in perl5.8.9, perl5.10.1, perl5.14.2 and perl5.16.1.

A test for this is attached below. I thank Bram on #p5p for
help in boiling down the test case.

The "insecure dependency" message refers one to 'perlsec'. I believe
the following is the relevant paragraph therefrom​:

#####
You may not use data derived from outside your program to affect
something else outside your program--at least, not by accident. All
command line arguments, environment variables, locale information (see
L<perllocale>), results of certain system calls (C<readdir()>,
C<readlink()>, the variable of C<shmread()>, the messages returned by
C<msgrcv()>, the password, gcos and shell fields returned by the
C<getpwxxx()> calls), and all file input are marked as "tainted".
Tainted data may not be used directly or indirectly in any command
that invokes a sub-shell, nor in any command that modifies files,
directories, or processes, B<with the following exceptions>​:
#####

... and renaming a file does not appear in that list of exceptions. My
guess is that, for good reason or bad, renaming in taint mode is
considered "any command that modifies files." So this seems to be a
case of "works as designed" -- though perhaps the design is sub-optimal.

Thank you very much.
Jim Keenan

@p5pRT
Copy link
Author

p5pRT commented May 27, 2013

The RT System itself - Status changed from 'new' to 'open'

@p5pRT
Copy link
Author

p5pRT commented May 27, 2013

From p5p@spam.wizbit.be

On Sun May 26 17​:15​:32 2013, jkeenan wrote​:

On Fri Sep 14 10​:24​:05 2012, mhasch@​cpan.org wrote​:

... and renaming a file does not appear in that list of exceptions.
My
guess is that, for good reason or bad, renaming in taint mode is
considered "any command that modifies files."

This is not the case..

If you change the 'elsif' to an 'if' then the test case passes.

Also note that the message is not specifc to rename.
If you change the elsif to 'elsif (open my $fh, ">",, $source_filename)
{' for example then the test will also fail.

There are two ways to make the test case work​:
* change the 'elsif (rename ..) ' to if (rename ....)'
* make sure the condition in the if block does not use a tainted value

The problem here is that the taint mode of the if-block is leaking into
the elsif block.. (which it shouldn't).

So this seems to be a
case of "works as designed" -- though perhaps the design is sub-
optimal.

To me this behaviour does not appear to be 'works as designed' but
looks more like a bug...

Best regards,

Bram

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants