Problem/Motivation
Drupal core - Critical - Third Party Libraries - SA-CORE-2019-001 is throwing the below fatal error when calling \Drupal\Core\Archiver\ArchiveTar::addModify
Warning: pack(): Type a: not enough arguments in Drupal\Core\Archiver\ArchiveTar->_writeHeader() (line 1475 of core/lib/Drupal/Core/Archiver/ArchiveTar.php).
I was able to pinpoint the missing pack()
argument and core/lib/Drupal/Core/Archiver/ArchiveTar.php LINE 1469 is missing the $v_reduced_filename argument
The below code…
$v_binary_data_first = pack(
"a100a8a8a8a12a12",
$v_perms,
$v_uid,
$v_gid,
$v_size,
$v_mtime
);
…needs to include the $v_reduced_filename
parameter…
$v_binary_data_first = pack(
"a100a8a8a8a12a12",
$v_reduced_filename,
$v_perms,
$v_uid,
$v_gid,
$v_size,
$v_mtime
);
For reference here is the original code from PEAR Archive_Tar.
https://github.com/pear/Archive_Tar/blob/master/Archive/Tar.php#L1510
I discovered this typo because Webform is using ArchiveTar to compress exported files.
@see #3026465: Submission export with compressed archive is no longer working
Proposed resolution
Add missing $v_reduced_filename
argument to \Drupal\Core\Archiver\ArchiveTar::_writeHeader
Remaining tasks
Determine if fixing the typo is acceptable
Determine if we need to write test coverage. (NOTE: Archive_Tar has test coverage)
Backport to 8.5, and 8.6.
Decide when this fix can be released.
User interface changes
N/A
API changes
N/A
Data model changes
N/A
Release notes snippet
N/A
Comment | File | Size | Author |
---|---|---|---|
#12 | 3026470-12.patch | 2.27 KB | alexpott |
#12 | 11-12-interdiff.txt | 822 bytes | alexpott |
#11 | 3026470-11.patch | 1.71 KB | alexpott |
#11 | 2-11-interdiff.txt | 1.57 KB | alexpott |
#2 | 3026470-2.patch | 499 bytes | jrockowitz |
Comments
Comment #2
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedComment #3
pandaski CreditAttribution: pandaski at govCMS (Australian Government Department of Finance) commentedVerified the same issue as 8.7.x
Comment #4
pandaski CreditAttribution: pandaski at govCMS (Australian Government Department of Finance) commentedReference: https://api.drupal.org/api/drupal/modules%21system%21system.tar.inc/func...
Comment #5
xjmThanks for the great issue report! I am honestly not sure how that even happened.
We should probably check a diff of the whole commit to ensure it's identical to the upstream library -- if this hunk is messed up, maybe others are as well. Also, we should check our test coverage; I thought we had our own test coverage for the library. Setting NR for us to look into those two things before we commit.
Comment #6
alexpottThis is why we need an issue to move this dependency into composer and out of pseudo-dependency land.
Comment #7
alexpottOpened #3026588: Move ArchiveTar to composer
Comment #8
alexpottSo we have to match the diff of
git diff b3aef2bced...HEAD core/lib/Drupal/Core/Archiver/ArchiveTar.php
where HEAD is a branch with this patch applied to https://github.com/pear/Archive_Tar/compare/1.4.0...1.4.5Here's our diff
and here is the pear diff
git diff 1.4.0...1.4.5 Archive/Tar.php
Comment #9
xjm#8 isn't going to help in that form; I think we need to diff the diffs.
Comment #10
alexpottDoing a diff of those two diff using git diff ignore whitespace feature gives us
Let's look at the diffs:
$v_reduced_filename = $this->_pathReduction($p_stored_filename);
Here's core:
Here's pear:
So it looks as if pear has some added blank lines - let's add them.
$v_devminor = '';
More blank lines - fixed.
$v_data = unpack($this->_fmt, $v_binary_data);
More blank lines - fixed
return OctDec(trim($tar_size));
Less blank lines - fixed
_maliciousFilename
Here's cores
and here is pear's
So all I can think is git diff got a bit confused here.
Following the same process on 8.x HEAD I see:
Which now sticks out like a sore thumb.
Comment #11
alexpottHere's a patch based on making things the same. And a diff of the new diffs.
As before there's some oddness in _maliciousFilename() but I think that is git diff and so many similar lines.
Comment #12
alexpottMissed a couple more new lines - adding them in to make the diff of the diffs are small as possible.
For me this is now rtbc - we've proved that all the differences are between the Drupal changes and pear changes are due to Drupal now using the full Pear library and just this one class - that all happens in the constructor. The rest of the file - the changes are the same.
I have only added whitespace to the patch.
Comment #13
Ayesh CreditAttribution: Ayesh commentedI worked on the primary patch for the Archive_Tar update for the security release. It was a manual backport because Drupal core had some changes and even with minor hunk changes, an upstream patch didn't apply well. I apologize about the mistakes in it :(
I will double check the Drupal 7's port as well, but it was a direct patch from upstream so I'm believe this doesn't apply to 7.x.
Comment #14
xjmCongrats on your first experience breaking core HEAD, Ayesh. It's a rite of passage. :D
Leaving RTBC while tests finish running. If it turns out to need a fix in 7.x as well, we'll move the issue to "Patch (to be ported)" once we commit it to the 8.x branches.
Comment #18
xjmAlright, committed to 8.7.x, 8.6.x, and 8.5.x. This will be available in the next bugfix release on Feb. 6 if not sooner. In the meanwhile, sites affected can apply the above patch or use the next development tarball.
If it turns out to affect 7.x too we can reopen the issue with "Patch (to be ported)".
Thanks!
Comment #19
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commented@everyone Thanks for fixing and committing this.
Waiting to Feb 6 should be fine. This issue is documented in the Webform issue queue and people can apply the patch.
Comment #21
xjm