Remove symlinks in zip files

[ImanSharaf]

I was checking this HackerOne report with a $29000 bounty and I found it very interesting. This is different than Zip Slip. In case of Zip Slip we can inject .. in the file path so we can extract our file in a wrong place. In this report, the attacker crafts a malicious symlink to /etc/passwd when the backend extracts it untar_zxf function only changes the permissions and extract the symlink as is, so the attacker was able to read the passwd file!
I believe we need an ASVS item to check for removal of symlinks in the zip files.

[elarlang]

This is interesting.

Is there any harm it can do to end user as well? I mean should application prevent uploading zip-files containing symlinks, or it is only necessary when application actually extracts files (from user) and processes their content.

[ImanSharaf]

If the user directly uploads a symlink that links to /etc/passwd the user's passwd file would be uploaded to the server. But if the app has a function to untar or unzip (for example, a Ruby app), and then the user is able to see the content of the uploaded zip or tar file, then the user will be able to see the server's passwd file. I believe on of the mitigations is to remove any symlinks when the app starts to extract a compressed file.

[csfreak92]

Interesting attack vector. Seems like a valid requirement to have. Does this mean it is sort of an RCE-level kind of threat the way this vulnerability works?

[ImanSharaf]

@csfreak92 It could lead to an RCE if the attacker would be able to fetch some sensitive secrets in the environment variables files or the source code.

[elarlang]

Well, played with that a bit. Now there is nice question - what is the actual defense?

Original proposal:

I believe we need an ASVS item to check for removal of symlinks in the zip files.

Options:

• removing only symlinks - like proposed, but It's not my favourite
• requirement do not allow to upload archives with symlinks at all
  • good: threat taken down in early stage and does not depend on configuration
  • bad: it requires inspecting the archive content on every file upload and may increase the risk for zip bombs
• require unpacking zip files without following symlinks or in sandboxed environment

[guillaume-d]

Well, played with that a bit. Now there is nice question - what is the actual defense?
[...]
Options:

* removing only symlinks - like proposed, but It's not my favourite

Why? (It is not mine either, see below, just asking for clarification.)

* requirement do not allow to upload archives with symlinks at all
  
  * good: threat taken down in early stage and does not depend on configuration

Indeed.

  * bad: it requires inspecting the archive content on every file upload and may increase the risk for zip bombs

The risk for *.zip bombs will not increase on inspection if one just scans the archive structure, because files in *.zip archives are always compressed individually (not-solid compression).
The scan can be done in linear time proportional to the compressed archive size, which of course should have an separate limit.

For example one can use:

• zipinfo -l
• else most programming languages offer low-level abstraction for the ZIP entries and the ZIP central directory

For other formats like compressed tarballs, *.7z, etc. using solid compression, this may indeed a problem.
Only allowing the ubiquitous ZIP format may be a viable option. Often it is not used for its unique compression or metadata features (if any) but just to collect files together, so offering much less popular choices may not matter.

Uncompressed tarballs are OK too: similarly tar tf can help detect symlinks there, else low-level abstractions for the TAR format should be readily available as well.

* require unpacking zip files without following symlinks or in sandboxed environment

With a sandbox symlinks do not do harm at once, but when deleting the sandbox one still has to make sure not to follow symlinks...

Another much better "meta-option" is of course to find a library or framework with safe defaults and good structured logging (for detection) that can do most of the hard work above...

[elarlang]

Why I don't like removing symlinks - it is automodification (changing integrity) for user-provided data.

Is there any valid and needed use-case why an application should allow symlinks?

My direction is something like (wordsmithing and correction needed):
Verify that the application do not allow uploading compressed files containing symlinks if not especially intended by the application.

Category V12.1, Level 2 (as zip bomb 12.1.2 is also level 2)

[csfreak92]

I am also inclined to agree with @elarlang on this one. Automodification of any uploaded files that may contain symlinks breaks the principle of integrity of the user's data.

@ImanSharaf, I am on the same boat as with that question of what is the valid and needed use-case for an application to allow symlinks? Maybe you could enlighten us more about it?

[elarlang]

@ImanSharaf, I am on the same boat as with that question of what is the valid and needed use-case for an application to allow symlinks? Maybe you could enlighten us more about it?

I think the question was addressed to @guillaume-d

[ImanSharaf]

@elarlang @csfreak92 @guillaume-d where are we at right now for this one? Can I commit the new item with the proposed solution?

[elarlang]

I don't think it's ready yet. FYI: we do some "commit sessions" with Josh and it will be addressed one day :)

[guillaume-d]

@ImanSharaf, I am on the same boat as with that question of what is the valid and needed use-case for an application to allow symlinks? Maybe you could enlighten us more about it?

I think the question was addressed to @guillaume-d

Sorry I missed the correction! I do not have a usecase for symlinks in archives either.
I was merely pointing out that @erlang's only "self-inflicted" drawback to his option of forbidding symlinks in uploaded archives does not hold for *.zip archives.
I cannot think of another drawback either.
So I am also in favour of forbidding symlinks unless absolutely necessary, for what it's worth.
Maybe avoiding other (solid) archive formats would also be good advice (so that the symlink scanning itself does not trigger zip bombs, see #1424 (comment)).

[elarlang]

As proposal we can finetune input from #1424 (comment)

Verify that the application do not allow uploading compressed files containing symlinks if not especially intended by the application.

Category V12.1, Level 2 (as zip bomb 12.1.2 is also level 2)

[tghosth]

Verify that the application do not allow uploading compressed files containing symlinks if not especially intended by the application.

What happens if we do need to accept symlinks? Is there a way to protect ourselves in that case @elarlang?

[elarlang]

I don't know what could be the reason to accept symlinks? But if you really need to, you need to validate it against allow-list or use sandboxed environment etc. Anyway, I don't think we need to cover that in more detailed than "if not especially intended by the application".

[tghosth]

Ok so how about:

Verify that the application does not allow uploading compressed files containing symlinks unless this is specifically required (in which case it will be necessary to enforce an allow list of the files that can be symlinked to).

[elarlang]

ok for me

[tghosth]

Opened #1670

[tghosth]

Opened #1671 to include a better CWE