Bug 21079 - perltidy new security issue CVE-2016-10374
Summary: perltidy new security issue CVE-2016-10374
Status: ASSIGNED
Alias: None
Product: Mageia
Classification: Unclassified
Component: Security (show other bugs)
Version: 5
Hardware: All Linux
Priority: Normal normal
Target Milestone: ---
Assignee: QA Team
QA Contact: Sec team
URL:
Whiteboard: feedback
Keywords:
Depends on:
Blocks:
 
Reported: 2017-06-12 12:13 CEST by David Walser
Modified: 2017-07-25 21:00 CEST (History)
5 users (show)

See Also:
Source RPM: perltidy-20160302.0.0-2.mga6.src.rpm
CVE:
Status comment:


Attachments
Example perl script with indentations removed. (667 bytes, text/plain)
2017-07-06 02:40 CEST, Len Lawrence
Details

Description David Walser 2017-06-12 12:13:12 CEST
Fedora has issued an advisory on June 11:
https://lists.fedoraproject.org/archives/list/package-announce@lists.fedoraproject.org/thread/KS6UVEQTFR4NWHO6BVXCHZB2TFEJX7P3/

Mageia 5 is also affected.
Comment 1 Marja van Waes 2017-06-13 10:18:40 CEST
Assigning to all packagers collectively, since there is no registered maintainer for this package.
Comment 2 Nicolas Lécureuil 2017-06-13 11:05:55 CEST
Fixed on svn for cauldron by Shlomi Fish ( waiting for freeze push )
Comment 3 Nicolas Lécureuil 2017-06-13 15:28:50 CEST
pushed on cauldron
Comment 4 Shlomi Fish 2017-07-05 15:55:30 CEST
perltidy-20170521.0.0-1.mga5 	was submited to core 5 / updates_testing. Assigning to QA - please test.
Comment 5 Len Lawrence 2017-07-05 21:41:53 CEST
Quoting from the link above (comment 0):
> Perl::Tidy tries to delete existing perltidy.ERR; but if deleting fails, it
> proceeds as if nothing happened. This can be abused to overwrite arbitrary
> files via symlink attack:
In the absence of the advisory we should assume that this is what has been fixed by the patch?

If so then the rest of the discussion on that link can be used as a basis for testing its success or otherwise, probably.  Back to this later.
Comment 6 David Walser 2017-07-06 01:45:30 CEST
Advisory:
========================

Updated perltidy package fixes security vulnerability:

perltidy relies on the current working directory for certain output files and
does not have a symlink-attack protection mechanism, which allows local users
to overwrite arbitrary files by creating a symlink (CVE-2016-10374).

References:
http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2016-10374
https://lists.fedoraproject.org/archives/list/package-announce@lists.fedoraproject.org/thread/KS6UVEQTFR4NWHO6BVXCHZB2TFEJX7P3/
========================

Updated packages in core/updates_testing:
========================
perltidy-20170521.0.0-1.mga5

from perltidy-20170521.0.0-1.mga5.src.rpm
Comment 7 Len Lawrence 2017-07-06 02:37:06 CEST
x86_64 in Mate, mga5, real hardware.

First look at the suggested test, following the text at the link already quoted, more or less verbatim.

$ sudo urpmi perl-Perl-Critic
$ mkdir tmp-pc
$ cd tmp-pc
$ mkdir tmp

Created file tmp/foo.pl
------------------------
$foo = 'bar'
$bar = 'baz';
------------------------

$ perlcritic -1 .
perltidy had errors!! at line 1, column 1.  See page 33 of PBP.  (Severity: 1)
Module does not end with "1;" at line 1, column 1.  Must end with a recognizable true value.  (Severity: 4)
Code not contained in explicit package at line 1, column 1.  Violates encapsulation.  (Severity: 4)
No package-scoped "$VERSION" variable found at line 1, column 1.  See page 404 of PBP.  (Severity: 2)
Code before strictures are enabled at line 1, column 1.  See page 429 of PBP.  (Severity: 5)
Code before warnings are enabled at line 1, column 1.  See page 431 of PBP.  (Severity: 4)

Updated perltidy.

$ echo foo > perltidy.ERR
$ perlcritic -1 .
# Same output as before, then:
$ cat perltidy.ERR
Perltidy version is 20170521

2: $bar = 'baz';
   ^            
found Scalar where operator expected

Missing ';' above?

May have to revisit this.

Functionality test using examples at https://www.tjhsst.edu/~dhyatt/superap/perl/ex5.html

