Zip Slip Vulnerability in FlightCrew

[mssalvatore]

Summary FlightCrew v0.9.2 and older are vulnerable to a directory traversal, allowing attackers to write arbitrary files via a ../ (dot dot slash) in a Zip archive entry that is mishandled during extraction. This vulnerability is also known as 'Zip Slip'.

Impact This vulnerability can be used to write files to arbitrary locations and could potentially result in granting an attacker remote access or arbitrary code execution.

This is a medium severity issue for Sigil users, but may have greater impact on third-party software that uses FlightCrew as a library.

Steps to Reproduce 1) Download the attached "zip-slip.zip"

2) On a linux system, process the epub using flightcrew-cli. flightcrew-cli --input-file zip-slip.zip

3) Check for the existence of "/tmp/evil.txt" with the contents "this is an evil one".

Futher Reading For more information on zip-slip vulnerabilities, see https://snyk.io/research/zip-slip-vulnerability

zip-slip.zip

[kevinhendricks]

Luckily, the plugin version of Flightcrew that we currently use does not unzip anything as that is handled inside Sigil itself. We will have to change to a newer version of zipios++ or remove all ../ from any file path in the central directory or in the zip itself.

Thank you for your bug report.

[mssalvatore]

It looks like Sigil v0.9.9 is also vulnerable, but I'd have to investigate a little more thoroughly. I can open another github issue in that project if I find it to be vulnerable.

[kevinhendricks]

Hi Mike,

We use minizip in Sigil not zipios but I will take a look. Luckily epubs are not general zip archives that might be used for backup so removing any and all relative up path segments is entirely legal.

I will postprocess the call to getName in the ExtractEpub routines to prevent the problem from hurting anyone.

If need be I will do the same to Sigil's minizip implementation. I assume Python's 3.7.2 Zipfile py module has already been hardened.

Kevin

On Jun 26, 2019, at 1:20 PM, Mike Salvatore notifications@github.com wrote:

It looks like Sigil v0.9.9 is also vulnerable, but I'd have to investigate a little more thoroughly. I can open another github issue in that project if I find it to be vulnerable.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

[kevinhendricks]

Addressed this issue (hopefully) with my latest commit. Please let me know if you have concerns about this approach. The is is to force extraction inside of destination no matter what.

[kevinhendricks]

Closing this issue as fixed in master. Please feel free to reopen it if you have any further concerns.

[kevinhendricks]

ps, used a similar fix in Sigil master.

[mssalvatore]

@kevinhendricks, I'd have to build a proof of concept to see whether or not this works from within a zip file, but "\.\./" resolves to "../" when I run it on my terminal:

$ ~/Downloads/test> cd \.\./
$ ~/Downloads>

.\./, \../, ..\/ also behave the same way.

Hypothetically, if the zip file path contained "\.\./", your string search wouldn't match and FlightCrew and Sigil could still be vulnerable.

I think a safer solution to prevent edge cases like this might be to resolve the canonical path of the file and verify that it is within path_to_folder. There are lots of examples of how other projects have fixed this issue here: https://github.com/snyk/zip-slip-vulnerability .

[kevinhendricks]

I will test those cases myself. I really do not want to link in an absolute path resolution routine here that depends on the filesystem being used. I can see where something like that would be needed for a real zip which might actually have relative paths via symlinks and things but none of that is allowed in an epub.

Removing upward path segments by walking the file path is easier I think and relies on no path library or filesystem structure. Even just replacing all "." with just "." and "\/" with "/" before the final replacement would catch all of these cases and string replacement for strings of max path length is cheap and fast and requires no external routines.

Thanks,

Kevin

On Jun 26, 2019, at 9:28 PM, Mike Salvatore notifications@github.com wrote:

@kevinhendricks, I'd have to build a proof of concept to see whether or not this works from within a zip file, but "../" resolves to "../" when I run it on my terminal:

$ ~/Downloads/test> cd ../ $ ~/Downloads> ../, ../, ..\/ also behave the same way.

Hypothetically, if the zip file path contained "../", your string search wouldn't match and FlightCrew and Sigil could still be vulnerable.

I think a safer solution to prevent edge cases like this might be to resolve the canonical path of the file and verify that it is within path_to_folder. There are lots of examples of how other projects have fixed this issue here: https://github.com/snyk/zip-slip-vulnerability .

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[kevinhendricks]

Hi Mike,

I did a little reading on the Zip file standard, here is the important part:

see https://pkware.cachefly.net/webdocs/casestudies/APPNOTE.TXT

4.4.17 file name: (Variable) 4.4.17.1 The name of the file, with optional relative path. The path stored MUST NOT contain a drive or device letter, or a leading slash. All slashes MUST be forward slashes '/' as opposed to backwards slashes '\' for compatibility with Amiga and UNIX file systems etc. If input came from standard input, there is no file name field.

So I can safely remove all backwards slashes from the zip local name, preventing all of those corner cases. That also prevents all escape sequence to represent hex or unicode chars as well. That will convert all your test cases to ../ and I should be good to go to force all final paths to be inside the target directory.

Take care,

Kevin

On Jun 26, 2019, at 9:28 PM, Mike Salvatore notifications@github.com wrote:

@kevinhendricks, I'd have to build a proof of concept to see whether or not this works from within a zip file, but "../" resolves to "../" when I run it on my terminal:

$ ~/Downloads/test> cd ../ $ ~/Downloads> ../, ../, ..\/ also behave the same way.

Hypothetically, if the zip file path contained "../", your string search wouldn't match and FlightCrew and Sigil could still be vulnerable.

I think a safer solution to prevent edge cases like this might be to resolve the canonical path of the file and verify that it is within path_to_folder. There are lots of examples of how other projects have fixed this issue here: https://github.com/snyk/zip-slip-vulnerability .

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[mssalvatore]

Hey Kevin,

I have a few questions about the latest patch set.

1) I believe FlightCrew can run on Windows (based on the INSTALL.txt). As the backslash is the windows directory separator, will automatically stripping out backslashes have unintended consequences? I'm not familiar with how EPUBs are handled cross platform, so this may be a non-issue.

2) I believe searching and replacing "/../" could still leave you with a directory traversal. If the zip contains a file name "../a../FILE", the current algorithm will prepend a '/', giving "/../a../FILE". It will then replace the first "/../" leaving "a../FILE". Then the first character will be erased because the code has assumed it is a '/'. While this significantly limits the security risk, I don't think that it's the desired behavior.

3) Given that once evil_or_corrupt_epub is set to "true" an exception will ultimately be thrown, I believe the commit on my fork to be logically equivalent to what's there without the need for the extra processing. Please let me know what you think. If so, this resolves question #2 but not question #1.

[kevinhendricks]

Mike,

1. If you read the spec I posted "\" chars are NOT allowed in local name entries in zip archives. So this is not a Windows vs Linux/Mac issue.

2. No, please see the spec on std:string erase and find_first_not_of. The find_first_not_of has been used for years to implement fancy ltrim functions.

#include <string>
#include <iostream>

int main(int argc, char** argv)
{
    std::string str1 = "a../";
    std::string str2 = "//////a../"; 
    std::cout << str1 << "  converts to " << str1.erase(0,str1.find_first_not_of("/"));
    std::cout << "\n";
    std::cout << str2 << "  converts to " << str2.erase(0,str2.find_first_not_of("/"));
    std::cout << "\n";
    return 0;
}

Desktop kbhend$ clang++ -O -omain main.cpp
Kevins-MacBook-Pro:Desktop kbhend$ ./main
a../  converts to a../
//////a../  converts to a../

3. Given it could be used a library it could be caught and ignored. It is still safer to then still prevent any damage from being done.

[kevinhendricks]

