Debian Bug report logs - #544931
[uscan] Regressions due to security fix

Toggle useless messages

Message #5 received at submit@bugs.debian.org (full text, mbox, reply):

From: "Adam D. Barratt" <adam@adam-barratt.org.uk>

To: submit@bugs.debian.org

Subject: [uscan] Regressions due to security fix

Date: Thu, 03 Sep 2009 19:50:21 +0100

Package: devscripts
Severity: serious
Version: 2.10.54

The security fixes introduced in 2.10.54 has a regression which should
not migrate to testing.

The regular expression parser always evaluates the entire expression,
making it trivial to create infinite loops with expressions such as
"s/(\d)/$1./g".

Adam

Message #8 received at 544931@bugs.debian.org (full text, mbox, reply):

From: Raphael Geissert <geissert@debian.org>

To: 544931@bugs.debian.org

Cc: control@bugs.debian.org

Subject: Re: Bug#544931: [uscan] Regressions due to security fix

Date: Fri, 4 Sep 2009 16:42:52 -0500

[Message part 1 (text/plain, inline)]

tag 544931 patch
thanks

I've tried many cases and they all passed.

Cheers,
-- 
Raphael Geissert - Debian Developer
www.debian.org - get.debian.net

[uscan-il-fix.patch (text/x-diff, inline)]

diff --git a/scripts/uscan.pl b/home/raphael/Deb/devscripts-2.10.54/scripts/uscan.pl
index 11fbc54..64fc9e5 100755
--- a/scripts/uscan.pl
+++ b/home/raphael/Deb/devscripts-2.10.54/scripts/uscan.pl
@@ -1846,7 +1846,6 @@ sub safe_replace($$) {
 
     if ($sep eq '{' or $sep eq '(' or $sep eq '[' or $sep eq '<') {
 	($parsed_ok, $regexp, $replacement, $flags) = quoted_regex_parse($pat);
-
 	return 0 unless $parsed_ok;
     } elsif ($pat !~ /^(?:s|tr|y)$esc((?:\\.|[^\\$esc])*)$esc((?:\\.|[^\\$esc])*)$esc([a-z]*)$/) {
 	return 0;
@@ -1899,16 +1898,18 @@ sub safe_replace($$) {
 	# a copy. We'll need to restore it to the current value if
 	# the global flag was set on the input pattern.
 	my $orig_replacement = $replacement;
+	my $prev = qr'.*?';
 
 	while (1) {
 	    eval {
 		# handle errors due to unsafe constructs in $regexp
 		no re 'eval';
 
-		my $re = qr/$flags$regexp/;
+		my $re = qr/^$prev\K$flags$regexp/;
 
 		@captures = ($$in =~ $re);
 		($first, $last) = ($-[0], $+[0]);
+		$prev = substr($$in, 0, $-[0]);
 	    };
 	    return 0 if $@;
 
@@ -1931,6 +1932,9 @@ sub safe_replace($$) {
 	    # Actually do the replacement
 	    substr $$in, $first, $last - $first, $replacement;
 
+	    $prev .= substr $$in, $first, length($replacement);
+	    $prev = qr,\Q$prev\E.*?,;
+
 	    if ($global) {
 		$replacement = $orig_replacement;
 	    } else {

Message #15 received at 544931@bugs.debian.org (full text, mbox, reply):

From: Ryan Niebur <ryanryan52@gmail.com>

To: 544931@bugs.debian.org

Subject: Re: Bug#544931: [uscan] Regressions due to security fix

Date: Fri, 4 Sep 2009 15:47:35 -0700

[Message part 1 (text/plain, inline)]

On Fri, Sep 04, 2009 at 04:42:52PM -0500, Raphael Geissert wrote:
> tag 544931 patch
> thanks
> 
> I've tried many cases and they all passed.
> 

applied in PET, seems okay for pkg-perl's repo.

-- 
_________________________
Ryan Niebur
ryanryan52@gmail.com

[signature.asc (application/pgp-signature, inline)]

Message #20 received at 544931@bugs.debian.org (full text, mbox, reply):

From: Martín Ferrari <martin.ferrari@gmail.com>

To: 544931@bugs.debian.org

Subject: another problem in the patch

Date: Sat, 5 Sep 2009 15:36:54 +0200

Hi Raphael, et al.

I've been also working on trying to produce a fix for this issue, but
I was stuck with dealing with zero-width matches, which are quite
tricky to handle. I just tried and your patch doesn't handle this,
take for example s/()/./g:


$ perl -MPET::Watch -e '$r = shift; $s = shift;
PET::Watch::safe_replace($s, $r); print "$s\n"' 's/()/./g' foo
foo

$ perl -e '$r = shift; $s = shift; eval "\$s =~ $r"; print "$s\n"'
's/()/./g' foo
.f.o.o.

I know this is a corner case, but it's quite a deviation from the
standard behaviour of regexes.

-- 
Martín Ferrari

Message #25 received at 544931@bugs.debian.org (full text, mbox, reply):

From: Martín Ferrari <martin.ferrari@gmail.com>

To: 544931@bugs.debian.org

Subject: Re: another problem in the patch

Date: Sat, 5 Sep 2009 18:35:43 +0200

On Sat, Sep 5, 2009 at 15:36, Martín Ferrari<martin.ferrari@gmail.com> wrote:

> $ perl -MPET::Watch -e '$r = shift; $s = shift;
> PET::Watch::safe_replace($s, $r); print "$s\n"' 's/()/./g' foo
> foo

This was a mistake, I didn't call the function properly. Raphael's
patch makes this loop forever!

$ time perl -MPET::Watch -e '$r = shift; $s = shift;
PET::Watch::safe_replace(\$s, $r); print "$s\n"' 's/()/./g' foo
^C

real	0m3.250s
user	0m3.116s
sys	0m0.040s

After a lot of reading, I found one solution to this:

$ perl -MPET::Watch -e '$r = shift; $s = shift;
PET::Watch::safe_replace(\$s, $r); print "$s\n"' 's/()/./g' foo
.f.o.o.

See the patch below, or refer to revision 748 of
svn://svn.debian.org/svn/pet/trunk/PET/Watch.pm

@@ -608,8 +608,6 @@
 	my $global = ($flags =~ s/g//);
 	$flags = "(?$flags)" if length $flags;

-	my (@captures, $first, $last);
-
 	# Behave like Perl and treat e.g. "\." in replacement as "."
 	# We allow the case escape characters to remain and
 	# process them later
@@ -629,24 +627,35 @@
 	# the global flag was set on the input pattern.
 	my $orig_replacement = $replacement;

+        my ($first, $last, $pos, $matched, @captures) = (0, -1);
 	while (1) {
 	    eval {
 		# handle errors due to unsafe constructs in $regexp
 		no re 'eval';

-		my $re = qr/$flags$regexp/;
-
-		@captures = ($$in =~ $re);
+                # restore position
+                pos($$in) = $pos if($pos);
+                if($last - $first == 0) {
+                    # previous match was a zero-width match, simulate it to set
+                    # the internal flag that avoids the infinite loop
+                    $$in =~ /()/g;
+                }
+                # Need to use /g to make it use and save pos()
+                $matched = scalar($$in =~ /$flags$regexp/g);
+                # save position and size of the match
+                $pos = pos($$in);
 		($first, $last) = ($-[0], $+[0]);
+                for my $i (0..$#-) {
+                    $captures[$i] = substr $$in, $-[0], $+[0] - $-[0];
+                }
 	    };
 	    return 0 if $@;

 	    # No match; leave the original string  untouched but return
 	    # success as there was nothing wrong with the pattern
-	    return 1 if @captures == 0;
+            return 1 unless $matched;

 	    # Replace $X
-	    unshift @captures, substr $$in, $first, $last - $first;
 	    $replacement =~ s/[\$\\](\d)/defined $captures[$1] ?
$captures[$1] : ''/ge;
 	    $replacement =~ s/\$\{(\d)\}/defined $captures[$1] ?
$captures[$1] : ''/ge;
 	    $replacement =~ s/\$&/$captures[0]/g;
@@ -659,14 +668,12 @@

 	    # Actually do the replacement
 	    substr $$in, $first, $last - $first, $replacement;
+            # Update position
+            $pos += length($replacement)- ($last - $first);

-	    if ($global) {
+            last unless($global);
 		$replacement = $orig_replacement;
-	    } else {
-		last;
 	    }
- 	}
-
 	return 1;
     }
 }

-- 
Martín Ferrari

Message #30 received at 544931@bugs.debian.org (full text, mbox, reply):

From: "Adam D. Barratt" <adam@adam-barratt.org.uk>

To: Martín Ferrari <martin.ferrari@gmail.com>, 544931@bugs.debian.org

Subject: Re: Bug#544931: another problem in the patch

Date: Sat, 05 Sep 2009 18:55:21 +0100

On Sat, 2009-09-05 at 18:35 +0200, Martín Ferrari wrote:
> On Sat, Sep 5, 2009 at 15:36, Martín Ferrari<martin.ferrari@gmail.com> wrote:
> 
> > $ perl -MPET::Watch -e '$r = shift; $s = shift;
> > PET::Watch::safe_replace($s, $r); print "$s\n"' 's/()/./g' foo
> > foo
> 
> This was a mistake, I didn't call the function properly. Raphael's
> patch makes this loop forever!

Indeed. We (well I) knew that though. :) Raphael just hadn't had time to
look at zero-width matches.

> See the patch below, or refer to revision 748 of
> svn://svn.debian.org/svn/pet/trunk/PET/Watch.pm

Thanks for the patch.  There were a couple of issues with it, one of
which I know you're aware of - the "build array of captures" loop always
used the first capture as it needed s/0/$i/g. The other was that Perl's
magic regex engine variables - @- etc - are only changed on /successful/
matches.  If the last match attempt failed then those variables retain
their contents from the last successful match.  In this case that meant
a load of Perl warnings as the code attempted to access beyond the end
of the string.

I've included an updated patch below.  PET SVN has also been updated to
fix both of the issues mentioned above.

I also reverted your changes near the bottom of the diff as, whilst I
agree that getting rid of the "if foo bar else baz" block results in
cleaner code, I'd like to keep the diff for the security team as small
as possible.

Regards,

Adam

--- scripts/uscan.pl	(revision 1995)
+++ scripts/uscan.pl	(working copy)
@@ -1880,8 +1880,6 @@
 	my $global = ($flags =~ s/g//);
 	$flags = "(?$flags)" if length $flags;
 
-	my (@captures, $first, $last);
-
 	# Behave like Perl and treat e.g. "\." in replacement as "."
 	# We allow the case escape characters to remain and
 	# process them later
@@ -1901,24 +1899,39 @@
 	# the global flag was set on the input pattern.
 	my $orig_replacement = $replacement;
 
+        my ($first, $last, $pos, $matched, @captures) = (0, -1);
 	while (1) {
 	    eval {
 		# handle errors due to unsafe constructs in $regexp
 		no re 'eval';
 
-		my $re = qr/$flags$regexp/;
+		# restore position
+		pos($$in) = $pos if($pos);
 
-		@captures = ($$in =~ $re);
-		($first, $last) = ($-[0], $+[0]);
+		if($last - $first == 0) {
+		    # previous match was a zero-width match, simulate it to set
+		    # the internal flag that avoids the infinite loop
+		    $$in =~ /()/g;
+		}
+		# Need to use /g to make it use and save pos()
+		$matched = scalar($$in =~ /$flags$regexp/g);
+
+		if ($matched) {
+		    # save position and size of the match
+		    $pos = pos($$in);
+		    ($first, $last) = ($-[0], $+[0]);
+		    for my $i (0..$#-) {
+			$captures[$i] = substr $$in, $-[$i], $+[$i] - $-[$i];
+		    }
+		}
 	    };
 	    return 0 if $@;
 
 	    # No match; leave the original string  untouched but return
 	    # success as there was nothing wrong with the pattern
-	    return 1 if @captures == 0;
+	    return 1 unless $matched;
 
 	    # Replace $X
-	    unshift @captures, substr $$in, $first, $last - $first;
 	    $replacement =~ s/[\$\\](\d)/defined $captures[$1] ?
$captures[$1] : ''/ge;
 	    $replacement =~ s/\$\{(\d)\}/defined $captures[$1] ?
$captures[$1] : ''/ge;
 	    $replacement =~ s/\$&/$captures[0]/g;
@@ -1931,6 +1944,8 @@
 
 	    # Actually do the replacement
 	    substr $$in, $first, $last - $first, $replacement;
+	    # Update position
+	    $pos += length($replacement)- ($last - $first);
 
 	    if ($global) {
 		$replacement = $orig_replacement;

Message #35 received at 544931@bugs.debian.org (full text, mbox, reply):

From: "Adam D. Barratt" <adam@adam-barratt.org.uk>

To: 544931@bugs.debian.org

Cc: Martín Ferrari <martin.ferrari@gmail.com>

Subject: Re: Bug#544931: another problem in the patch

Date: Sat, 05 Sep 2009 22:59:38 +0100

On Sat, 2009-09-05 at 18:55 +0100, Adam D. Barratt wrote:
> On Sat, 2009-09-05 at 18:35 +0200, Martín Ferrari wrote:
> > See the patch below, or refer to revision 748 of
> > svn://svn.debian.org/svn/pet/trunk/PET/Watch.pm
[...]
> I've included an updated patch below.  PET SVN has also been updated to
> fix both of the issues mentioned above.

fwiw, further testing has thrown up another issue, regarding \G - namely
that it doesn't work. :)  To be honest I'm debating whether lack of
support for \G is really a problem, given that it isn't used in any
watchfile in the archive currently.

We did discover that the pattern "s/(.)\G./-/" produces a number of Perl
warnings as it appears to make Perl's regex engine deeply unhappy.  With
your patch applied, $matches ends up as 1 whilst $#- is -1, causing the
warnings.  The original uscan code causes a segfault, which appears to
be directly attributable to perl:

	$ echo foo | perl -pe "s/(.)\G./-/g"
	Segmentation fault

In any case, the additional line below gets rid of the warnings.  The
result returned is incorrect, but whether that matters depends on
whether we care about supporting \G.

[...]
> +		if ($matched) {
> +		    # save position and size of the match
> +		    $pos = pos($$in);
> +		    ($first, $last) = ($-[0], $+[0]);
> +		    for my $i (0..$#-) {
> +			$captures[$i] = substr $$in, $-[$i], $+[$i] - $-[$i];
> +		    }

			$matched = 0 unless $#- >= 0;

> +		}

Regards,

Adam

Message #38 received at 544931@bugs.debian.org (full text, mbox, reply):

From: Raphael Geissert <geissert@debian.org>

To: 544931@bugs.debian.org

Cc: Martín Ferrari <martin.ferrari@gmail.com>

Subject: Re: Bug#544931: another problem in the patch

Date: Sat, 5 Sep 2009 17:24:20 -0500

Hi Martín, Adam, *

[Martín, hope you don't bother that I'm CC'ing you]

While checking something unrelated to the last patch I found that the 
following regex triggers an il:

opts=uversionmangle=sx(.?)\Gxprint("foo")xg \
http://sf.net/kcheckgmail/kcheckgmail-(.+)\.tar\.gz

Other than that and #545234, I have not been able to break it, for now.

What do you all think about the two patches?
regarding my patch: fixing zero width matches should be easy, and that's the 
only known failure atm
regarding Martín's-based patch: I'm not very keen on playing with pos(), and 
that was part of the reasons why I preferred to implement my fix the way I 
did it.

Cheers,
-- 
Raphael Geissert - Debian Developer
www.debian.org - get.debian.net

Message #43 received at 544931@bugs.debian.org (full text, mbox, reply):

From: Martín Ferrari <martin.ferrari@gmail.com>

To: Raphael Geissert <geissert@debian.org>

Cc: 544931@bugs.debian.org

Subject: Re: Bug#544931: another problem in the patch

Date: Sun, 6 Sep 2009 04:33:23 +0200

On Sun, Sep 6, 2009 at 00:24, Raphael Geissert<geissert@debian.org> wrote:
> Hi Martín, Adam, *
>
> [Martín, hope you don't bother that I'm CC'ing you]

Not at all, I'm not subscribed to the bug.

> While checking something unrelated to the last patch I found that the
> following regex triggers an il:
>
> opts=uversionmangle=sx(.?)\Gxprint("foo")xg \
> http://sf.net/kcheckgmail/kcheckgmail-(.+)\.tar\.gz

Regarding this and what Adam's said before, from perlop man page:

Note also that, currently,
               "\G" is only properly supported when anchored at the very
               beginning of the pattern.

So, I don't think we need to support any of these two (broken)
examples as they rely on unspecified behaviour. The correct behaviour
for \G *does* work, as you can see in the following snippet:

> Other than that and #545234, I have not been able to break it, for now.$ perl -MPET::Watch -e '$r = shift; $s = shift; PET::Watch::safe_replace(\$s, $r) or die; print "$s\n"' 's/o|\Gb/#/g' foobarbaz
f###arbaz

The \G makes the first b replaced but not the second.

> What do you all think about the two patches?
> regarding my patch: fixing zero width matches should be easy, and that's the
> only known failure atm
> regarding Martín's-based patch: I'm not very keen on playing with pos(), and
> that was part of the reasons why I preferred to implement my fix the way I
> did it.

Well, I'm obviously biased, but I really prefer the pos() version as
it mimics exactly what the regex engine does.

-- 
Martín Ferrari

Message #46 received at 544931@bugs.debian.org (full text, mbox, reply):

From: Raphael Geissert <atomo64@gmail.com>

To: Martín Ferrari <martin.ferrari@gmail.com>

Cc: 544931@bugs.debian.org

Subject: Re: Bug#544931: another problem in the patch

Date: Sat, 5 Sep 2009 21:54:02 -0500

On Saturday 05 September 2009 21:33:23 Martín Ferrari wrote:
> On Sun, Sep 6, 2009 at 00:24, Raphael Geissert<geissert@debian.org> wrote:
> > While checking something unrelated to the last patch I found that the
> > following regex triggers an il:
> >
> > opts=uversionmangle=sx(.?)\Gxprint("foo")xg \
> > http://sf.net/kcheckgmail/kcheckgmail-(.+)\.tar\.gz
>
> Regarding this and what Adam's said before, from perlop man page:
>
> Note also that, currently,
>                "\G" is only properly supported when anchored at the very
>                beginning of the pattern.
>
> So, I don't think we need to support any of these two (broken)
> examples as they rely on unspecified behaviour. The correct behaviour
>
> for \G *does* work, as you can see in the following snippet:
> > Other than that and #545234, I have not been able to break it, for now.$
> > perl -MPET::Watch -e '$r = shift; $s = shift;
> > PET::Watch::safe_replace(\$s, $r) or die; print "$s\n"' 's/o|\Gb/#/g'
> > foobarbaz
>
> f###arbaz
>
> The \G makes the first b replaced but not the second.
>

Sure, and like I said early on IRC, my patch doesn't support \G either; but 
also like I said, even if not supported we should not let uscan enter an il.

> > What do you all think about the two patches?
> > regarding my patch: fixing zero width matches should be easy, and that's
> > the only known failure atm
> > regarding Martín's-based patch: I'm not very keen on playing with pos(),
> > and that was part of the reasons why I preferred to implement my fix the
> > way I did it.
>
> Well, I'm obviously biased, but I really prefer the pos() version as
> it mimics exactly what the regex engine does.

And its bugs, as demonstrated above :)

Cheers,
-- 
Raphael Geissert - Debian Developer
www.debian.org - get.debian.net

Message #57 received at 544931-close@bugs.debian.org (full text, mbox, reply):

From: adam@adam-barratt.org.uk (Adam D. Barratt)

To: 544931-close@bugs.debian.org

Subject: Bug#544931: fixed in devscripts 2.10.55

Date: Fri, 11 Sep 2009 10:26:20 +0000

Source: devscripts
Source-Version: 2.10.55

We believe that the bug you reported is fixed in the latest version of
devscripts, which is due to be installed in the Debian FTP archive:

devscripts_2.10.55.dsc
  to pool/main/d/devscripts/devscripts_2.10.55.dsc
devscripts_2.10.55.tar.gz
  to pool/main/d/devscripts/devscripts_2.10.55.tar.gz
devscripts_2.10.55_amd64.deb
  to pool/main/d/devscripts/devscripts_2.10.55_amd64.deb



A summary of the changes between this version and the previous one is
attached.

Thank you for reporting the bug, which will now be closed.  If you
have further comments please address them to 544931@bugs.debian.org,
and the maintainer will reopen the bug report if appropriate.

Debian distribution maintenance software
pp.
Adam D. Barratt <adam@adam-barratt.org.uk> (supplier of updated devscripts package)

(This message was generated automatically at their request; if you
believe that there is a problem with it please contact the archive
administrators by mailing ftpmaster@debian.org)


-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Format: 1.8
Date: Fri, 11 Sep 2009 10:02:52 +0100
Source: devscripts
Binary: devscripts
Architecture: source amd64
Version: 2.10.55
Distribution: unstable
Urgency: high
Maintainer: Devscripts Devel Team <pkg-devscripts@teams.debian.net>
Changed-By: Adam D. Barratt <adam@adam-barratt.org.uk>
Description: 
 devscripts - scripts to make the life of a Debian Package maintainer easier
Closes: 542961 544931 545099 545327
Changes: 
 devscripts (2.10.55) unstable; urgency=high
 .
   * Urgency "high" for security fix fix
 .
   [ Ryan Niebur ]
   * uscan: fix a regression from the security fix when the command is
     given parameters. use Text::ParseWords::shellwords to parse the
     given command.
   * dch: fix comparison of versions when the new and current version are
     the same (Closes: #545099)
 .
   [ Adam D. Barratt ]
   * bts: Refer to the "--no-ack" option rather than the incorrect "--no-acks"
     in the description of the BTS_SUPPRESS_ACKS variable.  Thanks, Jakub
     Wilk.  (Closes: #542961)
   * licensecheck: Add missing blank line in POD.  Thanks, Nicolas Francois.
   * uscan: Fix regressions introduced by the security fix in the previous
     upload.  The parser now correctly keeps track of the last match when /g
     is used to avoid infinite loops.  Thanks, Raphael Geissert and Martín
     Ferrari.  (Closes: #544931)
   * l10n updates; thanks, Nicolas Francois:  (Closes: #545327)
     + Update French manpage translations
     + Fix addendum format for dcontrol in po4a/devscripts-po4a.conf
     + Use --previous when invoking po4a
   * debian/control: Bump version of po4a build-dependency to 0.31 to ensure
     support for --previous.
Checksums-Sha1: 
 1d7b69b68be13efd42585281c9e51c6aa849d87b 1399 devscripts_2.10.55.dsc
 82cf7176aa4eaa059bbb3cbe1fba445d50886684 690881 devscripts_2.10.55.tar.gz
 04b8b6093b5e7f97bbeec941b52dfec49a12086d 591714 devscripts_2.10.55_amd64.deb
Checksums-Sha256: 
 85caf05ad712f2142448b94da447311d4eef4751b7422c30761d31aace52a7bd 1399 devscripts_2.10.55.dsc
 346b37974bda0f3e8fc00128ab4cbec059df626699a11262a5653f94d14f28ac 690881 devscripts_2.10.55.tar.gz
 098efdf3358dab27ea584afca88435058f3b4dc6fedec8977c52dcd6289c7985 591714 devscripts_2.10.55_amd64.deb
Files: 
 05e802a4eecbeb35e713d3940c5025f0 1399 devel optional devscripts_2.10.55.dsc
 ef4c4aa009c7b0ec75a828052a449455 690881 devel optional devscripts_2.10.55.tar.gz
 ebe10998305de0fa8a59bd13e5f945a7 591714 devel optional devscripts_2.10.55_amd64.deb

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)

iEYEARECAAYFAkqqEtAACgkQokcE1TReOoX5HQCeKSQFALTVtjNMdYLppsULt3q0
v04An37O+FQ7PF+WeN22+NQV029A38lt
=OmwO
-----END PGP SIGNATURE-----

Send a report that this bug log contains spam.