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

Getting a malware trojan warning from our local machines for ImageProcessor.dll #162

Closed
kgiszewski opened this Issue May 4, 2015 · 14 comments

Comments

Projects
None yet
5 participants
@kgiszewski

kgiszewski commented May 4, 2015

Should we be concerned?

screen shot 2015-05-04 at 9 51 31 am

@cosmo0

This comment has been minimized.

Collaborator

cosmo0 commented May 4, 2015

Well, it can come from pretty much anywhere.

ImageProcessor itself does not contain malicious code (that's a benefit of open source, you can check it out), but we cannot guarantee that the place that you got your DLL from haven't added code on top of it.

You can also already have a virus that infects your PCs.

@JimBobSquarePants

This comment has been minimized.

Owner

JimBobSquarePants commented May 4, 2015

It certainly shouldn't, however I want to double check that nothing has somehow got piggy backed. What antivirus is that? Could you run the file though https://www.virustotal.com/

@JimBobSquarePants

This comment has been minimized.

Owner

JimBobSquarePants commented May 4, 2015

I think your server is compromised. Here's a scan of the binary from the latest package. https://www.virustotal.com/en/file/96ac35fd1eba8c0414ed8b0a051d679c52d8ea5310e0377a62fe303228016eda/analysis/1430752679/

@kgiszewski

This comment has been minimized.

kgiszewski commented May 4, 2015

Right so, as an update...

Our campus-wide malware definitions were updates on the 1st and that's when we started getting the notification (we use MalwareBytes). We get it on any machine using NuGet as our source.

I'm not at all concerned with the source code and since our checksum matches, I'll have to just ignore it for now.

Closing, but thought I'd ask :)

@kgiszewski kgiszewski closed this May 4, 2015

@JimBobSquarePants

This comment has been minimized.

Owner

JimBobSquarePants commented May 6, 2015

Raising it was definitely the right thing to do. Thanks.

@JimBobSquarePants

This comment has been minimized.

Owner

JimBobSquarePants commented Aug 21, 2015

Just to note. I will no longer be using png.out in future versions to post process images. That is the file which has been triggering the antivirus.

@nul800sebastiaan

This comment has been minimized.

Contributor

nul800sebastiaan commented Dec 20, 2015

After receiving 3 questions about this in a week, it seems like windows defender is now detecting this as a threat more aggressively than before. This is obviously not a good experience when you try out Umbraco for the first time, and I can't imagine it's a good experience for people checking out ImageProcessor for the first time.
On top of that, there's now been a few reports of the post processing tools not working on some hosts (#268 and #279). And it might be causing performance problems for some people with large amounts of images, which are unconfirmed, and I've not looked into this at all so it might not be related at all: https://our.umbraco.org/forum/umbraco-7/using-umbraco-7/73814-umbraco-728-upgrade-to-733-caused-memory-issues-tracked-down-to-upgrade-of-imageprocessor

I think we have a few options:

  1. The post processing could be separated out as it's own NuGet package, which doesn't get included in Umbraco so that it's not detected as a threat.
  2. The command line utilities are no longer embedded in the dll, which may or may not help (we don't know) with threat detection.
  3. Umbraco can fork and release a modified version of ImageProcessor.Web which doesn't include the post-processing tools so as to avoid trojan detection and incompatibilities with hosting providers.

I believe number 1, separating out the post processing is likely the best option as it will also avoid issues with people using IP without Umbraco.

Happy to work on it and provide a pull request after feedback on this!

@JimBobSquarePants

This comment has been minimized.

Owner

JimBobSquarePants commented Dec 20, 2015

Option 4.

Microsoft gets their arse in gear and stop detecting perfectly valid software as a virus... Do you have any contacts there because this is just stupid?

The code used to be a split out package but I integrated the it into the main solution so that all the caches could benefit.

I guess it's Hobson's choice... 1 seems to my only option. I should potentially be able to split it back out now that I'm using a stream. I may simply scrap it though. Depending on 3rd party software always seems to cause me problems.

The current virus detection is attributed to one program: PngOut.

I'm not the only one to have problems. madskristensen/WebEssentials2013#1508

OptiPng triggers a similar response also.
madskristensen/WebEssentials2013#541

That other issue is totally unrelated.

ImageProcessor.Web v4.4.0 intercepts all image requests after I reduced the processing criteria because Umbraco has RAMMFAR enabled by default, why is that the case? As such, the images on that site are being stored temporarily in memory, at full, unprocessed size. Why are they not using ImageProcessor and the built in cropper I wonder?

As regards to that. I'm going to enforce that all image requests require querystring params again. I may even drop storing images in memory at all.

I'm gonna create a new feature branch and take a crack at stripping it all out. I'll give you a shout if I need a hand though I'm hoping it won't take more than an hour or so.

@JimBobSquarePants

This comment has been minimized.

Owner

JimBobSquarePants commented Dec 21, 2015

@nul800sebastiaan I've done the work to split the code out. What's your ETA for v7.4.0?

df7fc60

@nul800sebastiaan

This comment has been minimized.

Contributor

nul800sebastiaan commented Dec 21, 2015

Why thank you kind sir, that was fast! No rush, most people are on holiday so no release until next year and it all depends on feedback from testers.

@Jeavon

This comment has been minimized.

Collaborator

Jeavon commented Dec 21, 2015

Seems like a sad but essential step, if Mads hasn't been able to get Defender fixed it would seem unlikely anytime soon :(

Two things, firstly with the post processor split out again, will it still work ok with the Azure cache plugin as it does now? Secondly, please don't totally remove the processing of images without querystrings, we have come to rely on it when used in combination with a CloudImageService, at the very least let it be enabled by configuration.
Thanks!

@JimBobSquarePants

This comment has been minimized.

Owner

JimBobSquarePants commented Dec 21, 2015

@nul800sebastiaan No worries, It was a bee in my bonnet so I had to get it fixed.

@Jeavon Yeah, this will work with all the cache systems as it now intercepts the stream rather than the old way using the file path. I'll tweak the code to allow the configuration of querystring-less image interception. By default I will probably have it off though.

@Jeavon

This comment has been minimized.

Collaborator

Jeavon commented Dec 21, 2015

@JimBobSquarePants many thanks!

@nul800sebastiaan

This comment has been minimized.

Contributor

nul800sebastiaan commented Dec 22, 2015

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment