Squirrel / Squirrel.Windows

An installation and update framework for Windows desktop apps
MIT License
7.35k stars 1.03k forks source link

Zip Slip Vulnerability - Update to patched SharpCompress #1393

Closed glen-nicol closed 5 years ago

glen-nicol commented 5 years ago

The SharpCompress library was vulnerable to the Zip Slip vulnerability until version 0.21.0 and this project currently uses 0.17.1.

I have seen the previous issues about update servers being owned and the stance on HTTPS being the boundary for security for this library but I would still recommend it be updated to be safe for enterprise users that may be using network drives to host their squirrel releases.

anaisbetts commented 5 years ago

I think this is important but we've had a lot of problems upgrading SharpCompress in the past because of its Disagreement with NuGet for what Zip files are shaped like, and despite it being a clever way to drop binaries, it doesn't actually constitute a security boundary here - if you can replace a zip file that we're extracting, you already can execute arbitrary code, simply by letting yourself be installed!

glen-nicol commented 5 years ago

I think that is mostly true. I suppose the danger comes with its ability hide easier by escaping the directory and impersonating a higher profile or more used application.

Ex: John Doe software's update server gets owned and then attackers use that to put some nasty javascript into the VS code directory. This could bypass a antivirus check that may get triggered from a different exe being deployed by squirrel. It also sticks around long after John Doe's software has been uninstalled.

I suppose a good AV would catch the new JS file. Maybe not.

anaisbetts commented 5 years ago

It's still worth looking into, thanks for the catch. For at least some of these, I believe we're manually extracting the Zip file or running through the contents - maybe we can patch this in Squirrel instead?

glen-nicol commented 5 years ago

I only looked at one place where it was used. If that is the case for all of them then great!

PhilWeisz commented 11 months ago

OK, it's 2023.. and the vulnerability is still around.

Squirrel is widely used. Not updating obviously vulnerable parts of any Open Source software is a very bad practice, especially when it's being used by many.

You have not acted/reacted in this particular case for a very long time now - despite it being overdue and instead, you actually closed this issue.

Regarding @anaisbetts's answer:

NuGet's policy and SharpCompress

.. problems .. [with] SharpCompress .. because of its Disagreement with NuGet for what Zip files are shaped like

It's you who didn't update SharpCompress to the newest version or at least to any patched version starting from v0.29 (which was freed from any known vulnerabilities) and likewise it is also you who is responsible for these security vulnerabilities from that very moment on.

What are the specific conflict issues between the Zip-file-anatomy that SharpCompress uses and NuGet's Zip-file-policy related to in particular?

Our perception is the following:

NuGet relies on the .NET ZipArchive class for Zip file handling, which generally adheres to the PKZIP specification. Additionally NuGet has the following requirements:

1.Magic Numbers: Starts with 'PK' (0x50, 0x4B). 2.Central Directory: Lists all files, must be present. 3.File Headers: Each compressed file has its own local header. 4.End of Central Directory Record: Must exist, marks archive end. 5.Compression Methods: Typically "Stored" (no compression) or "Deflate". 6.Version: Compatible with Zip specification versions that .NET supports.

Where among these requirements is the conflict located at? (Or did I miss any crucial parts of their requirement-specification?)

"The vulnerability is not an issue, because..."

and despite it being a clever way to drop binaries, it doesn't actually constitute a security boundary here - if you can replace a zip file that we're extracting, you already can execute arbitrary code, simply by letting yourself be installed!

You have to see it more globally than that. An actor with malicious intent, who manages to affect the releases of a Project P that has yours as a dep not only needs fewer steps to prepare his (actual) attack(s) in total but also gains many opportunities by vulnerable files originating from trusted origins. Because you deliver him an extra attack-surface to circumvent AV-detection. Even if you deleted or delete your installer/compression libraries after the installation is done - he has enough time to use your binary before or during installation. It doesn't address the actual problem - spreading files on many clients which are accepted by AV-systems and that can furthermore serve the goal of circumventing it.

You solve some of the major difficulties for such a person: Finding a way for introducing abuseable code that,..

The combination and sequential application of multiple vulnerabilities increases the different options for the attacker and often enables them to re-use these for further attacks when or because these weren't fixed. In case of a single-person- or single-organization-target - in the vast majority of cases - nobody even perceives which software/s was/were used/abused to conduct or initialize an attack.

When you have issues upgrading SharpCompress:

we've had a lot of problems upgrading SharpCompress in the past because.. [of NuGet's Disagreement].. for what [SharpCompress's] Zip files are shaped like

Why does it have to be SharpCompress? There are numerous different Open Source libraries addressing compression.

With respect for the operating systems your deployments are targeting in mind (Win/iOS/Mac): A) libs using the C/C++ programming language: zlib, libzip, 7-Zip/LZMA SDK B) libs only using C#: SharpZipLib, DotNetZip, ZipArchive (.NET), Ionic.Zip C) On top of that, every operating system vendor ships its own compression libraries, you could just write an adapter that combines those and use pre-processor-directives to select the actual one for compiling the release on that particular platform.

Also, I am pretty sure nobody would mind any potential size- or decompression-time-increases since the difference is mostly neglectable - unless you teach me different.


Your subsequent actions in the past ~5 years did not reflect the essence of your words when you stated:

I think this is important ..

I recommend re-addressing this issue in order to prevent getting flagged as a potentially unwanted program.

Since currently, it's not possible to differ between:

Thank you.

anaisbetts commented 11 months ago

@PhilWeisz I disagree, and despite you writing a wall of text, you have still not demonstrated how this security vulnerability has any tangible advantage or increased scope.

Again - if an attacker controls the Zip file, they have already won. There is zero reason to use clever Zip parsing vulnerabilities!

Please read https://devblogs.microsoft.com/oldnewthing/20060508-22/?p=31283, and if you can demonstrate how this is an actual security issue, with an actual security boundary being passed, please do comment.