ASVS icon indicating copy to clipboard operation
ASVS copied to clipboard

There is no item to check Zip Slip related vulnerabilities

Open ImanSharaf opened this issue 3 years ago • 11 comments

There was a situation that my target was not vulnerable against Directory Traversal Attack in a normal way, but it was allowing me to upload a compressed folder, I was able to upload a PHP shell file in a folder that had the permission to execute the file by doing this: 1- Create a zip file with a Python code and for the name of the target file to be compressed use something like this ../../../../../../../../var/www/target/root/shell.php [we cannot do it using normal applications that can ZIP files] I believe we need to have an ASVS item to check that.

Reference

ImanSharaf avatar Feb 11 '22 17:02 ImanSharaf

Abstractly, for my taste it's covered by path traversal points. See:

  • V12.3.1 Verify that user-submitted filename metadata is not used directly by system or framework filesystems and that a URL API is used to protect against path traversal.

If the file was written to given destination by zip library, then it's path traversal problem in zip library - should we get zip library specific requirement, I'm not sure. Taken in account, it was "out of box" in 2018, and now most widespread libraries should be fixed or "known to be vulnerable" == avoidable.

Perspective is - that zip library is one of the components in the application, and it should not contain known vulnerabilities:

  • V1.14.3 Verify that the build pipeline warns of out-of-date or insecure components and takes appropriate actions.
  • V14.2.1 Verify that all components are up to date, preferably using a dependency checker during build or compile time.

elarlang avatar Feb 11 '22 18:02 elarlang

I still believe that we need to mention Zip Slip attack as many pentesters might not be aware of this method, maybe we can change 12.1.2 and a note to check Zip Slip attack too.

ImanSharaf avatar Feb 11 '22 20:02 ImanSharaf

Extra awareness is never bad, but I think to cover all possible test-cases is more testing-guide context.

At the moment I would say - problem in the issue title is covered, there are ASVS requirements for reporting it.

Let's wait feedback from others as well.

elarlang avatar Feb 12 '22 10:02 elarlang

@jmanico What do you think about this one?

ImanSharaf avatar Feb 16 '22 07:02 ImanSharaf

I think there is a high likelihood that someone could write this into their own code and therefore just talking about vulnerable dependencies is not enough.

I agree that this is a kind of path traversal vulnerability but (as discussed in #1136) I am not sure if the 12.3 requirements are clear enough to cover this situation as well.

@ImanSharaf please could you look at the requirements in that section and see whether you think they cover the root cause of this vulnerability?

tghosth avatar Feb 23 '22 07:02 tghosth

I think there is a high likelihood that someone could write this into their own code

High likelihood, that people implement zip libraries not using zip libraries? If in general "take file path from some container and use it later" then it's not zip specific and should be handled as path traversal.

Mentioned 12.3.1 needs some update anyway.

elarlang avatar Feb 23 '22 07:02 elarlang

I agree 100% that zip slip should somehow be mentioned. Two things can go wrong. The library may be totally vulnerable and the developer may not be checking for this path traversal. For example, I typically unzip uploaded files into a staging directiony with OS-level limits....

jmanico avatar Jun 10 '22 13:06 jmanico

I agree 100% that zip slip should somehow be mentioned. Two things can go wrong.

Jim, I addressed those both cases in my comment https://github.com/OWASP/ASVS/issues/1222#issuecomment-1036519550

The library may be totally vulnerable ...

If the library is vulnerable, then it is covered by requirements:

  • V1.14.3 Verify that the build pipeline warns of out-of-date or insecure components and takes appropriate actions.
  • V14.2.1 Verify that all components are up to date, preferably using a dependency checker during build or compile time.

and the developer may not be checking for this path traversal.

Then it's clear path traversal, covered by requirement:

  • V12.3.1 Verify that user-submitted filename metadata is not used directly by system or framework filesystems and that a URL API is used to protect against path traversal.

At the moment I can not see, how this new requirement could be unique - without being duplicate one of mentioned requirements.

elarlang avatar Jun 15 '22 18:06 elarlang

So having looked at 12.3, I think we could consider a specific requirement for zipslip or at least a mention of zipslip within an existing item in this section as it already has multiple requirements related to relying on user provided file paths.

To be honest, I think 12.3.1, 12.3.2, 12.3.3 and 12.3.5 all need clarifications to make the differences between them clearer and zipslip could be a part of this.

@elarlang what do you think?

tghosth avatar Jun 22 '22 13:06 tghosth

Maybe an addition like so: V12.3.1 Verify that user-submitted filename metadata is not used directly by system or framework filesystems and that a URL API is used to protect against path traversal. Path traversal can also be exploited by extracting files from a zip file or other archive.

jmanico avatar Jun 23 '22 15:06 jmanico

So I disagree with combining it because whilst this is conceptually the same issue, the first part is solved using some sort of URL API but the zip slip thing would need to be fixed in the code that processes the zip files.

tghosth avatar Jul 04 '22 15:07 tghosth

So having looked at 12.3, I think we could consider a specific requirement for zipslip or at least a mention of zipslip within an existing item in this section as it already has multiple requirements related to relying on user provided file paths.

ok, let's go for separate requirement.

To be honest, I think 12.3.1, 12.3.2, 12.3.3 and 12.3.5 all need clarifications to make the differences between them clearer and zipslip could be a part of this.

there is separate issue (now) for those requirements: https://github.com/OWASP/ASVS/issues/1427

elarlang avatar Nov 22 '22 06:11 elarlang

So how about:

Verify that server-side file processing such as file decompression ignores user provided path information.

tghosth avatar Apr 03 '23 16:04 tghosth

If I could not know from this issue that proposed requirement is about zip slip, is it intuitive and understandable, that it is about zip slip? I think it makes sense to mention it somehow in the requirement.

elarlang avatar Apr 05 '23 09:04 elarlang

So how about:

Verify that server-side file processing such as file decompression ignores user provided path information to prevent vulnerabilities such as zip slip.

@elarlang

tghosth avatar May 31 '23 14:05 tghosth

Not sure about category, but let's get it into 12.3 and move later to better one if we('ll) have some better one.

elarlang avatar May 31 '23 16:05 elarlang