From mmaslano@redhat.com

Created by mmaslano@redhat.com

The new version of spam assassin is affected by perl tainted
problem. They've found a workaround/patch to fix this module
to be working again which can be found here​:
https://issues.apache.org/SpamAssassin/show_bug.cgi?id=6148
The tested version of perl was done on perl-5.10.0.

Flags:
    category=library
    severity=medium

This perlbug was built using Perl 5.10.0 in the Fedora build system.
It is being executed now by Perl 5.10.0 - Mon Jul 27 08:45:23 CEST 2009.

Site configuration information for perl 5.10.0:

Configured by Red Hat, Inc. at Mon Jul 27 08:45:23 CEST 2009.
Summary of my perl5 (revision 5 version 10 subversion 0) configuration:
  Platform:
    osname=linux, osvers=2.6.29.5-191.fc11.x86_64, archname=x86_64-linux-thread-multi
    uname='linux caladan 2.6.29.5-191.fc11.x86_64 #1 smp tue jun 16 23:23:21 edt 2009 x86_64 x86_64 x86_64 gnulinux '
    config_args='-des -Doptimize=-O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic -Accflags=-DPERL_USE_SAFE_P
UTENV -Dversion=5.10.0 -Dmyhostname=localhost -Dperladmin=root@localhost -Dcc=gcc -Dcf_by=Red Hat, Inc. -Dprefix=/usr -Dvendorprefix=/usr -Dsiteprefix=/usr/local -Dprivlib=/usr/lib
/perl5/5.10.0 -Dsitelib=/usr/local/lib/perl5/site_perl/5.10.0 -Dvendorlib=/usr/lib/perl5/vendor_perl/5.10.0 -Darchlib=/usr/lib64/perl5/5.10.0/x86_64-linux-thread-multi -Dsitearch=/
usr/local/lib64/perl5/site_perl/5.10.0/x86_64-linux-thread-multi -Dvendorarch=/usr/lib64/perl5/vendor_perl/5.10.0/x86_64-linux-thread-multi -Dinc_version_list=none -Darchname=x86_6
4-linux-thread-multi -Dlibpth=/usr/local/lib64 /lib64 /usr/lib64 -Duseshrplib -Dusethreads -Duseithreads -Duselargefiles -Dd_dosuid -Dd_semctl_semun -Di_db -Ui_ndbm -Di_gdbm -Di_sh
adow -Di_syslog -Dman3ext=3pm -Duseperlio -Dinstallusrbinperl=n -Ubincompat5005 -Uversiononly -Dpager=/usr/bin/less -isr -Dd_gethostent_r_proto -Ud_endhostent_r_proto -Ud_sethosten
t_r_proto -Ud_endprotoent_r_proto -Ud_setprotoent_r_proto -Ud_endservent_r_proto -Ud_setservent_r_proto -Dscriptdir=/usr/bin -Dotherlibdirs=/usr/lib/perl5/site_perl'
    hint=recommended, useposix=true, d_sigaction=define
    useithreads=define, usemultiplicity=define
    useperlio=define, d_sfio=undef, uselargefiles=define, usesocks=undef
    use64bitint=define, use64bitall=define, uselongdouble=undef
    usemymalloc=n, bincompat5005=undef
  Compiler:
    cc='gcc', ccflags ='-D_REENTRANT -D_GNU_SOURCE -DPERL_USE_SAFE_PUTENV -DDEBUGGING -fno-strict-aliasing -pipe -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -I/
