Aspnetcore: System.IO.Pipelines.dll in the targeting pack is not a reference assembly

[natemcmaster]

The versions of System.IO.Pipelines.dll and System.Threading.Channels.dll in Microsoft.AspNetCore.App.Ref aren't the actual reference assembly -- they're the implementation. This can cause problems later if there were to be a security issue with one of these since we don't plan to service the targeting pack. It also means the .dlls include unnecessary bloat.

This is happening because these packages only include a lib folder. To get the ref, we'll need to find a way to flow it from the corefx repo. (cref https://github.com/dotnet/corefx/tree/master/src/System.IO.Pipelines/ref/)

cc @davidfowl @Pilchie

[JunTaoLuo]

@DamianEdwards @Pilchie We should discuss who can take a look at this soon.

[Pilchie]

@anurse - is this something you or your team could take a look at?

I'm not sure that this is required for Preview 7, but looks like it needs to be done fore 3.0 GA given the servicing implications.

[anurse]

This can cause problems later if there were to be a security issue with System.IO.Pipelines.dll because we don't plan to service the targeting pack

I'm not sure I understand why this is a problem. Even though it's the full assembly, it's placed in the ref/ folder, right? That mean it's only used for compilation and not for deployment. That's precisely why we don't need to service targeting packs, because they're never used for deployment. Even if you compile against a version of System.IO.Pipelines with the issue, you won't run on it.

This is happening because System.IO.Pipelines the package only includes a lib folder. To get the ref, we'll need to find a way to flow it from the corefx repo.

We'll have to follow up with corefx to see if we can do that. I can file a bug to check in on that.

It also means the .dll includes unnecessary bloat.

Yeah, that is a reason to get a ref asm :).

@anurse - is this something you or your team could take a look at?

Yeah, we can take a look at it.

[anurse]

@ericstj do you have thoughts on the need to specifically use a ref assembly in the ref/ folder of our targeting pack package? To do this, we'll probably need corefx to either produce the ref/ folder in System.IO.Pipelines.nupkg or provide some other asset we can flow through Maestro to get the ref assembly.

[natemcmaster]

That's precisely why we don't need to service targeting packs, because they're never used for deployment.

While this is true, what we have learned from the past is that IT departments will scan hard drives for files that have known security issues, and flag issues regardless of the file's location. So hypothetically if there is a problem in System.IO.Pipelines.dll 3.0.0, we might still need to service the targeting pack. This problem regularly comes up with .NET Core 2.* packages that are still shipping vulnerable binaries in LZMA package cache, even though there is little chance the vulnerable binary makes it into the app's runtime.

[ericstj]

@joperezr this is due to https://github.com/dotnet/corefx/commit/6832425094123c858b9e5da02469c98ca8c200c4.

[anurse]

@natemcmaster That's definitely true. Though it also depends on how they are scanning those files. I don't know if they are sophisticated enough to identify the difference between a reference assembly and an implementation assembly. Most scanners I've come across are looking at file name and file version alone. Still, I do agree it's better to keep the unpatched binary off the machine as much as possible.

@ericstj @joperezr Is this something we can resolve in corefx (getting a ref/ folder for System.IO.Pipelines)? Should I just file a bug there to do so?

[ericstj]

Yeah, file an issue requesting a change. FWIW this is not a bug, we intentionally omit ref's in the case a lib will be used by desktop projects due to lots of old tooling that tries to load references for execution. I think we can get away with having a reference, only for 'netcoreapp'.

[anurse]

Filed https://github.com/dotnet/corefx/issues/38729 . This work is currently blocked on that issue.

[ericstj]

Sorry, what's "blocked" on this, I wouldn't expect anything to be blocked.

[anurse]

Resolving this issue, which is that we currently have an implementation assembly in our targeting pack rather than a ref assembly. You are correct that a user scenario is not blocked.

[natemcmaster]

We didn't want to put ref assemblies in Microsoft.Extensions.* packages, so we created a transport package with ref assemblies which is only used to move bits from aspnet/Extensions into aspnet/AspNetCore. We could do the same thing for this package if it doesn't make sense to put the ref into System.IO.Pipelines.nupkg.

https://github.com/aspnet/Extensions/tree/master/src/TargetingPack/Microsoft.Internal.Extensions.Refs/ref
https://github.com/aspnet/AspNetCore/blob/56ffc6b58210292a18f548bf4eddff193f73a4d6/src/Framework/ref/Microsoft.AspNetCore.App.Ref.csproj#L48
https://github.com/aspnet/AspNetCore/blob/56ffc6b58210292a18f548bf4eddff193f73a4d6/src/Framework/ref/Microsoft.AspNetCore.App.Ref.csproj#L92-L101

[joperezr]

BTW, seems like System.Threading.Channels is on the same boat. It is also in the targeting pack and it is also a lib instead of a ref. I presume you would need a fix for that as well.

[anurse]

@joperezr That's correct.

[natemcmaster]

Actually, that's not correct. System.Threading.Channels is part of netcoreapp. https://github.com/aspnet/AspNetCore/issues/10938, https://github.com/dotnet/corefx/pull/38207. Only System.IO.Pipelines is affected by this.

[joperezr]

You are right, I was basing myself on the preview6 sdk, but I do see in master that this is now part of Microsoft.NETCore.App

[natemcmaster]

@Pilchie this one might affect servicing 3.0.x and would be good to review for RTM.

[anurse]

Looks like we're good here. I grabbed 3.0.0-preview7.19365.7 of Microsoft.AspNetCore.App.Ref off NuGet.org and dumped System.IO.Pipelines.dll. Looks like a refasm to me:

[natemcmaster]

Would recommend removing this test exclusion

https://github.com/aspnet/AspNetCore/blob/4921dc3aa3496eb3e9fe73255213ce54a0ba29d6/src/Framework/test/TargetingPackTests.cs#L37-L38

[anurse]

Sounds good. I'll reopen to keep it on the radar :).

[Pilchie]

Poaching.

[Pilchie]

Closing as this was fixed in 3.0.