Bugzilla will be upgraded to version 5.0. The upgrade date is tentatively scheduled for 2 December 2018, pending final testing and feedback.
Bug 1378885 - Perl version module does not accept tainted input
Perl version module does not accept tainted input
Status: CLOSED ERRATA
Product: Red Hat Enterprise Linux 7
Classification: Red Hat
Component: perl-version (Show other bugs)
7.2
Unspecified Unspecified
unspecified Severity unspecified
: rc
: ---
Assigned To: Petr Pisar
Jakub Heger
Lenka Špačková
: Patch
Depends On:
Blocks: 1298243 1420851 1465904 1466370 1473612 1380363
  Show dependency treegraph
 
Reported: 2016-09-23 09:02 EDT by Jindrich Novy
Modified: 2018-04-10 04:38 EDT (History)
6 users (show)

See Also:
Fixed In Version: perl-version-0.99.07-3.el7
Doc Type: Release Note
Doc Text:
The *version* Perl module now supports tainted input and tainted version objects Previously, the *version* module of Perl was unable to correctly parse tainted input. Consequently, when building a version object from a tainted variable, the `version->new()` method reported the `Invalid version format (non-numeric data)` error. This update adds support for parsing tainted input and for printing tainted version objects and strings.
Story Points: ---
Clone Of:
Environment:
Last Closed: 2018-04-10 04:38:01 EDT
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
Support for parsing tainted input (first part) (1.69 KB, patch)
2016-09-27 10:15 EDT, Petr Pisar
no flags Details | Diff
Support for stringyfing tainted version (second part) (1.13 KB, patch)
2016-09-27 10:17 EDT, Petr Pisar
no flags Details | Diff


External Trackers
Tracker ID Priority Status Summary Last Updated
CPAN 118087 None None None 2016-09-26 11:19 EDT
Red Hat Product Errata RHBA-2018:0663 None None None 2018-04-10 04:38 EDT

  None (edit)
Description Jindrich Novy 2016-09-23 09:02:42 EDT
Description of problem:
There is a regression in version.pm behavior in case tainting is enabled.

Version-Release number of selected component (if applicable):
perl-version-0.99.07-2.el7.x86_64

How reproducible:
1. write the following script:
#!/usr/bin/perl -Tw
use version;
$ENV{PATH}='/usr/bin';
$a=`echo 1.2.3`;
chomp $a;
version->new($a);

Steps to Reproduce:
Run the script.

Actual results:
Invalid version format (non-numeric data) at ./p.pl line 6.

Expected results:
works


Additional info:
Run like perl -w instead of perl-wT to make it work.
Comment 1 Petr Pisar 2016-09-23 09:55:53 EDT
Upstream 0.9908 version does not suffer from this problem.
Comment 2 Jindrich Novy 2016-09-23 10:07:03 EDT
Thanks Petr.

Any plans to update to 0.9908 any time soon in RHEL7? RHEL7u3?

Could you please provide me with a testing package so that I can confirm?
Comment 4 Petr Pisar 2016-09-26 09:06:10 EDT
I'm sorry for a positive message. It's not corrected in any upstream version. There are two version module implementations (a pure-Perl vpp and XS vxs) and it looks like the XS one suffers from this issue. But not when running on recent perl.
Comment 5 Frank Hirtz 2016-09-26 09:36:54 EDT
A quick test appeared to confirm that 9908 didn't show the issue reproduced by the test script on RHEL7, all else being constant:

<snip>
[root@dell-r430-6 SPECS]# rpm -q perl-version
perl-version-0.99.07-2.el7.x86_64
[root@dell-r430-6 SPECS]# ./test.pl 
Invalid version format (non-numeric data) at ./test.pl line 6.
[root@dell-r430-6 SPECS]# rpm -Uvh ~/perl-version-0.99.08-1test.el7_2.x86_64.rpm 
Preparing...                          ################################# [100%]
Updating / installing...
   1:perl-version-3:0.99.08-1test.el7_################################# [ 50%]
Cleaning up / removing...
   2:perl-version-3:0.99.07-2.el7     ################################# [100%]
[root@dell-r430-6 SPECS]# rpm -q perl-version
perl-version-0.99.08-1test.el7_2.x86_64
[root@dell-r430-6 SPECS]# ./test.pl 
[root@dell-r430-6 SPECS]# 
</snip>