usr/include/gdbm',
    optimize='-O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic',
    cppflags='-D_REENTRANT -D_GNU_SOURCE -DPERL_USE_SAFE_PUTENV -DDEBUGGING -fno-strict-aliasing -pipe -I/usr/local/include -I/usr/include/gdbm'
    ccversion='', gccversion='4.4.0 20090506 (Red Hat 4.4.0-4)', gccosandvers=''
    intsize=4, longsize=8, ptrsize=8, doublesize=8, byteorder=12345678
    d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=16
    ivtype='long', ivsize=8, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=8
    alignbytes=8, prototype=define
  Linker and Libraries:
    ld='gcc', ldflags =''
    libpth=/usr/local/lib64 /lib64 /usr/lib64
    libs=-lresolv -lnsl -lgdbm -ldb -ldl -lm -lcrypt -lutil -lpthread -lc
    perllibs=-lresolv -lnsl -ldl -lm -lcrypt -lutil -lpthread -lc
    libc=/lib/libc-2.10.1.so, so=so, useshrplib=true, libperl=libperl.so
    gnulibc_version='2.10.1'
  Dynamic Linking:
    dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E -Wl,-rpath,/usr/lib64/perl5/5.10.0/x86_64-linux-thread-multi/CORE'
    cccdlflags='-fPIC', lddlflags='-shared -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic'

Locally applied patches:


@INC for perl 5.10.0:
    /usr/local/lib64/perl5/site_perl/5.10.0/x86_64-linux-thread-multi
    /usr/local/lib/perl5/site_perl/5.10.0
    /usr/lib64/perl5/vendor_perl/5.10.0/x86_64-linux-thread-multi
    /usr/lib/perl5/vendor_perl/5.10.0
    /usr/lib/perl5/vendor_perl
    /usr/lib64/perl5/5.10.0/x86_64-linux-thread-multi
    /usr/lib/perl5/5.10.0
    /usr/lib/perl5/site_perl
    .


Environment for perl 5.10.0:
    HOME=/home/marca
    LANG=en_US.UTF-8
    LANGUAGE=
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)
    PATH=/usr/lib64/qt-3.3/bin:/usr/kerberos/bin:/usr/local/bin:/usr/bin:/bin:/usr/games:/usr/local/sbin:/usr/sbin:/sbin:/home/marca/bin
    PERL_BADLANG (unset)
    SHELL=/bin/bash

From @obra

Marcela,

After reviewing the linked spamassassin bug, it's not clear to me what's
going on here other than that "Something in spamassassin is causing $1 to
get tainted". The linked fixes are very much bandaids to stop the
propagation of such a thing.

It would be hugely helpful to us if you could give us a bit more
information about what you folks are seeing that's causing the tainting
bug.

Thanks,
Jesse

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

From mmaslano@redhat.com

On 10/10/2009 10​:47 PM, Jesse via RT wrote​:

Marcela,

After reviewing the linked spamassassin bug, it's not clear to me what's
going on here other than that "Something in spamassassin is causing $1 to
get tainted". The linked fixes are very much bandaids to stop the
propagation of such a thing.

It would be hugely helpful to us if you could give us a bit more
information about what you folks are seeing that's causing the tainting
bug.

Thanks,
Jesse

Upstream claims it was solved by this commit

0abd0d7 - disable non-unicode case insensitive trie matching
This bug can be closed.

--
Marcela Mašlá�ová
BaseOS team Brno

From @Tux

Upstream claims it was solved by this commit

0abd0d7 - disable non-unicode case
insensitive trie matching

@Tux - Status changed from 'open' to 'resolved'

@demerphq - Status changed from 'resolved' to 'open'

From @demerphq

I have reopened this ticket. That this patch fixed this problem says
that the explanation of the problem is wrong or something much deeper is
wrong.

From Mark.Martinec@ijs.si

Upstream claims it was solved by this commit

0abd0d7 - disable non-unicode case
insensitive trie matching

Where does this claim come from?

I'm the reporter of the #69973 bug (closed by the above mentioned patch),
but also have years of encounters (at amavisd-new and SpamAssassin) with
cases where global variables $1, $2, etc get tainted by some complex
expression, and subsequently start spreading taintedness in unrelated
regular expressions - and I doubt these two cases have much in common.

