Skip to content
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

Zip Slip Vulnerability in FlightCrew #52

Closed
mssalvatore opened this issue Jun 26, 2019 · 26 comments
Closed

Zip Slip Vulnerability in FlightCrew #52

mssalvatore opened this issue Jun 26, 2019 · 26 comments

Comments

@mssalvatore
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

kevinhendricks commented Jun 26, 2019 via email

@kevinhendricks
Copy link
Contributor

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
Copy link
Contributor

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

@kevinhendricks
Copy link
Contributor

ps, used a similar fix in Sigil master.

@mssalvatore
Copy link
Contributor Author

@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
Copy link
Contributor

kevinhendricks commented Jun 27, 2019 via email

@kevinhendricks
Copy link
Contributor

kevinhendricks commented Jun 27, 2019 via email

mssalvatore added a commit to mssalvatore/flightcrew that referenced this issue Jun 27, 2019
@mssalvatore
Copy link
Contributor Author

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 Media-type for TrueType fonts? #2 but not question SVG not allowed in XHTML? #1.

@kevinhendricks
Copy link
Contributor

kevinhendricks commented Jun 27, 2019

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../

  1. 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
Copy link
Contributor

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
Copy link
Contributor Author

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

@kevinhendricks
Copy link
Contributor

Thanks for filing this issue and helping to improve flightcrew.

@nluedtke
Copy link

nluedtke commented Jul 5, 2019

This issue was assigned CVE-2019-13241.

@dougmassay
Copy link
Contributor

This issue was assigned CVE-2019-13241.

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

@kevinhendricks
Copy link
Contributor

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

@mssalvatore
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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

@nluedtke
Copy link

nluedtke commented Jul 6, 2019

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
Copy link
Contributor Author

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
Copy link
Contributor

@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
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants