Clamav prescan

[aspacca]

• option to prescan with clamav before upload (rejecting if positive)

[stefanbenten]

I saw we have used the int status code in various places, so not the biggest need to change those.
But from what i can see the actual scanning would void the data and the actual backend upload should not get any data.

[stefanbenten]

I'll review/test this tomorrow 👍

[stefanbenten]

Overall worked fine for me, just a couple of small changes.

[stefanbenten]

For bigger files the upload also seems to "hang" while the prescan is ongoing. Should we add something to the web interface that shows this "step"?

[aspacca]

For bigger files the upload also seems to "hang" while the prescan is ongoing. Should we add something to the web interface that shows this "step"?

I haven't tested on UI, I will check.

The main problem is due to clamd limits:
1- I had to change from stream scan to file scan, on big files it would take forever with the stream
2- Maximum scan dimension is 4TB, and is still advised by clamav documentation to not set so high. Bigger file will be reported as clean. We should document this and probably apply automatically max-upload-size to match the max scan size set in clamd if clam prescan is enabled.

[stefanbenten]

I'll configured clamd for 4G files. Something i was thinking of was is splitting the temp files. I need to test if that works for a single binary thats malicious getting split up into say 4 smaller files and then scanned in parallel. In practice i would assume finding the same matches, but statistically, i might split the malicious code apart not letting clamd recognize it.

I wonder if for the UI we can simply adjust the progress bar.

[aspacca]

I think that splitting the file could lead to false negative.
In UI I'm not sure how we can handle: progress bar is rendered according to the upload progress

[stefanbenten]

I need to dive into that frontend code to look further, but yeah unlikely an easy way.

[stefanbenten]

@aspacca Lets get this pushed along and limit file size scan limit to something like 1G for now. It is better than nothing 👍

[aspacca]

@stefanbenten I wanted to refactor this PR in order to allow both ScanStream and ScanFile according to the type of clamav property (file or stream)

also setting automatically the file limit in transfer.sh if the prescan is enabled

[aspacca]

@stefanbenten feel free to merge after testing

[stefanbenten]

Thanks, will push the linting fixes as well after testing!

[aspacca]

I did the linting

[aspacca]

hi @stefanbenten

for me it is good to merge, I will later refine something around CLAMAV_HOST (now it will only work with local clamav instance)

[stefanbenten]

Thanks for pushing this up. I'll give it a go in a bit 👍

[stefanbenten]

Working like a charm! Thanks again @aspacca !

[aspacca]

let's wait to make a release when I'll add back support for clamav on the network :)

[LinusCDE]

This change along with missing (/tmp) broke my setup today. I'm using watchtower to update daily and today my transfer.sh instance went down due to this error:

I could trace this back to the newest commit and this PR. It seems that setting either CLAMAV_HOST to anything other than an empty string or PERFORM_CLAMAV_PRESCAN to true solved the issue. Having the prescan set to true while it doesn't affect the runtime without any clamav seems weird though. Also sure the panic should happen if CLAMAV_HOST is not set AND ONLY when PERFORM_CLAMAV_PRESCAN is false??

Aside: While looking up the command arguments, I also noticed that, on the dockerhub site, provider has no env specified while it does exist in the go code. So that could probably be added to dockerhub as well.

My setup is pretty simple btw: Just a local file provider with a mounted volume for the files. Also needed to add a volume /tmp now as well or any uploads would fail with 500 for missing "/tmp/transfer-something`.

EDIT: If interested, here is my current docker-compose file. The changes I made today where adding the /tmp volume as well as commenting out the command I used previously while adding the environment section.

[aspacca]

@LinusCDE unfortunately we cannot garantuee issues like this to happen on main branch and docker images based on main branch. You must consider main as unstable

The problem is here: https://github.com/dutchcoders/transfer.sh/pull/459/files#diff-8e494a434a8037b6c0b888e25b2baae7618fe65e792d4a155dadd096e9350667L396: !v shoud be v

fixed on #460

[aspacca]

the readme on docker hub has to be updated manually unluckily, but I see the missing env for provider is also on the repo.
I will fix as soon as possible.

as for /tmp to be mounted as volume I will change the Dockerfile to create it, so that's not needed unless you want to.

@LinusCDE do you mind opening two issues for the above? thanks :)

[LinusCDE]

@LinusCDE unfortunately we cannot garantuee issues like this to happen on main branch and docker images based on main branch. You must consider main as unstable

I can understand this way of thinking. However I intuitively thought the other way. Since some consider latest/main/master stable (having a dev branch or unstable builds) it can usually either be "stable" or "unstable". Seeing that on dockerhub, there are two "unstable" tags ("unstable" and "edge") and no explicit latest stable ones like "stable" "v1" or "1", I assumed latest to be the stable one. Even though latest gets updated daily, I just thought, this would be because the build is scheduled daily.

Should I create an issue as well for this?

Since I have auto-updating and there is no tag for the latest stable release, this would force me update the tag manually for each release and at sometime forget about this and let the version rot. I use watchtower to keep my containers/services up-to-date and usually try to make it target the latest stable branch (preferably of the current major version like "1", so I still need to address breaking changes manually).

[MarkusFreitag]

@aspacca Do you mean that there is no tag that can be used to pull the current stable release?
Honestly I considered edge to be the unstable build and latest to be a link to the current release.

[stefanbenten]

We probably could make a stable tag and only update it when we push tags and its corresponding builds. How would that sound?

[LinusCDE]

That sounds great!

[aspacca]

@aspacca Do you mean that there is no tag that can be used to pull the current stable release? Honestly I considered edge to be the unstable build and latest to be a link to the current release.

latest is an alias to edge/nightly. edge is built on the latest commit on master, nightly is schedelude every day (https://github.com/dutchcoders/transfer.sh/blame/main/.github/workflows/build-docker-images.yml#L38-L40)

In the end edge and nightly will converge, but until the next daily schedule edge will be forward to nightly

I personally agree with https://vsupalov.com/docker-latest-tag/, as long as we talk about production environment.
I think it's should be up to any CD logic to fetch/discover the most updated versioned tag and deploy it.

Anyway, let's add a stable tag :)

[LinusCDE]

On production, where people are paid to monitor their deployment this makes sense. But not when I'm having dozens of containers, that either update automatically or get forgotten.

Also production seems to usually have a host of other problems (namely having own up to date builds and not waiting on a dependency chain to fix CVEs). So they will most likely create their own containers anyway, and this is what it seems to target.