I tried several times to boil down a tainted-$1 case to a smallish
test program, but I failed so far. Within a complex program I can
pinpoint the expressions where $1 gets tainted and stays so, e.g.
within IO​::Handle​::_open_mode_string​:

sub _open_mode_string {
  my ($mode) = @​_;
  $mode =~ /^\+?(<|>>?)$/
  or $mode =~ s/^r(\+?)$/$1</
  or $mode =~ s/^w(\+?)$/$1>/
  or $mode =~ s/^a(\+?)$/$1>>/
  or croak "IO​::Handle​: bad open mode​: $mode";
  $mode;
}

yet when this code is isolated in a test program the problem
can no longer be reproduced - which is why I haven't reported
the problem to perl bug tracking so far. If I remember correctly
this nightmare started with perl 5.8.0.

Workarounds range from avoiding use of r/w/a in IO​::File​::open,
to isolating code sections by localizing $1, $2, ... to a close
neighbourhood to where it is used. Using the local($1) within blocks
turns out to be the best practice when writing new code to avoid
future surprises.

  Mark

From Mark.Martinec@ijs.si

See also​:
  https://issues.apache.org/SpamAssassin/show_bug.cgi?id=3838#c11

Mark

From @demerphq

2009/11/3 Mark Martinec <Mark.Martinec@​ijs.si>​:

Upstream claims it was solved by this commit

0abd0d7 - disable non-unicode case
insensitive trie matching

Where does this claim come from?

I'm the reporter of the #69973 bug (closed by the above mentioned patch),
but also have years of encounters (at amavisd-new and SpamAssassin) with
cases where global variables $1, $2, etc get tainted by some complex
expression, and subsequently start spreading taintedness in unrelated
regular expressions - and I doubt these two cases have much in common.

I tried several times to boil down a tainted-$1 case to a smallish
test program, but I failed so far. Within a complex program I can
pinpoint the expressions where $1 gets tainted and stays so, e.g.
within IO​::Handle​::_open_mode_string​:

sub _open_mode_string {
   my ($mode) = @​_;
   $mode =~ /^\+?(<|>>?)$/
     or $mode =~ s/^r(\+?)$/$1</
     or $mode =~ s/^w(\+?)$/$1>/
     or $mode =~ s/^a(\+?)$/$1>>/
     or croak "IO​::Handle​: bad open mode​: $mode";
   $mode;
}

yet when this code is isolated in a test program the problem
can no longer be reproduced - which is why I haven't reported
the problem to perl bug tracking so far. If I remember correctly
this nightmare started with perl 5.8.0.

Workarounds range from avoiding use of r/w/a in IO​::File​::open,
to isolating code sections by localizing $1, $2, ... to a close
neighbourhood to where it is used. Using the local($1) within blocks
turns out to be the best practice when writing new code to avoid
future surprises.

Are you experiencing it on 5.10? And what about 5.11/blead?

cheers,
Yves

--
perl -Mre=debug -e "/just|another|perl|hacker/"

From @demerphq

2009/11/3 Mark Martinec <Mark.Martinec@​ijs.si>​:

Are you experiencing it on 5.10? And what about 5.11/blead?

I believe the
 https://issues.apache.org/SpamAssassin/show_bug.cgi?id=6148#c1
was the last documented case of this kind, with 5.10.0.

I'm pretty well barricaded behind these local($1) containments now,
so probably I wouldn't notice other cases.

I'm running 5.10.1 on our mailers now. I suppose I could
remove these localizations of $1,$2,etc and see what happens.
Will let you know if I can reproduce it on 5.10.1.
(although the OP of this problem report seems to have
suffered form such a case).

Yes, knowing if this is fixed without the localizations would be nice.

Also it would be really nice to get to the bottom of this.

I have looked at the regex code and i have looked at the $1 fetch
logic and i dont see how it possibly could ever be tainted.

At the very least we should assert that it isnt.

Yves

--
perl -Mre=debug -e "/just|another|perl|hacker/"

From Mark.Martinec@ijs.si

Are you experiencing it on 5.10? And what about 5.11/blead?

I believe the
  https://issues.apache.org/SpamAssassin/show_bug.cgi?id=6148#c1
was the last documented case of this kind, with 5.10.0.

I'm pretty well barricaded behind these local($1) containments now,
so probably I wouldn't notice other cases.

I'm running 5.10.1 on our mailers now. I suppose I could
remove these localizations of $1,$2,etc and see what happens.
Will let you know if I can reproduce it on 5.10.1.
(although the OP of this problem report seems to have
suffered form such a case).

  Mark

From Mark.Martinec@ijs.si

Yves,

I'm running 5.10.1 on our mailers now. I suppose I could
remove these localizations of $1,$2,etc and see what happens.
Will let you know if I can reproduce it on 5.10.1.

Done. And I believe I have it distilled now to a small test case.

Also it would be really nice to get to the bottom of this.

I have looked at the regex code and i have looked at the $1 fetch
logic and i dont see how it possibly could ever be tainted.

At the very least we should assert that it isnt.

#!/usr/bin/perl -T

  use strict;
  use re 'taint';
  use Scalar​::Util qw(tainted);

  my $mailbox = 'abc@​example.com';
  $mailbox .= substr($ENV{PATH},0,0); # make it tainted

  # $1 and $2 become tainted
  my(@​r) = $mailbox =~ /^(.*?)(\@​.*)$/ ? ($1,$2) : ($mailbox,'');
  printf("%d %d\n", tainted($1), tainted($2));

  my($nm) = 'aaa-ccc'; # not tainted
  printf("%d\n", tainted($nm));

  $nm =~ s/^aaa-(.*)$/$1/; # $nm becomes tainted
  printf("%d\n", tainted($nm));

Mark

From @rgarcia

2009/11/5 Mark Martinec <Mark.Martinec@​ijs.si>​:

At the very least we should assert that it isnt.

#!/usr/bin/perl -T

 use strict;
 use re 'taint';
 use Scalar​::Util qw(tainted);

 my $mailbox = 'abc@​example.com';
 $mailbox .= substr($ENV{PATH},0,0);  # make it tainted

 # $1 and $2 become tainted
 my(@​r) = $mailbox =~ /^(.*?)(\@​.*)$/ ? ($1,$2) : ($mailbox,'');
 printf("%d %d\n", tainted($1), tainted($2));

 my($nm) = 'aaa-ccc';  # not tainted
 printf("%d\n", tainted($nm));

 $nm =~ s/^aaa-(.*)$/$1/;  # $nm becomes tainted
 printf("%d\n", tainted($nm));

At 1st glance I would say that is because $1 and $2 appear in the same
expression than the tainted $mailbox and thus become tainted, just
like the rest of the expression. As says perlsec :

| For efficiency reasons, Perl takes a conservative view of
| whether data is tainted. If an expression contains tainted data,
| any subexpression may be considered tainted, even if the value
| of the subexpression is not itself affected by the tainted data.

From @demerphq

2009/11/6 Mark Martinec <Mark.Martinec@​ijs.si>​:

On Thursday November 5 2009 23​:57​:05 Rafael Garcia-Suarez wrote​:

 # $1 and $2 become tainted
 my(@​r) = $mailbox =~ /^(.*?)(\@​.*)$/ ? ($1,$2) : ($mailbox,'');
[...]
 $nm =~ s/^aaa-(.*)$/$1/;  # $nm becomes tainted

At 1st glance I would say that is because $1 and $2 appear in the same
expression than the tainted $mailbox and thus become tainted, just

I don't think that is the problem per se.

The point is that in the s/^aaa-(.*)$/$1/ the $1 is supposed
to get assigned an entirely new value, as captured by the regexp,
and hence it should lose its taintedness flag!

I concur, it seems to me to be a bug if $1 becomes tainted at all.

I mean the whole way of detainting things is via rewgex capture vars,
so if they become tainted it seems like a problem.

Yves

--
perl -Mre=debug -e "/just|another|perl|hacker/"

From Mark.Martinec@ijs.si

On Thursday November 5 2009 23​:57​:05 Rafael Garcia-Suarez wrote​:

# $1 and $2 become tainted
my(@​r) = $mailbox =~ /^(.*?)(\@​.*)$/ ? ($1,$2) : ($mailbox,'');
[...]
$nm =~ s/^aaa-(.*)$/$1/; # $nm becomes tainted

At 1st glance I would say that is because $1 and $2 appear in the same
expression than the tainted $mailbox and thus become tainted, just

I don't think that is the problem per se.

The point is that in the s/^aaa-(.*)$/$1/ the $1 is supposed
to get assigned an entirely new value, as captured by the regexp,
and hence it should lose its taintedness flag!

  Mark

From Mark.Martinec@ijs.si

my(@​r) = $mailbox =~ /^(.*?)(\@​.*)$/ ? ($1,$2) : ($mailbox,'');
$nm =~ s/^aaa-(.*)$/$1/; # $nm becomes tainted

I concur, it seems to me to be a bug if $1 becomes tainted at all.

I mean the whole way of detainting things is via rewgex capture vars,
so if they become tainted it seems like a problem.

Not to forget that the program uses​: use re 'taint', so I believe
it is reasonable to expect that $1 and $2 will receive tainted
captures of a tainted target string.

Not sure if it still applies to 5.10.1, but I should point out
that these tainted-$1 incidents were noticed with perl 5.8.?
even before starting to apply the use re 'taint' (and doing
untainting by explicitly calling a dedicated subroutine).

While I can understand how the $nm =~ s/^aaa-(.*)$/$1/ propagates
taintedness of $1 onto $nm (due to a simplistic taint logic),
I wonder if something can be done to ditch the old contents and
flags of $1 before it gets a chance to taint the whole expression.

  Mark

From @demerphq

2009/11/6 Mark Martinec <Mark.Martinec@​ijs.si>​:

On Thursday November 5 2009 23​:57​:05 Rafael Garcia-Suarez wrote​:

 # $1 and $2 become tainted
 my(@​r) = $mailbox =~ /^(.*?)(\@​.*)$/ ? ($1,$2) : ($mailbox,'');
[...]
 $nm =~ s/^aaa-(.*)$/$1/;  # $nm becomes tainted

At 1st glance I would say that is because $1 and $2 appear in the same
expression than the tainted $mailbox and thus become tainted, just

I don't think that is the problem per se.

The point is that in the s/^aaa-(.*)$/$1/ the $1 is supposed
to get assigned an entirely new value, as captured by the regexp,
and hence it should lose its taintedness flag!

I totally missed the re 'taint'. apologies.

yves

--
perl -Mre=debug -e "/just|another|perl|hacker/"

From @iabyn

On Thu, Nov 05, 2009 at 09​:28​:10PM +0100, Mark Martinec wrote​:

Yves,

I'm running 5.10.1 on our mailers now. I suppose I could
remove these localizations of $1,$2,etc and see what happens.
Will let you know if I can reproduce it on 5.10.1.

Done. And I believe I have it distilled now to a small test case.

Also it would be really nice to get to the bottom of this.

I have looked at the regex code and i have looked at the $1 fetch
logic and i dont see how it possibly could ever be tainted.

At the very least we should assert that it isnt.

#!/usr/bin/perl -T

use strict;
use re 'taint';
use Scalar​::Util qw(tainted);

my $mailbox = 'abc@​example.com';
$mailbox .= substr($ENV{PATH},0,0); # make it tainted

# $1 and $2 become tainted
my(@​r) = $mailbox =~ /^(.*?)(\@​.*)$/ ? ($1,$2) : ($mailbox,'');
printf("%d %d\n", tainted($1), tainted($2));

my($nm) = 'aaa-ccc'; # not tainted
printf("%d\n", tainted($nm));

$nm =~ s/^aaa-(.*)$/$1/; # $nm becomes tainted
printf("%d\n", tainted($nm));

Now fixed by commit 447ee13
in branch davem/post-5.12, which should be merged back into blead
once 5.12 has been released, and thus appear in 5.13 onwards​:

commit 447ee13
Author​: David Mitchell <davem@​iabyn.com>
AuthorDate​: Thu Mar 25 10​:56​:35 2010 +0000
Commit​: David Mitchell <davem@​iabyn.com>
CommitDate​: Thu Mar 25 10​:56​:35 2010 +0000

  RT #67962​: $1 treated as tainted in untainted match
 
  Fix the issue in the following​:
 
  use re 'taint';
  $tainted =~ /(...)/;
  # $1 now correctly tainted
  $untainted =~ s/(...)/$1/;
  # $untainted now incorrectly tainted
 
  The problem stems from when $1 is updated.
 
  pp_substcont, which is called after the replacement expression has been
  evaluated, checks the returned expression for taintedness, and if so,
  taints the variable being substituted. For a substitution like
  s/(...)/x$1/ this works fine​: the expression "x".$1 causes $1's get magic
  to be called, which sets $1 based on the recent match, and is marked as
  not tainted. Thus the returned expression is untainted. In the variant
  s/(...)/$1/, the returned value on the stack is $1 itself, and its get
  magic hasn't been called yet. So it still has the tainted flag from the
  previous pattern.
 
  The solution is to mg_get the returned expression *before* testing for
  taintedness.

Affected files ...
 
  M pp_ctl.c
  M t/op/taint.t

Differences ...

diff --git a/pp_ctl.c b/pp_ctl.c
index de34879..a35cd43 100644
--- a/pp_ctl.c
+++ b/pp_ctl.c
@@ -278,9 +278,11 @@ PP(pp_substcont)
 	if (cx->sb_iters > cx->sb_maxiters)
 	    DIE(aTHX_ "Substitution loop");
 
+	SvGETMAGIC(TOPs); /* possibly clear taint on $1 etc: #67962 */
+
 	if (!(cx->sb_rxtainted & 2) && SvTAINTED(TOPs))
 	    cx->sb_rxtainted |= 2;
-	sv_catsv(dstr, POPs);
+	sv_catsv_nomg(dstr, POPs);
 	/* XXX: adjust for positive offsets of \G for instance s/(.)\G//g with positive pos() */
 	s -= RX_GOFS(rx);
 
diff --git a/t/op/taint.t b/t/op/taint.t
index f601552..e3a5712 100644
--- a/t/op/taint.t
+++ b/t/op/taint.t
@@ -17,7 +17,7 @@ use Config;
 use File::Spec::Functions;
 
 BEGIN { require './test.pl'; }
-plan tests => 321;
+plan tests => 325;
 
 $| = 1;
 
@@ -1380,6 +1380,22 @@ foreach my $ord (78, 163, 256) {
 }
 
 
+# Bug RT #67962: old tainted $1 gets treated as tainted
+# in next untainted # match
+
+{
+    use re 'taint';
+    "abc".$TAINT =~ /(.*)/; # make $1 tainted
+    ok(tainted($1), '$1 should be tainted');
+
+    my $untainted = "abcdef";
+    ok(!tainted($untainted), '$untainted should be untainted');
+    $untainted =~ s/(abc)/$1/;
+    ok(!tainted($untainted), '$untainted should still be untainted');
+    $untainted =~ s/(abc)/x$1/;
+    ok(!tainted($untainted), '$untainted should yet still be untainted');
+}
+
 
 # This may bomb out with the alarm signal so keep it last
 SKIP: {



-- 

In the 70's we wore flares because we didn't know any better.
What possible excuse does the current generation have?

@iabyn - Status changed from 'open' to 'resolved'