[wp-trac] [WordPress Trac] #42016: Validation of filenames (while unzipping) causes unexpected failures

[WordPress Trac]

#42016: Validation of filenames (while unzipping) causes unexpected failures
----------------------------+-----------------------
 Reporter:  Ipstenu         |       Owner:
     Type:  defect (bug)    |      Status:  reopened
 Priority:  normal          |   Milestone:  4.8.3
Component:  Filesystem API  |     Version:  4.8.2
 Severity:  normal          |  Resolution:
 Keywords:  has-patch       |     Focuses:
----------------------------+-----------------------

Comment (by birgire):

 The {{{validate_file()}}} does currently not have a test, so I think we
 need one here.

 [attachment:42016.tests.diff] contains tests for:

 {{{
 status file-path allowed-files
 0 null
 0 ''
 0 ' '
 0 '.'
 0 '..'
 0 './'
 0 'foo.ext' ( 'foo.ext' )
 0 'foo..ext'
 1 '../'
 2 'c:'
 3 'foo.ext' ( 'bar.ext' )

 }}}

 It would be helpful to expand this list for e.g.


 {{{
 '../../'
 '../../.'
 '../foo.ext'
 '../../foo.ext'
 '../../'
 '../../.'
 '../foo.ext'
 '../../foo.ext'
 '..\'

 }}}

 etc so we better understand the modified {{{validate_file()}}}.

 @DavidAnderson thanks for the patches.

 I wonder if {{{strpos()}}} and {{{substr_count()}}} could be used instead
 of {{{preg_match}}} and {{{count($matches)}}} in

 {{{
 if ( preg_match( '#\.\./#', $file, $matches ) && ( count( $matches ) > 1
 || '../' != substr( $file, -3, 3 ) ) )

 }}}

 I'm not sure the about the {{{count( $matches ) > 1}}} check as

 {{{
 $file = '../../../';
 preg_match( '#\.\./#', $file, $matches );
 echo count( $matches );

 }}}

 outputs 1.

 I also wonder about using {{{mb_substr()}}} instead of {{{substr()}}} and
 adding a true in the {{{in_array()}}} check?

 Ps: it might be helpful to look at

 https://github.com/fuzzdb-project/fuzzdb/blob/master/attack/path-
 traversal/traversals-8-deep-exotic-encoding.txt

 https://github.com/danielmiessler/SecLists/blob/master/Fuzzing/FUZZDB_WindowsAattacks.txt

--
Ticket URL: <https://core.trac.wordpress.org/ticket/42016#comment:12>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform