Signing ARM/ARM64 crossgened images broken

[morganbr]

Signing ARM or ARM64 crossgened images fails because the images are incorrectly identified as regular managed assemblies and the SignTool task tries (and fails) to StrongName sign them.

This is most likely due to the ContentUtil.GetAssemblyName method, which uses AssemblyName.GetAssemblyName to detect whether an assembly is managed. AssemblyName.GetAssemblyName throws for x86 and x64 crossgened images, but returns a value for ARM images

arcade/src/Microsoft.DotNet.SignTool/src/ContentUtil.cs

Line 64 in d7e189c

return AssemblyName.GetAssemblyName(fullFilePath);

[JohnTortugo]

Can you post a link to a build or where we can get some logs?

[morganbr]

https://dnceng.visualstudio.com/internal/_build/results?buildId=58190 is a test signed build that hit this (real signing has a similar problem). You can test it with System.Private.CoreLib from any ARM or ARM64 build.

[JohnTortugo]

@morganbr is this blocking your work?

[morganbr]

I've worked around it by not signing the binary where we hit this, but we'll need to sign it eventually.

[JohnTortugo]

Thanks for confirming. I'll put this on my list of soon to do things.

[JohnTortugo]

Weird enough what we have is the recommended way to implement this .. https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/concepts/assemblies-gac/how-to-determine-if-a-file-is-an-assembly I'm looking into adding a stronger check.

[JohnTortugo]

So, my understanding right now is that crossgen seems to keep information on the output file that mislead all checks that I've tried so far. The only way I could successfully distinguish between managed / native code was by trying to load the file.

Right now my idea is to try to load the file using AssemblyLoadContext.LoadFromAssemblyPath and treat BadImageFormat as native code condition.

Any suggestion here @tmat ?

[JohnTortugo]

I tried to move forward with the idea of AssemblyLoadContext.LoadFromAssemblyPath but got some problems since AFAIU it's not possible to unload the file so I couldn't subsequently open the file for writing.

I ended up implementing a check by inspecting the PE structure of the file. AFAIU we should be able to tell whether the file is managed or not if it has an entry at position 14 of the Data directory (for instance, see comment here) and also checking that the content/size of that entry matches expected values.

[sbomer]

After taking an arcade update, we're now seeing signing of SPC fail on x64 and x86 in addition to ARM/ARM64, with the following (from https://dnceng.visualstudio.com/internal/_build/results?buildId=74359):

2019-01-16T22:24:49.3841998Z   3> 16.22.24.48 F:\vsagent\20\s\bin\Product\Windows_NT.x64.Release\System.Private.CoreLib.dll does not represent a strongly named assembly
2019-01-16T22:24:49.3842203Z   21> 16.22.24.48 Wait 2 minutes before retry

[JohnTortugo]

I'm taking a look

[JohnTortugo]

The problem is that you're using the default version of sign tool that comes with the SDK - It's a known issue - unfortunately that version doesn't have the fix for this issue. I'll update the default version on the SDK and once you get a new maestro update everything should work fine.

[morganbr]

@JohnTortugo My guess would be that your change now accurately detects that S.P.CoreLib is managed, but doesn't notice that it's crossgenned (and thus still shouldn't run through SN). I'm not sure how to find it in the file, but crossgenned images have the IL_Library corflags bit set. The metadata reader API may be helpful.

[sbomer]

Thank you! In case it helps, we recently switched x64 and x86 images for SPC from fragile NGEN to R2R.

[JohnTortugo]

@JohnTortugo My guess would be that your change now accurately detects that S.P.CoreLib is managed, but doesn't notice that it's crossgenned (and thus still shouldn't run through SN). I'm not sure how to find it in the file, but crossgenned images have the IL_Library corflags bit set. The metadata reader API may be helpful.

Thanks @morganbr . I'm inclined to think that this is not the case.. we have some unit tests for checking this and they are passing. Nevertheless, the information you shared will definitely look in any eventual issue!

[RussKeldorph]

Our inability to sign S.P.C. is blocking the next .NET Core 3.0 Preview, so resolving this is urgent. Can we put in some hack logic for now to special-case this file rather than trying to detect the right thing to do?

[JohnTortugo]

You can remove the file from the signing list. Create a file named Signing.props in the eng\ directory of your repo and use this to exclude the file from signing:

<Project>
    <ItemGroup>
        <FileSignInfo Include="<FileName>" CertificateName="None" />
    </ItemGroup>
</Project>

More documentation about the signing configuration [is available here], just in case. (https://github.com/dotnet/arcade/blob/master/Documentation/CorePackages/Signing.md)

You may also try setting the latest version of sign tool in Version.props:

    <MicrosoftDotNetSignToolVersion>1.0.0-beta.19067.6</MicrosoftDotNetSignToolVersion>

[morganbr]

@JohnTortugo , the issue is that it must be (authenticode) signed for the preview. We already have the workaround you've described to unblock our build.

[JohnTortugo]

I'll ping @morganbr on Teams.

[jashook]

We are hitting this issues again. /cc @RussKeldorph. This is currently blocking builds.

[jashook]

Example: https://dev.azure.com/dnceng/internal/_build/results?buildId=95917

[markwilkie]

Hmmmm, the signtool sources have not changed in a couple of weeks. Any chance something else changed?

[chcosta]

Looking

[sbomer]

We only recently (yesterday) got the latest signing package with https://github.com/dotnet/coreclr/pull/22544/files#diff-8b8f08ffbf7b863fb3700c1718eeb4cbL16, which removed an override we had in place (originally to get the latest signing package because arcade hadn't updated yet).

So #1917 might still be relevant.

[jashook]

We are currently building cleanly by using a fix old sign tool. See PR dotnet/coreclr#22609

[jashook]

/cc @hoyosjs

[chcosta]

It's not yet clear to me why the SignTool is applying the "SilverlightCert121" strongname to System.Private.CoreLib which is causing this failure.

[markwilkie]

Adding @tmat in case he has insight

[chcosta]

The publickeytoken for System.Private.CoreLib matches

arcade/src/Microsoft.DotNet.Arcade.Sdk/tools/Sign.proj

Line 22 in 9104c21

<StrongNameSignInfo Include="SilverlightCert121" PublicKeyToken="7cec85d7bea7798e" CertificateName="Microsoft400" />

which causes the signtool to default to using 'SilverlightCert121' for strongname signing.

It looks like all the changes in this area have been present for 4+ months. Has Coreclr ever signed correctly on Arcade (without the workaround)? If yes, then that's confusing.

[hoyosjs]

This is built from https://github.com/dotnet/coreclr/tree/fc70d035c7a633a63bc783303df01e50ff8c755f which already used Arcade signing using version 1.0.0-beta.19067.6 of the SignTool

[chcosta]

I need to dig in to the intent of how things are implemented to understand if adding <strongname>none</strongname> Metadata to the corelib sign item is the intended way to handle this. @tmat?

[chcosta]

I don't think specifying strongname as "none" is really how this works. Ignore my previous comment.

[chcosta]

This is the change that broke things - 2a8bae4#diff-4fff90b794ff979def515807c64d5fd9

[chcosta]

I believe that this is true

My guess would be that your change now accurately detects that S.P.CoreLib is managed, but doesn't notice that it's crossgenned (and thus still shouldn't run through SN). I'm not sure how to find it in the file, but crossgenned images have the IL_Library corflags bit set. The metadata reader API may be helpful.

@tmat, can you provide guidance on whether the design is that this should be fixed in the signtool or via

<Project>
    <ItemGroup>
        <FileSignInfo Include="<FileName>" CertificateName="None" />
    </ItemGroup>
</Project>

[tmat]

This is quite a long thread. What's the issue now?

[chcosta]

It's what I highlighted most recently.

that your change now accurately detects that S.P.CoreLib is managed, but doesn't notice that it's crossgenned (and thus still shouldn't run through SN). I'm not sure how to find it in the file, but crossgenned images have the IL_Library corflags bit set.

System.Private.CoreLib is now being detected as managed but not being flagged as "crossgen'd so don't strongname sign". Is the intention of this that the signtool would do the detection to determine that an assembly is managed and crossgen'd (so don't strongname sign), or that the repo would opt out of strongname signing for assemblies in this category?

[tmat]

We define default strong name and authenticode certicicates here:
https://github.com/dotnet/arcade/blob/master/src/Microsoft.DotNet.Arcade.Sdk/tools/Sign.proj#L20-L25

So I assume one of them kicks in for System.Private.CoreLib.

I think it would be useful if the SignTool detected crossgen'd images and skipped strong name signing automatically.

[chcosta]

One of the unit tests ("Crossgenerated") is explicitly checking for strong name signing of crossgened assemblies. Is this intentional? @natemcmaster , is AspNet strong name signing crossgened assemblies?

[natemcmaster]

is AspNet strong name signing crossgened assemblies?

The assemblies are strong named before we run crossgen. Our build does not run sn.exe or use delay-signing. IIRC crossgen moves the location of the strong name signature.

[markwilkie]

@chcosta - what's the latest?

[chcosta]

Waiting for code review of #2079. I pinged @tmat yesterday but he's a busy guy and I don't believe that anyone is blocked on this at the moment.