TeamAmaze / AmazeFileManager

Material design file manager for Android
https://teamamaze.xyz
GNU General Public License v3.0
5.1k stars 1.54k forks source link

Fix possibility of archive entry being extracted to outside extract destination #1258

Closed TranceLove closed 5 years ago

TranceLove commented 6 years ago

Details about the vulnerability: here; fix for 3.2.1 is #1260 which had been released to Google Play Store as version 3.2.2.

Also brought the test suite from e0d40191e in #1222 for behavioural verification.

IOException will be thrown on erroneous archive entry, causing app to pop up an "Error" Toast.

Safe to cherry-pick into release-3.3.0.

Our sincere thanks to folks at snyk.io who tipped us about the vulnerability.

TranceLove commented 6 years ago

For fix against 3.2.1 the current Google Play Store release, see TranceLove:AmazeFileManager/bugfix/fix-b0rkenzip-321.

Since 3.2.1 codebase doesn't in favour of writing unit tests, verification was done against Pixel 2 emulators running 5.1.1, 6.0, 7.1.1, 8.1, plus Oneplus One running Slim7 (7.1.2), Galaxy S2 running SlimLP (5.1.1) and GPD XD running LegacyROM (4.4.4).

EmmanuelMess commented 6 years ago

@TranceLove PR TranceLove:fix-b0rkenzip-321 into hotfix-zip.

EmmanuelMess commented 6 years ago

In the win file, it seems to try to unzip the bad file (try to open it and then try to open anything else, check the notifications).

TranceLove commented 6 years ago

Added 9722b0d which ports 9888ae7 from #1260 for hotfix-zip.

Unit test also updated to reflect change (team members only).

EmmanuelMess commented 5 years ago

The non win extraction seems to still be broken. Also, I think we shouldn't show an "Error" toast on extraction, there was no error, the extraction is "complete" as far as the user is concerned, and explaining the real issue (probably malicious file) is too specific for the common man.

TranceLove commented 5 years ago

Added 3a88087. Unit test also updated to reflect changes (team members only - it will appear in PR once it's approved)

TranceLove commented 5 years ago

If you need a Dialog, you'll need to find a way to display a dialog in ExtractService.DoWork, best at onInvalidEntriesFoundBeforeStart(invalidArchiveEntries)...

EmmanuelMess commented 5 years ago

Ok, for some reason I thought you were trying to show compressed files in app.

You are trying to extract files (we do need better names, "decompress" and "extract" are synonymous, sorry for the naming mistake). We then have a serious problem, we cannot show a dialog without opening an activity (to the best of my knowledge). For now let's just keep the toast.

CompressedHelper.getExtractorInstance(...) is not supposed to handle ui stuff. For now it is a good idea to return doInBackground() with a boolean, then show a toast in ´onPostExecute()´.

TranceLove commented 5 years ago

See if it's what you'd expect in 045a51e.

VishalNehra commented 5 years ago

@EmmanuelMess

EmmanuelMess commented 5 years ago

@VishalNehra

EmmanuelMess commented 5 years ago

We merge this and release 3.3.0, @VishalNehra please review.

VishalNehra commented 5 years ago

I started a review for this. Also, we should wait for #1285 to pass before releasing v3.3.0

VishalNehra commented 5 years ago

@TranceLove please merge this with master and see the review I started.

TranceLove commented 5 years ago

@VishalNehra sorry but seems I couldn't find a review here... would it disappear in a force update?

However, if you're talking about this line...

<string name="multiple_invalid_archive_entries">Some files in the archive cannot be extracted; archive may be damaged or came from malicious source.</string>

Please see if you need a better wording for this.

VishalNehra commented 5 years ago

My bad, might have been network problem.

VishalNehra commented 5 years ago

Can you still not see the review? Basically I asked to use replaceAll instead of replace in Extractor#fixEntryName

TranceLove commented 5 years ago

Sorry but really couldn't see it... but is done at dac7948.

Also added B0rkenZipTest since PR approval is closing in soon.