Yarn: Files are extracted before their hashes are checked.

[ghost]

Do you want to request a feature or report a bug?
A bug.

What is the current behavior?
Currently, downloaded files are extracted before their hashes are checked.
https://github.com/yarnpkg/yarn/blob/master/src/fetchers/tarball-fetcher.js#L75

What is the expected behavior?
Files should be verified before they are extracted.

[BYK]

This is a performance optimization since calculating the file hash requires us to read it twice.

We do remove the extracted assets AFAIK so it should be equivalent at the end. What do you think? //cc @arcanis @kaylieEB @rally25rs

[ghost]

I don't like that any processing happens on a file before its hash is checked. The file could be a ZIP bomb or contain an exploit causing malicious code to execute if the file is decompressed.

[kaylie-alexa]

Not a bug per se, but I agree would be a nice enhancement :)

[BYK]

I don't like that any processing happens on a file before its hash is checked. The file could be a ZIP bomb or contain an exploit causing malicious code to execute if the file is decompressed.

I thought about these also, but it doesn't prevent one from putting a zip bomb with a correct hash so not sure if this would __really__ help preventing such cases. The aim was mostly to prevent unknown/malicious code being injected into applications which is a lot more dangerous than zip bombs.

I mean I do agree with you that it is not absolute security but for the problem, it was designed to solve, it is enough and reading the file twice would incur some performance penalty. I'm trying to judge if that penalty is worth what we are trying to protect against.

[BYK]

Well, an alternative approach that may be as fast is checking the checksum during download. How about that?

[ghost]

@BYK Great!

[ghost]

@BYK What about an exploit for tar or gzip format? That would be more dangerous.

[ghost]

@BYK Any progress on this issue?

[BYK]

@ghost no progress. Would you like to give it a shot?

[JoaoSRocha]

hey guys! i will be trying to help with this over the next few weeks!

[vog]

Has there been any progress on this?

Given the strong security promises on the website, I feel a bit tricked seeing an open issue like that, especially when open for several months but no warning at the website.

I'm perfectly fine if strong security is not a primary goal for Yarn. Even with that caveat it is a huge improvement over NPM. Just be honest about that on your project website.

[BYK]

@vog I see that you care a lot about the security and the promises we make. I agree that this is less than ideal but I don't see how this breaks the claims we make on the web page.

If you have something in mind, feel free to submit a PR to fix this issue or to the website repo for better wording and we'd be more than happy to review and merge those.

[vog]

Done. I hope that helps:

https://github.com/yarnpkg/website/pull/823