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

Updating assets can bypass virus scanning #72

Closed
chrisroos opened this issue Jul 18, 2017 · 0 comments
Closed

Updating assets can bypass virus scanning #72

chrisroos opened this issue Jul 18, 2017 · 0 comments
Assignees
Labels

Comments

@chrisroos
Copy link
Contributor

Updating an existing asset by uploading a different file but with the same name causes the asset-manager to bypass virus scanning.

Steps to reproduce:

  1. Create temporary file
$ echo `date` > tmp.txt
$ cat tmp.txt
Tue 18 Jul 2017 17:22:54 BST
  1. Upload file
$ curl http://localhost:3000/assets --form "asset[file]=@tmp.txt"
{"_response_info":{"status":"created"},"id":"http://localhost:3000/assets/596e35e4edf877ece4000003","name":"tmp.txt","content_type":"text/plain","file_url":"http://static.dev.gov.uk/media/596e35e4edf877ece4000003/tmp.txt","state":"unscanned"}
  1. Request file info

Ensure virus scanning is running (bundle exec rake jobs:work) and that the response says the file is clean.

$ curl http://localhost:3000/assets/596e35e4edf877ece4000003
{"_response_info":{"status":"ok"},"id":"http://localhost:3000/assets/596e35e4edf877ece4000003","name":"tmp.txt","content_type":"text/plain","file_url":"http://static.dev.gov.uk/media/596e35e4edf877ece4000003/tmp.txt","state":"clean"}
  1. Request the file

Observe that the content matches the content of our temporary file created in step 1.

$ curl http://localhost:3000/media/596e35e4edf877ece4000003/tmp.txt
Tue 18 Jul 2017 17:22:54 BST
  1. Update the temporary file
$ echo `date` > tmp.txt
$ cat tmp.txt
Tue 18 Jul 2017 17:25:09 BST
  1. Stop the virus scanning job

  2. Update the asset

NOTE. The file is already marked as clean in the response.

$ curl http://localhost:3000/assets/596e35e4edf877ece4000003 --request PUT --form "asset[file]=@tmp.txt"
{"_response_info":{"status":"success"},"id":"http://localhost:3000/assets/596e35e4edf877ece4000003","name":"tmp.txt","content_type":"text/plain","file_url":"http://static.dev.gov.uk/media/596e35e4edf877ece4000003/tmp.txt","state":"clean"}
  1. Request the file

Observe that the content matches the content of our updated temporary file that we uploaded in step 7.

$ curl http://localhost:3000/media/596e35e4edf877ece4000003/tmp.txt
Tue 18 Jul 2017 17:25:09 BST
@chrisroos chrisroos added the bug label Jul 18, 2017
@floehopper floehopper self-assigned this Aug 15, 2017
floehopper added a commit that referenced this issue Aug 16, 2017
Previously a virus scan was only triggered if the file had "changed"
according to `ActiveModel::Dirty` [1]. However, this seemed to have been
based on whether the filename of the file had changed rather than
whether the content had changed which seems to be the more relevant
issue. This meant that (as described in #72) it was possible to update
an existing asset with a file having the same name, but different
content and for the asset to remain in the "clean" state. This seems
like a security hole.

Initially I investigated triggering a virus scan when the file *content*
changed. However, this proved awkward due to the way both
`state_machines` and `carrierwave` gems work. So after discussion with
@chrisroos & @chrislo, I decided to trigger a virus scan whenever the
`Asset#file` attribute is changed. While this means virus scans will
sometimes be triggered unnecessarily, I don't think it should happen too
often and it seems better to do this than allow unscanned files to be
served to the citizens.

I'm not sure it's the most elegant solution, but since the `Asset#file=`
method already existed, it seemed simplest to make use of it. A better
solution might involve persisting an MD5 digest of the file content
which has been virus scanned and then only re-scan the content if it has
changed. However, the extra complexity doesn't seem warranted.

Fixes #72.

[1]: http://api.rubyonrails.org/v4.2.7.1/classes/ActiveModel/Dirty.html
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants