New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SharpCompress generates Zips that don't pass System.IO.Packaging validation #164

Closed
paulcbetts opened this Issue Aug 22, 2016 · 11 comments

Comments

Projects
None yet
3 participants
@paulcbetts

paulcbetts commented Aug 22, 2016

I'm working on switching Squirrel.Windows to SharpCompress in Squirrel/Squirrel.Windows#803, and one thing I'm seeing is that files compressed with SharpCompress and then later opened via NuGet's code (which uses WPF's System.IO.Packaging code) blow up here:

    WindowsBase.dll!MS.Internal.IO.Zip.ZipIOLocalFileBlock.Validate(string fileName, MS.Internal.IO.Zip.ZipIOCentralDirectoryBlock centralDir, MS.Internal.IO.Zip.ZipIOCentralDirectoryFileHeader centralDirFileHeader) Unknown
    WindowsBase.dll!MS.Internal.IO.Zip.ZipIOLocalFileBlock.ParseRecord(System.IO.BinaryReader reader, string fileName, long position, MS.Internal.IO.Zip.ZipIOCentralDirectoryBlock centralDir, MS.Internal.IO.Zip.ZipIOCentralDirectoryFileHeader centralDirFileHeader)    Unknown
    WindowsBase.dll!MS.Internal.IO.Zip.ZipIOLocalFileBlock.SeekableLoad(MS.Internal.IO.Zip.ZipIOBlockManager blockManager, string fileName) Unknown
    WindowsBase.dll!MS.Internal.IO.Zip.ZipIOBlockManager.LoadLocalFileBlock(string zipFileName) Unknown
    WindowsBase.dll!MS.Internal.IO.Zip.ZipArchive.GetFile(string zipFileName)   Unknown
    WindowsBase.dll!MS.Internal.IO.Zip.ZipArchive.GetFiles()    Unknown
    WindowsBase.dll!System.IO.Packaging.ZipPackage.ContentTypeHelper.ContentTypeHelper(MS.Internal.IO.Zip.ZipArchive zipArchive, System.IO.Packaging.ZipPackage.IgnoredItemHelper ignoredItemHelper)    Unknown
    WindowsBase.dll!System.IO.Packaging.ZipPackage.ZipPackage(System.IO.Stream s, System.IO.FileMode mode, System.IO.FileAccess access, bool streaming) Unknown
    WindowsBase.dll!System.IO.Packaging.Package.Open(System.IO.Stream stream, System.IO.FileMode packageMode, System.IO.FileAccess packageAccess, bool streaming)   Unknown
    WindowsBase.dll!System.IO.Packaging.Package.Open(System.IO.Stream stream)   Unknown
>   NuGet.Squirrel.dll!NuGet.ZipPackage.EnsureManifest() Line 142   C#
    NuGet.Squirrel.dll!NuGet.ZipPackage.ZipPackage(string filePath, bool enableCaching) Line 53 C#

http://referencesource.microsoft.com/#WindowsBase/Base/MS/Internal/IO/Zip/ZipIOLocalFileBlock.cs,788

        private void Validate(string fileName, 
            ZipIOCentralDirectoryBlock centralDir,
            ZipIOCentralDirectoryFileHeader centralDirFileHeader)
        {
            // check that name matches parameter in a case sensitive culture neutral way
            if (0 != String.CompareOrdinal(_localFileHeader.FileName, fileName))
            {
                throw new FileFormatException(SR.Get(SRID.CorruptedData));
            }

            // compare compressed and uncompressed sizes, crc from central directory 
            if ((VersionNeededToExtract != centralDirFileHeader.VersionNeededToExtract) ||
                (GeneralPurposeBitFlag != centralDirFileHeader.GeneralPurposeBitFlag) ||
                (CompressedSize != centralDirFileHeader.CompressedSize) ||
                (UncompressedSize != centralDirFileHeader.UncompressedSize) ||
                (CompressionMethod != centralDirFileHeader.CompressionMethod) ||
                (Crc32 != centralDirFileHeader.Crc32))
            {
                throw new FileFormatException(SR.Get(SRID.CorruptedData));
            }

            // check for read into central directory (which would indicate file corruption)
            if (Offset + Size > centralDir.Offset)
                throw new FileFormatException(SR.Get(SRID.CorruptedData));

            // No CRC check here
            // delay validating the actual CRC until it is possible to do so without additional read operations
            // This is only for non-streaming mode (at this point we only support creation not consumption)
            // This is to avoid the forced reading of entire stream just for CRC check
            // CRC check is delegated  to ProgressiveCrcCalculatingStream and CRC is only validated
            //  once calculated CRC is available. This implies that CRC check operation is not
            //  guaranteed to be performed
        }

