evolutionise commented 11 months ago

Background

A customer is reporting packages vulnerable to CVE-2021-24112 are turning up on their security scanning. The affected package is System.Drawing.Common.

We don't reference System.Drawing.Common directly in Halibut. It is a transitive dependency through System.Configuration.ConfigurationManager. In Halibut, we also don't directly use System.Configuration.ConfigurationManager. It is transitively referenced via System.Runtime.Caching.

The vulnerability is not exploitable because we are not using the affected versions of .NET Core/Mono.

Results

Fixing this has required two changes:

I've removed the direct reference to System.Configuration.ConfigurationManager as we aren't using it anywhere in Halibut.

The Config Manager is referenced by System.Runtime.Caching, which we do use to cache certain requests at the transport layer. I've upgraded System.Runtime.Caching from 5.0.0 to 8.0.0. This also upgrades System.Configuration.ConfigurationManager from 5.0.0 to 8.0.0. At some point in those changes, the Config Manager library no longer references the System.Drawing.Common library which removes the vulnerable package.

Doing this will remove one of the transitive dependencies Tentacle has on System.Drawing.Common - see this issue.

Some vulnerable versions remain inside Halibut - but those are in projects that are not included in the Production build of Halibut, specifically various Test and Build projects. I haven't upgraded those at this point because I want to keep the set of changes required to fix the Production side of this as streamlined as possible. Turning on auto-updates for dependencies will ensure these get fixed in a timely manner - that work is currently under way.

Fixes https://github.com/OctopusDeploy/Halibut/issues/555

Before

System.Drawing.Common 5.0.0 is implicitly referenced in the Halibut project.

After

The only implicit references to System.Drawing.Common are in non-production projects.

How to review this PR

Any thoughts on removing the explicit reference to System.Configuration.ConfigurationManager? I think this makes it tidier but open to discussion.

Quality :heavy_check_mark:

We have a decent set of tests around the caching where we use System.Runtime.Caching, so it's likely we'd catch any changes in behaviour.

Pre-requisites

[x] I have read How we use GitHub Issues for help deciding when and where it's appropriate to make an issue.
[x] I have considered informing or consulting the right people, according to the ownership map.
[x] I have considered appropriate testing for my change.

evolutionise commented 11 months ago

https://app.shortcut.com/octopusdeploy/story/65659/find-where-we-missed-updating-system-serviceprocess-servicecontroller-in-tentacle-due-to-cve

shortcut-integration[bot] commented 11 months ago

This pull request has been linked to Shortcut Story #65659: Find where we missed updating System.ServiceProcess.ServiceController in Tentacle due to CVE.

OctopusDeploy / Halibut