Used sub3 and removed the indentations -> untidyfile

$ perltidy -o tidyfile untidyfile
$ cat tidyfile
#!/bin/perl
# SUB3:   Demonstrates how to return a scalar from a subroutine call.  The
#         module modifies a subrange of @arr1, and returns number of changes.

sub sub3 {
    my ($diff);
    print "  In sub3: Parms = @_ \n";    # Print out parameter string
    print "  In sub3:  ";
    for ( $_[0] .. $_[1] )               # Step through subrange, one by one
    {
        print $_, ". ", $arr1[$_], "   ";  # Default variable is current counter

        $arr1[$_] = $arr1[$_] . "XX";      # Concatenate operator is "." (dot)
    }

    print "\n";
    print "  In sub3: @arr1 \n";

    $diff = $_[1] - $_[0] + 1;             # Determine number of items used

    return $diff;                          # Assign return value to subroutine
}
Comment 8 Len Lawrence 2017-07-06 02:40:03 CEST
Created attachment 9463 [details]
Example perl script with indentations removed.

$ perltidy -o tidyfile untidyfile
puts the indentations back.
Comment 9 Len Lawrence 2017-07-06 02:49:40 CEST
Looks like I missed the point of the discussion by not intializing perltidy.ERR before the pre-update test.  Shall revisit this on another system.
Comment 10 Len Lawrence 2017-07-06 11:56:49 CEST
Repeated the before-update test, ensuring that perltidy.ERR existed in the current directory, random content.
$ perlcritic -1 .
This analyses any perl file found in the current directory and below and reports a syntax error and records it in perltidy.ERR in the current directory, which is undesirable behaviour.

Repeating this after the update and using a dummy perltidy.ERR file the diagnosis is again written to the current directory.  Looks like the bug is not fixed.

If perltidy.ERR is deleted beforehand the result is the same.
Comment 11 Len Lawrence 2017-07-06 12:20:51 CEST
Checked this in a Cauldron virtualbox and the results were the same.

Perltidy version is 20170521
Comment 12 Shlomi Fish 2017-07-06 14:11:52 CEST
(In reply to Len Lawrence from comment #10)
> Repeated the before-update test, ensuring that perltidy.ERR existed in the
> current directory, random content.
> $ perlcritic -1 .
> This analyses any perl file found in the current directory and below and
> reports a syntax error and records it in perltidy.ERR in the current
> directory, which is undesirable behaviour.
> 
> Repeating this after the update and using a dummy perltidy.ERR file the
> diagnosis is again written to the current directory.  Looks like the bug is
> not fixed.
> 
> If perltidy.ERR is deleted beforehand the result is the same.

was perltidy.ERR a symlink to a different file beforehand?
Comment 13 Len Lawrence 2017-07-06 14:36:13 CEST
No, I was missing the plot again.  The perltidy.ERR file needs to be replaced by a symbolic link which the user cannot remove.  Have not found a way to do that.  

Linked perltidy.ERR to a copy of a file in another user's directory.  The link is replaced by the local diagnosis, thus breaking the link.
Comment 14 Herman Viaene 2017-07-20 12:25:54 CEST
MGA5-32 on Asus A6000VM Xfce
No installation issues.
Changed tests above to see what the difference is between perlcritic and perltidy with regard to the .ERR file.
I put the foo.pl file and the untidy file in an otherwise empty folder.
Run "perlcritic -1 ." thus against the two files, generates output to the CLI, none to perltidy.ERR. Even if this .ERR file existed before this command (cat foo to it in the same folder as the test files), it deletes it.
Run "perlcritic -1 foo.pl" gives error feedback at the CLI and fills in the perltidy.ERR file,creating it if needed, overwriting it if present.
Run "perltidy -o tidyfile untidyfile" creates the tidyfile as indicated above, and does nothing to the perltidy.ERR file. Leaving it as is, if it existed before, not creating it if it was not there.
Now whether that is intended behavior, is beyond me.
Comment 15 Len Lawrence 2017-07-25 21:00:21 CEST
Still trying to make sense of this.  I cannot remember where that reference to an undeleteable symlink came from.  What is certain is that the updated perlcritic is still overwriting a file in the current directory, even one with readonly permissions.  perltidy behaves as Herman says in comment 14.

If the intention of the update is to write diagnostic output to ~/tmp, ./tmp or /tmp then it looks like it fails and the update is not OK.

We could do with some clarification on this.

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