If you want a live repro, clone that PR and run the CreateFullPackagesFromDeltaSmokeTest test, or if you want a sample file to check out, https://cl.ly/0W3D3V1x0V2J

@adamhathcock

This comment has been minimized.

Show comment
Hide comment
@adamhathcock

adamhathcock Aug 23, 2016

Owner

Did you create the zip file with ZipWriter? It might not like the streaming mode entries as there's a trailing header behind the file bytes to record the actual size and whatnot.

Knowing which way you created the file will help the most. It could be a bug in SharpCompress or it could be that the packaging API doesn't like streaming created zip files that are to spec :) Does something like WinZip or WinRar open the file?

I'm about to go on vacation for a week so I'm not sure when I'll get to take a look.

Owner

adamhathcock commented Aug 23, 2016

Did you create the zip file with ZipWriter? It might not like the streaming mode entries as there's a trailing header behind the file bytes to record the actual size and whatnot.

Knowing which way you created the file will help the most. It could be a bug in SharpCompress or it could be that the packaging API doesn't like streaming created zip files that are to spec :) Does something like WinZip or WinRar open the file?

I'm about to go on vacation for a week so I'm not sure when I'll get to take a look.

@paulcbetts

This comment has been minimized.

Show comment
Hide comment
@paulcbetts

paulcbetts commented Aug 23, 2016

Usually something like https://github.com/Squirrel/Squirrel.Windows/pull/803/files#diff-dc4101658eac6c569618cb80ac0bffccR99. 7-Zip seems to have no problem opening the file

@adamhathcock

This comment has been minimized.

Show comment
Hide comment
@adamhathcock

adamhathcock Aug 23, 2016

Owner

Cool. I'll try to setup a test Tuesday when I'm back in front of a machine.

Owner

adamhathcock commented Aug 23, 2016

Cool. I'll try to setup a test Tuesday when I'm back in front of a machine.

@adamhathcock

This comment has been minimized.

Show comment
Hide comment
@adamhathcock

adamhathcock Sep 27, 2016

Owner

Coming up with nothing. Then again, I don't think I know how to properly use the Packaging API.

Owner

adamhathcock commented Sep 27, 2016

Coming up with nothing. Then again, I don't think I know how to properly use the Packaging API.

@paulcbetts

This comment has been minimized.

Show comment
Hide comment
@paulcbetts

paulcbetts Sep 27, 2016

@adamhathcock You and me both fam. An easier test if you're playing around with it is to use the NuGet library, the ZipPackage class there is super straightforward and hits this bug

paulcbetts commented Sep 27, 2016

@adamhathcock You and me both fam. An easier test if you're playing around with it is to use the NuGet library, the ZipPackage class there is super straightforward and hits this bug

@adamhathcock

This comment has been minimized.

Show comment
Hide comment
@adamhathcock

adamhathcock Sep 28, 2016

Owner

@paulcbetts I must be thick because I just downloaded the Squirrel.Core.1.1.0.0.nupkg and can enumerate the parts

 using (Stream stream = File.Open("Squirrel.Core.1.1.0.0.nupkg", FileMode.Open, FileAccess.ReadWrite))
            {
                var package = ZipPackage.Open(stream);
                foreach (var part in package.GetParts())
                {
                    Console.WriteLine(part.Uri);
                }
            }
Owner

adamhathcock commented Sep 28, 2016

@paulcbetts I must be thick because I just downloaded the Squirrel.Core.1.1.0.0.nupkg and can enumerate the parts

 using (Stream stream = File.Open("Squirrel.Core.1.1.0.0.nupkg", FileMode.Open, FileAccess.ReadWrite))
            {
                var package = ZipPackage.Open(stream);
                foreach (var part in package.GetParts())
                {
                    Console.WriteLine(part.Uri);
                }
            }
