Bug 21079 - perltidy new security issue CVE-2016-10374
Summary: perltidy new security issue CVE-2016-10374
Status: RESOLVED FIXED
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: MGA5-64-OK MGA5-32-OK advisory
Keywords: validated_update
Depends on:
Blocks:
 
Reported: 2017-06-12 12:13 CEST by David Walser
Modified: 2017-08-24 23:19 CEST (History)
7 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.
David Walser 2017-06-12 12:13:27 CEST

CC: (none) => shlomif
Whiteboard: (none) => MGA5TOO

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.

Assignee: bugsquad => pkg-bugs
CC: (none) => marja11

Nicolas Lécureuil 2017-06-13 11:05:29 CEST

CC: (none) => mageia
Assignee: pkg-bugs => shlomif

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

Version: Cauldron => 5
Whiteboard: MGA5TOO => (none)

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.

Status: NEW => ASSIGNED
Assignee: shlomif => qa-bugs

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.

CC: (none) => tarazed25

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.
Len Lawrence 2017-07-06 11:57:48 CEST

Keywords: (none) => NEEDINFO

David Walser 2017-07-06 12:06:10 CEST

Keywords: NEEDINFO => (none)
Whiteboard: (none) => feedback

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?
David Walser 2017-07-06 14:14:19 CEST

Whiteboard: feedback => (none)

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.

CC: (none) => herman.viaene

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.
Len Lawrence 2017-07-25 21:00:40 CEST

Whiteboard: (none) => feedback

Comment 16 David Walser 2017-08-22 18:54:06 CEST
Shlomi, is there any guidance you can give here?
Comment 17 Shlomi Fish 2017-08-22 20:46:37 CEST
(In reply to David Walser from comment #16)
> Shlomi, is there any guidance you can give here?

Perhaps all we can do is file a bug upstream. Perhaps provide a reproducible testcase as a shell and/or perl program.
Comment 18 David Walser 2017-08-22 21:33:41 CEST
Thanks Shlomi.  Over to you Len.

Whiteboard: feedback => (none)

Comment 19 Len Lawrence 2017-08-23 17:00:53 CEST
This still bemuses me.  No idea how to reproduce the symlink attack which was addressed by Debian I think.  See this extract from the perltidy change log:

Fixed debian #862667: failure to check for perltidy.ERR deletion can lead 
    to overwriting abritrary files by symlink attack. Perltidy was continuing 
    to write files after an unlink failure.  Thanks to Don Armstrong 
    for a patch.

That would be the fix that I don't know how to test.

RedHat Bug 1452050 - CVE-2016-10374
Status: CLOSED WONTFIX

This seems to indicate that the issue of writing the error file to the current directory is not important and from the above not actually relevant to this bug.

So, as in many other cases we have to assume that the patch is effective and give an OK on the strength of that.  Holding off in case somebody objects.
Len Lawrence 2017-08-24 10:49:11 CEST

Whiteboard: (none) => MGA5-64-OK

Comment 20 Len Lawrence 2017-08-24 10:51:28 CEST
@Herman - comment 14.
Are you happy to add the 32-bit OK?
Comment 21 Herman Viaene 2017-08-24 19:38:31 CEST
Doing so.

Whiteboard: MGA5-64-OK => MGA5-64-OK MGA5-32-OK

Lewis Smith 2017-08-24 22:21:47 CEST

Keywords: (none) => validated_update
Whiteboard: MGA5-64-OK MGA5-32-OK => MGA5-64-OK MGA5-32-OK advisory
CC: (none) => lewyssmith, sysadmin-bugs

Comment 22 Mageia Robot 2017-08-24 23:19:16 CEST
An update for this issue has been pushed to the Mageia Updates repository.

http://advisories.mageia.org/MGASA-2017-0301.html

Status: ASSIGNED => RESOLVED
Resolution: (none) => FIXED


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