Took a look at what you have in your fork but looking for "../" is not correct as someone already pointed out. That is why I changed it this morning to what I have now here. I am pretty confident it is reasonably secure in that no escape sequences can be used to create corner cases and prepending a / and then replacing all /../ with just a slash should prevent any upward movement and finally stripping any leading / (it is not just erasing the first char) should do the trick. I ran new cli version on my test set of epubs (see MobileRead's epubs in the public domain) and they all ran as expected, so nothing here should cause issue for normal epubs. It of course detects your zip-slip testcase and properly reports it via an exception.

So I think flightcrew should be good to go.

A similar fix (but much easier using QStrings) in Sigil has been pushed to master there as well.

[mssalvatore]

Ah, ok. I was misinterpreting the behavior of std::string.erase(0,0). I don't see a way around it. Thanks!

[kevinhendricks]

Thanks for filing this issue and helping to improve flightcrew.

[nluedtke]

This issue was assigned CVE-2019-13241.

[dougmassay]

This issue was assigned CVE-2019-13241.

And? The issue has been resolved. So why a new listing?

[kevinhendricks]

Yes, why does your CVE say awaiting analysis when this bug was already fixed. Please update that CVE as closed and fixed.

[mssalvatore]

Analysis is performed by NVD staff, not whomever files the CVE.

CVE has been marked for Analysis. Normally once in this state the CVE will be analyzed by NVD staff within 24 hours. https://nvd.nist.gov/vuln

[dougmassay]

Analysis is performed by NVD staff, not whomever files the CVE.

Yes, but there's no point in analyzing v0.9.2 and earlier (as the CVE indicates). It's already been determined that those versions are vulnerable--and we can't unring that particular bell even if we wanted to. Analyze the current codebase.

Analyzing an older vulnerability that has already been acknowledged (and subsequently fixed) is a pointless waste of resources. Analyze/test the new code if anything. I really hate the inept vindictiveness of the internet.

@kevinhendricks : what say we just scrap this repository altogether and create a new one based on the latest fixed code for flightcrew_plugin only. That's all we need for Sigil anyway.

[kevinhendricks]

Given flightcrew cli was never meant for production use on a website server or even a library, and no one has ever indicated to us they have used it in this way, that is probably a good idea as we would be constantly supporting it for things we no longer use in Sigil itself.

All of this attention for something that could only ever unpack files into places the user has write permission to anyway, and given that flightcrew in plugin form is used only when an epub has already been unzipped, gives us even more reason to reduce it to only a Sigil plugin.

When we add in the high overall age of the flightcrew codebase (Sigil itself dropped using boost, and Xerces long ago) plus flightcrew only supporting epub2, and flightcrew having its functionality basically replaced by epubcheck which can handle both epub2 and epub3, it does seem the right thing to do.

So I am okay with killing flightcrew, and putting it back in stripped down form, removing all use of zipios, and even boost, and Qt, and just making it a Sigil plugin. It will take some work, but it will prevent longterm headaches.

That said, given how important epubcheck passage is to professional epub production, perhaps we should just kill flightcrew completely and not replace it as it is just upkeep work the two of us as do not need.

What about just killing it completely? That would be the easiest. If we still want an internal check feature, we could probably put one together in python (ala calibre's check) that would be a lot easier to maintain and just include it in our Sigil codebase.

I think my vote would be to kill it completely, recommend the epubcheck plugin for Sigil, and then extend our simple well-formed check in Sigil with some additional python code that will catch glaring issues.

[dougmassay]

I'm fine with killing it completely. Let's give it a bit of a cooling-off period, though, in case we've overlooked something.

[kevinhendricks]

Sounds good to me. Hacking away Xerces, zipios, boost, and Qt use would be a major project, but doable.

[nluedtke]

This issue was assigned CVE-2019-13241.

And? The issue has been resolved. So why a new listing?

Not sure, I was just linking here for reference. It is unclear who requested it. (A flaw in the process...)

[mssalvatore]

Not sure, I was just linking here for reference. It is unclear who requested it. (A flaw in the process...)

I requested it.

And? The issue has been resolved. So why a new listing?

The purpose of CVE is described at https://cve.mitre.org/about/index.html. In summary, it is a list of publicly known vulnerabilities, both past and present.

Many security tools and processes rely on CVE. For example, Ubuntu (and other linux distributions) rely on CVE to know what software needs to be patched and redistributed to users. Now that a CVE has been assigned, these distributions will take action to either backport the fixes or upgrade the packaged version to the latest fixed version of FlightCrew. Without a CVE, this issue could fly under the radar and the packaged version of FlightCrew may not be fixed. In other words, even though FlightCrew is fixed in this repository, many instances of FlightCrew installed by users may remain vulnerable without the "new listing."

I really hate the inept vindictiveness of the internet.

I'm not sure why you feel this process is vindictive. Sorry for the trouble.

[kevinhendricks]

@mssalvatore Are you aware of anyone actually using flightcrew aside from its use as a Sigil plugin (where it does not unzip anything). Was this bug issued because of an actual failure in the field? If so, I would love to know if it is actually used as a library or cli form, or as part of a website service, or part of a production chain, etc. As far as we can tell by activity in bug reports and pulls (you are our only recent activity), we are flightcrew's only users (Sigil). Any info along that front will help us make a decision regarding either rewriting it as a plugin only vs killing the project entirely, vs trying to keep it as is. Rewriting it as a plugin only greatly reduces exposure and risk, but might not be worth the time. Keeping it as is (library and cli) actually increases risk and exposure and work for us unless someone is actually using it in the form.

[mssalvatore]

Are you aware of anyone actually using flightcrew aside from its use as a Sigil plugin

I did a reverse dependency search in Ubuntu and it looks like check-all-the-things [1] [2] relies on FlightCrew for validating EPUBs.

Was this bug issued because of an actual failure in the field?

No. I chose to investigate FlightCrew for security bugs more or less at random.

Any info along that front will help us make a decision

In a few weeks I may be able to give you an indication of how many Ubuntu users have installed FlightCrew, but not necessarily what they're using it for. At the moment, I don't have that data and I don't know when it will be made available to me.