@paulcbetts

This comment has been minimized.

Show comment
Hide comment
@paulcbetts

paulcbetts Oct 21, 2016

@adamhathcock Lemme try it again with the latest SharpCompress

paulcbetts commented Oct 21, 2016

@adamhathcock Lemme try it again with the latest SharpCompress

@damieng

This comment has been minimized.

Show comment
Hide comment
@damieng

damieng May 19, 2017

Contributor

So I've found what is causing this - it's the check to make sure the version requirement from the Central Directory File Header matches the version requirement of the ZIP Local File Header contained within it.

Unfortunately for SharpCompress generated files there is a mismatch.

ZIP central directory file header is set to 10 at https://github.com/adamhathcock/sharpcompress/blob/master/src/SharpCompress/Writers/Zip/ZipCentralDirectoryEntry.cs#L33

ZIP local file header is set to 20 at https://github.com/adamhathcock/sharpcompress/blob/master/src/SharpCompress/Writers/Zip/ZipWriter.cs#L162

I suspect we can probably just change the 20 to a 10 in the ZipWriter? I can send a PR with the fix and a test if this is the case. Alternatively I guess we could change the central one to 20 but that might reduce compatibility. I guess a lot of tools outside of Microsoft's Packaging are only using one of the two.

Contributor

damieng commented May 19, 2017

So I've found what is causing this - it's the check to make sure the version requirement from the Central Directory File Header matches the version requirement of the ZIP Local File Header contained within it.

Unfortunately for SharpCompress generated files there is a mismatch.

ZIP central directory file header is set to 10 at https://github.com/adamhathcock/sharpcompress/blob/master/src/SharpCompress/Writers/Zip/ZipCentralDirectoryEntry.cs#L33

ZIP local file header is set to 20 at https://github.com/adamhathcock/sharpcompress/blob/master/src/SharpCompress/Writers/Zip/ZipWriter.cs#L162

I suspect we can probably just change the 20 to a 10 in the ZipWriter? I can send a PR with the fix and a test if this is the case. Alternatively I guess we could change the central one to 20 but that might reduce compatibility. I guess a lot of tools outside of Microsoft's Packaging are only using one of the two.

@adamhathcock

This comment has been minimized.

Show comment
Hide comment
@adamhathcock

adamhathcock May 19, 2017

Owner

That's definitely a problem.

Quickly reviewing APPNOTE.txt again I should be dynamically changing the version needed to extract based on features.

Changing to 10 is probably the benign thing to do for now. I'll investigate more by looking at other implementations next week.

Owner

adamhathcock commented May 19, 2017

That's definitely a problem.

Quickly reviewing APPNOTE.txt again I should be dynamically changing the version needed to extract based on features.

Changing to 10 is probably the benign thing to do for now. I'll investigate more by looking at other implementations next week.

@adamhathcock

This comment has been minimized.

Show comment
Hide comment
@adamhathcock

adamhathcock May 19, 2017

Owner

Maybe it's just the fact that it's mismatched is the problem. Otherwise I would have said 20 is the proper thing for now.

Owner

adamhathcock commented May 19, 2017

Maybe it's just the fact that it's mismatched is the problem. Otherwise I would have said 20 is the proper thing for now.

@damieng

This comment has been minimized.

Show comment
Hide comment
@damieng

damieng May 19, 2017

Contributor

It looks like Deflate is only supported in version '20' so I think you're right - changing the ZIP central dictory file header to '20' would be the smallest fix.

If there was a need to support '10' then it would need to also not use deflate or encryption.

Contributor

damieng commented May 19, 2017

It looks like Deflate is only supported in version '20' so I think you're right - changing the ZIP central dictory file header to '20' would be the smallest fix.

If there was a need to support '10' then it would need to also not use deflate or encryption.

adamhathcock added a commit that referenced this issue May 22, 2017

Merge pull request #236 from damieng/zip-min-version-of-20
Default zip ver to 20 (deflate/encyption), fixes #164
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment