Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Discussion: Antivirus on File Upload #679

Closed
jeurgen opened this issue Sep 27, 2019 · 18 comments
Closed

Discussion: Antivirus on File Upload #679

jeurgen opened this issue Sep 27, 2019 · 18 comments
Assignees
Milestone

Comments

@jeurgen
Copy link

jeurgen commented Sep 27, 2019

ASVS recommends applications to scan files being obtained from untrusted sources for viruses:  
 
"12.4.2 Verify that files obtained from untrusted sources are scanned by antivirus 
scanners to prevent upload of known malicious content."
 
I am not entirely happy with this requirement and will be discussing advantages and disadvantages in this issue in order to launch a discussion on possible improvements. 
 
Usually uploaded files are not executed on the server-side. They therefore do not put server systems at risk. Risks evolve for application users when malicious files are downloaded and executed on the client-side. One could therefore suggest that files should be scanned by antivirus on the user endpoint only.  
 
However, I still see the following advantages of server-side AV scanning: 
 

  • Users expect applications to take care of their security and implicitly assume this includes anti-virus scanning.  

  • A malicious file may benefit from the reputation of a trusted platform. The probability of this file being executed by a victim might increase. 

 
On the other hand, there are also a number of disadvantages: 
 

  • Increased attack surface: it is well-known that AV products are themselves often prone to vulnerabilities. These vulnerabilities might then be exploited through the application. 
     

  • AV scanning can cause unexpected behaviour if, .e.g, uploaded files are stored in database blobs and AV then quarantines the entire database.  Of course, this would imply an AV misconfiguration or misuse, but I heard it happens in practice. 
     

  • Files could be lost without the sender or receiver noticing. This of course depends on the particular implementation, but in practice the AV scan is not performed synchronously within the application itself but is performed by a third-party component.  

  • Server-side AV scanning increases the overall complexity, as applications often do not offer out of the box interfaces for AV integration. A common approach is therefore to offload AV scanning to reverse proxies. While this is perfectly possible it still adds another layer of complexity with – depending on the load – multiple additional server systems being deployed. 

In conclusion, I believe that the disadvantages outweigh the advantages. I would therefore suggest revising the requirement for server-side AV scanning. A possible trade-off could be to warn users about infected files when downloading. This would leave most of the disadvantages of server-side scanning in place, but has two benefits: 
 
1. Files don’t get deleted unnoticed, if AV scanning was not synchronized during upload. 

2. AV scanning takes place at the time of downloading with up-to-date signatures. 

 
Google and Microsoft are following this approach with their cloud storage, for example. Dropbox and Xing don’t seem to do any antivirus scanning. To me both of these methods (‘warn on download’ and ‘do nothing’) seem preferable to AV-Scanning on file upload.  

@elarlang
Copy link
Collaborator

Increased attack surface: it is well-known that AV products are themselves often prone to vulnerabilities. These vulnerabilities might then be exploited through the application.

It's not different from using whatever other component or library.

AV scanning can cause unexpected behavior if, .e.g, uploaded files are stored in database blobs and AV then quarantines the entire database. Of course, this would imply an AV misconfiguration or misuse, but I heard it happens in practice.

AND

Files could be lost without the sender or receiver noticing. This of course depends on the particular implementation, but in practice the AV scan is not performed synchronously within the application itself but is performed by a third-party component.

If to use it as separate service on file upload AND download, it does not affect (and scan) application itself.

Something to take in account:

  • user-uploaded files are stored in public folder (or content is accessible with HTTP request for non-authenticated user) - application is spreading known malware and will be blocked by browsers, ignored by Google etc. In this case, warning on download is not good. Additionally for potentially infected users, clear reputation damage.
  • opening user-uploaded files is part of officer job, if to not filter them in advance, it increases possibility for targeted attacks against organization or company

In conclusion, I believe that the disadvantages outweigh the advantages.