Is this expected since (I don't believe) we're using the vxs interface here?
Comment 6 Petr Pisar 2016-09-26 09:52:12 EDT
We should use vxs. Strace the old version to see no vpp module is loaded. I don't undetstand yet why the the fallback happens.

Moreover the reproducer in the original report is incomplete. It's necessary to check the string value of version->new($a) is equaled to "1.2.3". This is not true for 0.9908 that returns empty string.
Comment 7 Petr Pisar 2016-09-26 10:48:00 EDT
I located the change in the perl. Large commit:

4bac9ae47b5ad7845a24e26b0e95609805de688a is the first bad commit
commit 4bac9ae47b5ad7845a24e26b0e95609805de688a
Author: Chip Salzenberg <chip@pobox.com>
Date:   Fri Jun 22 15:18:18 2012 -0700

    Magic flags harmonization.
[...]
    This patch removes the shift mechanic for magic flags, thus exposing (and
    fixing) some improper usage of magic SVs in which mg_get() was not called
    correctly.  It also has the side effect of making magic get functions
    specifically set their SVs to undef if that is desired, as the new behavior
    of empty get functions is to leave the value unchanged.  This is a feature,
    as now get magic that does not modify its value, e.g. tainting, does not
    have to be special cased.


changes various internals and this is not definitely something we want to port into perl. It causes vxs version implementation to handle tainted values.

I think the version module was never intended to handle tainted values. I will ask upstream what he thinks about it.
Comment 8 Petr Pisar 2016-09-26 10:52:49 EDT
(In reply to Petr Pisar from comment #6)
> I don't undetstand yet why the the fallback happens.
>
That's fall back happened because version::vxs did not match version module because code from upstream mercurial repository will often forget to update the $version::vxs::VERSION value.
Comment 9 Petr Pisar 2016-09-26 11:07:33 EDT
The change in version responsible for not croaking about "non-numeric data" is this commit:

changeset:   480:efb44fbc6bd8
user:        John Peacock <john.peacock@havurah-software.org>
date:        Sat Feb 01 13:30:16 2014 -0500
summary:     Deal with certain tiedscalars (e.g. created by Readonly::XS).

It added this code:

--- a/vutil/vutil.c     Sat Feb 01 08:55:41 2014 -0500
+++ b/vutil/vutil.c     Sat Feb 01 13:30:16 2014 -0500
[...]
@@ -605,7 +609,11 @@
        version = savesvpv(ver);
        SAVEFREEPV(version);
     }
-    else if ( SvPOK(ver) )/* must be a string or something like a string */
+    else if ( SvPOK(ver)
+#if PERL_VERSION_LT(5,17,2)
+             || (SvTYPE(ver) == SVt_PVMG && SvPOKp(ver))
+#endif
+           )/* must be a string or something like a string */
     {
        STRLEN len;
        version = savepvn(SvPV(ver,len), SvCUR(ver));

That's responsible crunching the tainted string, thus not croaking. But still because of missing the above-mentioned perl change it does not retrieve the value and stores an empty string instead.
Comment 10 Petr Pisar 2016-09-27 03:50:51 EDT
By the way, how exactly is this bug a regression?

As far as I know, the first RHEL-7 prerelease had perl-version-0.99.02-3.el7 that does not die with the error, but returns an empty string and that's also incorrect behavior. Then first official RHEL-7 release delivered perl-version-0.99.07-1.el7 that dies with the reported error and it does not differ from current perl-version-0.99.07-2.el7.x86_64.

It probably behaves differently from RHEL-6, but Red Hat does not guarantee Perl compatibility across multiple RHEL major versions.

I talked to version module author, and he tends to ban the tainted input completely. That means the current perl-version-0.99.07-2.el7.x86_64 behavior plus ban in the pure-perl variant could become the new official behavior. If you have different opinion how the future (= upstream) version should behave, you can express your ideas in the upstream report <http://rt.cpan.org/Public/Bug/Display.html?id=118087>.
Comment 11 Jindrich Novy 2016-09-27 05:05:44 EDT
Thanks Petr.

In the meantime we will have a look whether to disable tainting for module in question which fails due to this regression.
Comment 12 Jindrich Novy 2016-09-27 06:20:04 EDT
... by regression I mean regression from ordinary Perl behaviour across RHELs. If it is broken since the first release of RHEL7 then it is still a regression/bug as this behaviour is obviously not a feature but broken functionality.
Comment 13 Jindrich Novy 2016-09-27 09:08:42 EDT
Just for the record - updating the reproducer to:

#!/usr/bin/perl -Tw
use version;
$ENV{PATH}='/usr/bin';
$a=`echo 1.2.3`;
chomp $a;
#($a) = $a =~ /(.*)/;
$v = version->new($a);
$vStr = $v->stringify();
if ($a eq $vStr) {
    print "Version is good\n"
} else {
    print "Version '$vStr' is bad\n";
    exit 1;
}

which is also sensitive to an empty version string (thanks to Mark Wilson).
Comment 14 Petr Pisar 2016-09-27 10:15 EDT
Created attachment 1205220 [details]
Support for parsing tainted input (first part)

This is the upstream code from 0.9908 not to die on a tainted input.
Comment 15 Petr Pisar 2016-09-27 10:17 EDT
Created attachment 1205222 [details]
Support for stringyfing tainted version (second part)

This is a proposed fix to allow printing version object that was created from tainted input. The output string will be also tainted as the input.
Comment 16 Petr Pisar 2016-09-27 10:49:11 EDT
I developed a fix that adds support for parsing tainted input and stringifying such objects. An unsupported testing package is available on <http://people.redhat.com/~ppisar/perl-version-0.99.07-3.el7/>.
Comment 23 errata-xmlrpc 2018-04-10 04:38:01 EDT
Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHBA-2018:0663

Note You need to log in before you can comment on or make changes to this bug.