Multiple Arbitrary File Write vulnerabilities

[catfag]

A specially crafted malicious .unitypackage can save or overwrite arbitrary files on the system when extracted with unitypackage_extractor due to several vulnerabilities. This could be used to install malware or overwrite important files.

tarfile.extractAll

unitypackage_extractor/unitypackage_extractor/extractor.py

Lines 16 to 17 in a05765a

	with tarfile.open(name=packagePath, encoding=encoding) as upkg:
	upkg.extractall(tmpDir)

Please note the warning in TarFile.extractall's docs: It doesn't ensure that files aren't created outside of the specified path. Here's a proof of concept:

touch /tmp/badfile
# Make a tarfile with an absolute path
tar -cf bad.tar -PC / /tmp/badfile
rm /tmp/badfile
python -c "import tarfile; tarfile.open('bad.tar').extractall('.')"
ls /tmp/badfile # It exists again!

When fixing this please keep in mind that tarfiles can also contain symlinks and other weird things. So maybe this approach might help to mitigate this issue.

No validation of "pathname" in .unitypackage

unitypackage_extractor/unitypackage_extractor/extractor.py

Lines 28 to 36 in a05765a

	with open(f"{assetEntryDir}/pathname", encoding=encoding) as f:
	pathname = f.readline()
	pathname = pathname[:-1] if pathname[-1] == '\n' else pathname #Remove newline
	#Extract to the pathname
	print(f"Extracting '{dirEntry.name}' as '{pathname}'")
	assetOutPath = os.path.join(outputPath, pathname)
	os.makedirs(os.path.dirname(assetOutPath), exist_ok=True) #Make the dirs up to the given folder
	shutil.move(f"{assetEntryDir}/asset", assetOutPath)

This very similar to the above. If a specially crafted .unitypackage has a modified pathname file in it that contains an absolute path, then the asset file will be moved to that location. This issue exists regardless of whether the user specifies an extraction output path or not, as os.path.join() will ignore its first argument when the second argument is an absolute path (tested on Windows and Linux).

As above the path should be validated. I don't think any valid .unitypackage will have absolute paths in it (in the tar itself, or in the pathfiles) so it would make sense to abort extraction if this is the case and inform the user.

[Cobertos]

Thank you for the link to tarsafe, I will give that a look.

For the second vulnerability, you think it would just be enough to skip w/ notify versus abort? I like the idea of it at least being able to continue if one pathname is malformed/absolute, for whatever reason

Thank you for the submit

[catfag]

At the end of the day, I don't think it really matters whether it'll skip or abort. I just have a personal preference for abort here (of course notifying the user why it aborted) and not extracting anything, because I don't see any reason why Unity (or external stuff, which seems rarely used) would generate them. If for some reason there'd be a lot of .unitypackage files out there that had invalid entries in them (e.g. if, say, Unity 1.0 stored some metadata files under absolute paths or something) I'd choose skipping, but since this is entirely unexpected (and a potential security issue) I'm going for aborting.

[Cobertos]

Aight, should be fixed, added 2 tests for the pathname writing outside the extract path as well as using tarsafe now.

Let me know if #15 looks good