I don't agree.

  1. Files don't get deleted unnoticed, if AV scanning was not synchronized during upload. 

  2. AV scanning takes place at the time of downloading with up-to-date signatures.

Even I don't agree with all the argumentation provided, I can see that those 2 requirements are something what current requirement needs.

What AV related requirements should cover:

  • if user uploads malicious content - it's potential attack against your company/organization, you need to investigate it, so you can not just delete it, you need to store it but do not provide to site users
  • public and "private" files may need different behavior
    • there is need to actively scan files in public automatically (to avoid serving known malicious content from trusted domain)
    • private files, served by program code for authenticated users, it's ok to do it on file download

If to put "scan again on file download" to this requirement, there may need to rethink about the subcategory also.

@danielcuthbert
Copy link
Collaborator

I appreciate the comments here, this is a topic that we've had amongst ourselves a few times and I do agree with the numerous points being made here, except the "disadvantages outweigh the advantages" one.

It's clear that 12.4.2 needs either a re-write to be clearer about what it is we are trying to achieve, or expanding it into a series of requirements dealing with the complexities of objects being uploaded and subsequent processing and use of said objects.

I'm leaning more towards the latter over the former, but keen to hear others views

@vanderaj
Copy link
Member

If we can do a re-write without changing the meaning of 12.4.2, I'm happy to include this in 4.0.2. If it ends up being a huge change or multiple new items in 12.4, then this will have to wait for 4.1.

@vanderaj vanderaj self-assigned this Jun 23, 2020
@vanderaj vanderaj added this to the 4.0.2 milestone Jun 23, 2020
@elarlang
Copy link
Collaborator

Quickfix proposal (without creating new requirement) for v4.0.2

Current:

V12.4.2 Verify that files obtained from untrusted sources are scanned by antivirus scanners to prevent upload of known malicious content.

Proposal:

V12.4.2 Verify that files obtained from untrusted sources are scanned by antivirus scanners to prevent upload and hosting of known malicious content.

or "serving" instead of hosting?

@danielcuthbert
Copy link
Collaborator

Hosting is a bit oldfashioned right? yeah serving is more in line with what you would do if you had a delivery site for malicious code

@jeurgen
Copy link
Author

jeurgen commented Jul 20, 2020

Thanks for picking up on the issue. I think the discussion has turned into the wrong direction at the moment. I doubt the added value by appending "hosting" or "serving". The proposed changes do not deal with my actual concerns.

If AV scanning should be done mandatorily, then we should discuss about the right timing.

I still think it would give users a false sense of security, if they rely on AV Scans of a web application at the time of file upload. During the past months I dealt with several cases of emotet-like malware. Most of the time AVs were only able to detect the related files hours/days after the campaigns began.

As an attacker I would be happy to find a platform that does AV scanning on upload but not on download. It would give my malicious file the reputation of "AV Check passed" without having passed it at the relevant moment. The critical moment is when files from untrusted sources get delivered to other application users. I would agree on AV-scanning being done optionally also on file upload. But more important is the time of file delivery.

In the current proposal users are left unclear about the term "serving". This term would first need to be defined more precisely. Only adding the word "serving" would make the wording even more vague.

@elarlang
Copy link
Collaborator

elarlang commented Jul 20, 2020

  1. please explain, how given proposal is wrong direction
  2. what is your proposal?

Context - take in account, that for ASVS v4.0.2 the meaning for requirement text in big picture should stay the same. If we talk about separate requirements, then it's for ASVS v4.1 and a bit different discussion.

@tghosth
Copy link
Collaborator

tghosth commented Aug 23, 2020

I am going to push this to 4.1 as I think it is going to be too involved for 4.0.2

@tghosth tghosth modified the milestones: 4.0.2, 4.1 Aug 23, 2020
@ghost
Copy link

ghost commented Aug 28, 2020

I have misgivings about AV scanning being mandated also. Or to be precise, shifting any responsibility for scanning of files from the enduser (uploader or downloader). I think the increased attack surface argument is valid. But also I think it is accepted that static analysis (likely to be what server-side solutions will perform) is just not effective. It is too easily bypassed - hence the rise in EPP and EDR solutions in enterprises. Consumer products are also usually much more robust than the AV engines typically found running against e.g S3 buckets.

I also think it puts us on a path towards discussing who is responsible for uploaded content more generally, as this is where this issue is covered in website terms and conditions. Uploading illegal images could be more damaging than malware to both the site owner and users. As it is for privacy requirements, a steer in the direction of legal advice feels more appropriate for applications hosting user supplied content.

@jmanico
Copy link
Member

jmanico commented Aug 28, 2020 via email

@elarlang
Copy link
Collaborator

elarlang commented Aug 28, 2020

I use covid virus as metaphor...

So, @garethwj72 , you want to say, cashier in a shop (server, providing service for a lot of customers) don't need to wear a mask because it's shop visitor responsibility and problem, if they'll get a virus and only they need to wear masks?

People are willing to download content from trusted domains/sites, it completely to make sense to give some trust to this content on the server side. Client side hygiene is different topic and outside of ASVS scope.

And with this attitude, we should not have anti-virus checks, spam filters etc on email servers and hope that end-user can handle it.

@jmanico
Copy link
Member

jmanico commented Aug 28, 2020 via email

@ghost
Copy link

ghost commented Aug 28, 2020

@elarlang - without resorting to metaphors, I have yet to see a web application that does scanning for malware that isn't easy to bypass. Your average office worker has figured out they can sneak things around using zip+password. The average pentest goes as far as throwing an Eicar string at it. I just think it's a weak control that's too vague and too tied up in a whole other area of policing user created content.

@jmanico Just adding to the discussion as Daniel requested above, with no expectation of it being removed. I adapt ASVS to my needs and there is very little in it I won't stand behind.

@ay-kay
Copy link

ay-kay commented Aug 28, 2020

I disagree with @jmanico that all big players do this, during upload or in general. Microsoft claims to scan asynchronously in the background and to warn or block when downloading. Google states to scan for viruses before a file is downloaded. Users can download the virus-infected file, but only after acknowledging the risk of doing so. Note that Google also limits AV scanning to files smaller than 100 MB. Dropbox does not do AV scanning at all and recommends client-side scanning. iCloud does not seem to do any AV scanning - but there are no viruses on Macs either 😉

I also see some disadvantages with the scan on upload approach. If you can provoke a denial of service for the whole application with just an Eicar upload, this might not be good. Can we inform users about such difficulties? I also see the point that signatures may be more up-to-date at the time of download. I also understand @elarlang's argument, that special attention must be payed to public upload folders. But as @garethwj72 points out, there are not only viruses, which can cause damage.

In the end, we must also investigate whether traditional signature-based AV scanning is still effective and relevant today, whether on the client or on the server-side.

All in all, I think that already the discussion here shows that this requirement can be improved.

Finally, I really don't like the idea that everyone should fork their own ASVS. This is not my understanding on how to move a common standard forward.

@jmanico
Copy link
Member

jmanico commented Aug 28, 2020 via email

@elarlang
Copy link
Collaborator

One more idea or point-of-view - we don't need to put to the requirement, what is the world-wide situation at the moment (lack of virus checks), we need to set the goal or direction, where we would like to go.

@jmanico
Copy link
Member

jmanico commented Aug 28, 2020 via email

jmanico added a commit that referenced this issue Mar 12, 2021
@jmanico
Copy link
Member

jmanico commented Mar 12, 2021

Commit 8b3363c takes on suggestions of Elar and I'm closing this out. I regret we are not removing this per the original posters request and suggest those who disagree to fork the standard and remove this requirement for their teams.

PS: AntiVirus on upload is much safer in a serverless function.

@jmanico jmanico closed this as completed Mar 12, 2021
elarlang pushed a commit to elarlang/ASVS that referenced this issue Oct 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants