From a78e73c3828778d9bcd17bbba56ba165ab9120b7 Mon Sep 17 00:00:00 2001 From: pgavlin Date: Fri, 19 Dec 2014 17:40:40 -0800 Subject: [PATCH] Add public key validation to assembly name parsing. This is necessary to correctly reject invalid assembly names in InternalsVisibleTo and to match the native compiler. (changeset 1389827) --- .../Portable/Emitter/Model/SymbolAdapter.cs | 9 +- .../Symbols/Source/SourceAssemblySymbol.cs | 78 +++---- .../InternalsVisibleToAndStrongNameTests.cs | 19 ++ .../AssemblyIdentityDisplayNameTests.cs | 12 +- .../AssemblyIdentity.DisplayName.cs | 212 ++++++++++++++++-- .../InternalsVisibleToAndStrongNameTests.vb | 13 ++ 6 files changed, 271 insertions(+), 72 deletions(-) diff --git a/Src/Compilers/CSharp/Portable/Emitter/Model/SymbolAdapter.cs b/Src/Compilers/CSharp/Portable/Emitter/Model/SymbolAdapter.cs index b9f7143721f..a97aed9d9ec 100644 --- a/Src/Compilers/CSharp/Portable/Emitter/Model/SymbolAdapter.cs +++ b/Src/Compilers/CSharp/Portable/Emitter/Model/SymbolAdapter.cs @@ -111,9 +111,10 @@ internal IEnumerable GetCustomAttributesToEmit(ModuleCompil CSharpAttributeData attribute = userDefined[i]; if (this.Kind == SymbolKind.Assembly) { - // We need to filter out duplicate assembly attributes, - // i.e. attributes that bind to the same constructor and have identical arguments. - if (((SourceAssemblySymbol)this).IsIndexOfDuplicateAssemblyAttribute(i)) + // We need to filter out duplicate assembly attributes (i.e. attributes that + // bind to the same constructor and have identical arguments) and invalid + // InternalsVisibleTo attributes. + if (((SourceAssemblySymbol)this).IsIndexOfOmittedAssemblyAttribute(i)) { continue; } @@ -126,4 +127,4 @@ internal IEnumerable GetCustomAttributesToEmit(ModuleCompil } } } -} \ No newline at end of file +} diff --git a/Src/Compilers/CSharp/Portable/Symbols/Source/SourceAssemblySymbol.cs b/Src/Compilers/CSharp/Portable/Symbols/Source/SourceAssemblySymbol.cs index f293da8dca7..2d4ea737c46 100644 --- a/Src/Compilers/CSharp/Portable/Symbols/Source/SourceAssemblySymbol.cs +++ b/Src/Compilers/CSharp/Portable/Symbols/Source/SourceAssemblySymbol.cs @@ -67,12 +67,14 @@ internal sealed class SourceAssemblySymbol : MetadataOrSourceAssemblySymbol, IAt private IDictionary lazyForwardedTypesFromSource; /// - /// Indices of duplicate assembly attributes, i.e. attributes that bind to the same constructor and have identical arguments, that must not be emitted. + /// Indices of attributes that will not be emitted for one of two reasons: + /// - They are duplicates of another attribute (i.e. attributes that bind to the same constructor and have identical arguments) + /// - They are InternalsVisibleToAttributes with invalid assembly identities /// /// /// These indices correspond to the merged assembly attributes from source and added net modules, i.e. attributes returned by method. /// - private HashSet lazyDuplicateAttributeIndices; + private ConcurrentSet lazyOmittedAttributeIndices; private ThreeState lazyContainsExtensionMethods; @@ -1068,16 +1070,25 @@ private bool IsKnownAssemblyAttribute(CSharpAttributeData attribute) return false; } + private void AddOmittedAttributeIndex(int index) + { + if (this.lazyOmittedAttributeIndices == null) + { + Interlocked.CompareExchange(ref this.lazyOmittedAttributeIndices, new ConcurrentSet(), null); + } + + this.lazyOmittedAttributeIndices.Add(index); + } + /// /// Gets unique source assembly attributes that should be emitted, /// i.e. filters out attributes with errors and duplicate attributes. /// - private HashSet GetUniqueSourceAssemblyAttributes(out HashSet attributeIndicesToSkip) + private HashSet GetUniqueSourceAssemblyAttributes() { ImmutableArray appliedSourceAttributes = this.GetSourceAttributesBag().Attributes; HashSet uniqueAttributes = null; - attributeIndicesToSkip = null; for (int i = 0; i < appliedSourceAttributes.Length; i++) { @@ -1086,12 +1097,7 @@ private HashSet GetUniqueSourceAssemblyAttributes(out HashS { if (!AddUniqueAssemblyAttribute(attribute, ref uniqueAttributes)) { - if (attributeIndicesToSkip == null) - { - attributeIndicesToSkip = new HashSet(); - } - - attributeIndicesToSkip.Add(i); + AddOmittedAttributeIndex(i); } } } @@ -1205,8 +1211,7 @@ private ImmutableArray GetNetModuleAttributes(out Immutable private WellKnownAttributeData ValidateAttributeUsageAndDecodeWellKnownAttributes( ImmutableArray attributesFromNetModules, ImmutableArray netModuleNames, - DiagnosticBag diagnostics, - out HashSet attributeIndicesToSkip) + DiagnosticBag diagnostics) { Debug.Assert(attributesFromNetModules.Any()); Debug.Assert(netModuleNames.Any()); @@ -1218,7 +1223,7 @@ private ImmutableArray GetNetModuleAttributes(out Immutable int sourceAttributesCount = this.GetSourceAttributesBag().Attributes.Length; // Get unique source assembly attributes. - HashSet uniqueAttributes = GetUniqueSourceAssemblyAttributes(out attributeIndicesToSkip); + HashSet uniqueAttributes = GetUniqueSourceAssemblyAttributes(); var arguments = new DecodeWellKnownAttributeArguments(); arguments.AttributesCount = netModuleAttributesCount; @@ -1230,6 +1235,8 @@ private ImmutableArray GetNetModuleAttributes(out Immutable // That is why we are iterating attributes backwards. for (int i = netModuleAttributesCount - 1; i >= 0; i--) { + var totalIndex = i + sourceAttributesCount; + CSharpAttributeData attribute = attributesFromNetModules[i]; if (!attribute.HasErrors && ValidateAttributeUsageForNetModuleAttribute(attribute, netModuleNames[i], diagnostics, ref uniqueAttributes)) { @@ -1239,16 +1246,11 @@ private ImmutableArray GetNetModuleAttributes(out Immutable // CONSIDER: Provide usable AttributeSyntax node for diagnostics of malformed netmodule assembly attributes arguments.AttributeSyntaxOpt = null; - this.DecodeWellKnownAttribute(ref arguments, isFromNetModule: true); + this.DecodeWellKnownAttribute(ref arguments, totalIndex, isFromNetModule: true); } else { - if (attributeIndicesToSkip == null) - { - attributeIndicesToSkip = new HashSet(); - } - - attributeIndicesToSkip.Add(i + sourceAttributesCount); + AddOmittedAttributeIndex(totalIndex); } } @@ -1257,13 +1259,8 @@ private ImmutableArray GetNetModuleAttributes(out Immutable private void LoadAndValidateNetModuleAttributes(ref CustomAttributesBag lazyNetModuleAttributesBag) { - // Indices of duplicate assembly attributes, i.e. attributes that bind to the same constructor and have identical arguments, that must not be emitted. - HashSet attributeIndicesToSkip; - if (compilation.Options.OutputKind.IsNetModule()) { - // Compute duplicate source assembly attributes, i.e. attributes with same constructor and arguments, that must not be emitted. - var unused = GetUniqueSourceAssemblyAttributes(out attributeIndicesToSkip); Interlocked.CompareExchange(ref lazyNetModuleAttributesBag, CustomAttributesBag.Empty, null); } else @@ -1277,12 +1274,12 @@ private void LoadAndValidateNetModuleAttributes(ref CustomAttributesBag.Empty; } - // Check if we have any duplicate assembly attribute that must not be emitted, - // unless we are emitting a net module. - if (attributeIndicesToSkip != null) - { - Debug.Assert(attributeIndicesToSkip.Any()); - Interlocked.CompareExchange(ref lazyDuplicateAttributeIndices, attributeIndicesToSkip, null); - } - if (Interlocked.CompareExchange(ref lazyNetModuleAttributesBag, netModuleAttributesBag, null) == null) { this.AddSemanticDiagnostics(diagnostics); } diagnostics.Free(); - Debug.Assert(attributeIndicesToSkip == null || - !attributeIndicesToSkip.Any((index) => index < 0 || index >= this.GetAttributes().Length)); } Debug.Assert(lazyNetModuleAttributesBag.IsSealed); @@ -1454,16 +1441,15 @@ public sealed override ImmutableArray GetAttributes() /// /// This method must be invoked only after all the assembly attributes have been bound. /// - internal bool IsIndexOfDuplicateAssemblyAttribute(int index) + internal bool IsIndexOfOmittedAssemblyAttribute(int index) { + Debug.Assert(this.lazyOmittedAttributeIndices == null || !lazyOmittedAttributeIndices.Any(i => i < 0 || i >= this.GetAttributes().Length)); Debug.Assert(this.lazySourceAttributesBag.IsSealed); Debug.Assert(this.lazyNetModuleAttributesBag.IsSealed); Debug.Assert(index >= 0); Debug.Assert(index < this.GetAttributes().Length); - Debug.Assert(this.lazyDuplicateAttributeIndices == null || - !this.DeclaringCompilation.Options.OutputKind.IsNetModule()); - return this.lazyDuplicateAttributeIndices != null && this.lazyDuplicateAttributeIndices.Contains(index); + return this.lazyOmittedAttributeIndices != null && this.lazyOmittedAttributeIndices.Contains(index); } /// @@ -1948,10 +1934,11 @@ private void DecodeTypeForwardedToAttribute(ref DecodeWellKnownAttributeArgument } } - private static void DecodeOneInternalsVisibleToAttribute( + private void DecodeOneInternalsVisibleToAttribute( AttributeSyntax nodeOpt, CSharpAttributeData attrData, DiagnosticBag diagnostics, + int index, ref ConcurrentDictionary, Tuple>> lazyInternalsVisibleToMap) { // this code won't be called unless we bound a well-formed, semantically correct ctor call. @@ -1970,6 +1957,7 @@ private void DecodeTypeForwardedToAttribute(ref DecodeWellKnownAttributeArgument if (!AssemblyIdentity.TryParseDisplayName(displayName, out identity, out parts)) { diagnostics.Add(ErrorCode.WRN_InvalidAssemblyName, GetAssemblyAttributeLocationForDiagnostic(nodeOpt), displayName); + AddOmittedAttributeIndex(index); return; } @@ -2037,10 +2025,10 @@ AttributeLocation IAttributeTargetSymbol.AllowedAttributeLocations internal override void DecodeWellKnownAttribute(ref DecodeWellKnownAttributeArguments arguments) { - DecodeWellKnownAttribute(ref arguments, isFromNetModule: false); + DecodeWellKnownAttribute(ref arguments, arguments.Index, isFromNetModule: false); } - private void DecodeWellKnownAttribute(ref DecodeWellKnownAttributeArguments arguments, bool isFromNetModule) + private void DecodeWellKnownAttribute(ref DecodeWellKnownAttributeArguments arguments, int index, bool isFromNetModule) { var attribute = arguments.Attribute; Debug.Assert(!attribute.HasErrors); @@ -2049,7 +2037,7 @@ private void DecodeWellKnownAttribute(ref DecodeWellKnownAttributeArguments + { + var assembly = module.ContainingAssembly; + Assert.NotNull(assembly); + Assert.False(assembly.GetAttributes().Any(attr => attr.IsTargetAttribute(assembly, AttributeDescription.InternalsVisibleToAttribute))); + }); + } + #endregion } diff --git a/Src/Compilers/Core/CodeAnalysisTest/MetadataReferences/AssemblyIdentityDisplayNameTests.cs b/Src/Compilers/Core/CodeAnalysisTest/MetadataReferences/AssemblyIdentityDisplayNameTests.cs index 522b30cd83f..5f519618e45 100644 --- a/Src/Compilers/Core/CodeAnalysisTest/MetadataReferences/AssemblyIdentityDisplayNameTests.cs +++ b/Src/Compilers/Core/CodeAnalysisTest/MetadataReferences/AssemblyIdentityDisplayNameTests.cs @@ -527,10 +527,14 @@ public void TryParseDisplayName_Keys() TestParseDisplayName("foo, PublicKeyToken=111111111111111, Version=1.0.0.1", null); TestParseDisplayName("foo, PublicKeyToken=1111111111111111111, Version=1.0.0.1", null); TestParseDisplayName("foo, PublicKey=1, Version=1.0.0.1", null); - - // TODO: fusion doesn't accept the key - //TestParseDisplayName("foo, PublicKey=11, Version=1.0.0.1", - // new AssemblyIdentity("foo", new Version(1, 0, 0, 1), publicKeyOrToken: new byte[] { 0x11 }.AsImmutable(), hasPublicKey: true)); + TestParseDisplayName("foo, PublicKey=1000000040000000", null); + TestParseDisplayName("foo, PublicKey=11, Version=1.0.0.1", null); + + // TODO: need to calculate the correct token for the ECMA key. + // TestParseDisplayName("foo, PublicKey=0000000040000000", + // expectedParts: 0, + // expectedFusion: null, // Fusion rejects the ECMA key. + // expected: new AssemblyIdentity("foo", hasPublicKey: true, publicKeyOrToken: new byte[] { 0, 0, 0, 0, 0, 0, 0, 0, 4, 0, 0, 0, 0, 0, 0, 0 }.AsImmutable())); // if public key token calculated from public key matches, then it's ok to specify both TestParseDisplayName("foo, Culture=neutral, Version=1.0.0.0, PublicKey=" + StrPublicKey1 + ", PublicKeyToken=" + StrPublicKeyToken1, diff --git a/Src/Compilers/Core/Portable/MetadataReference/AssemblyIdentity.DisplayName.cs b/Src/Compilers/Core/Portable/MetadataReference/AssemblyIdentity.DisplayName.cs index e5ed499915f..7745d41777f 100644 --- a/Src/Compilers/Core/Portable/MetadataReference/AssemblyIdentity.DisplayName.cs +++ b/Src/Compilers/Core/Portable/MetadataReference/AssemblyIdentity.DisplayName.cs @@ -5,6 +5,7 @@ using System.Diagnostics; using System.Linq; using System.Reflection; +using System.Runtime.InteropServices; using System.Text; using Microsoft.CodeAnalysis.Collections; using Roslyn.Utilities; @@ -275,12 +276,16 @@ public static bool TryParseDisplayName(string displayName, out AssemblyIdentity continue; } - ImmutableArray value = ParseKey(propertyValue); - if (value.Length == 0) + ImmutableArray value; + if (!TryParsePublicKey(propertyValue, out value)) { return false; } + // NOTE: Fusion would also set the public key token (as derived from the public key) here. + // We may need to do this as well for error cases, as Fusion would fail to parse the + // assembly name if public key token calculation failed. + publicKey = value; parsedParts |= AssemblyIdentityParts.PublicKey; } @@ -299,18 +304,9 @@ public static bool TryParseDisplayName(string displayName, out AssemblyIdentity } ImmutableArray value; - if (string.Equals(propertyValue, "null", StringComparison.OrdinalIgnoreCase) || - string.Equals(propertyValue, "neutral", StringComparison.OrdinalIgnoreCase)) + if (!TryParsePublicKeyToken(propertyValue, out value)) { - value = ImmutableArray.Create(); - } - else - { - value = ParseKey(propertyValue); - if (value.Length != PublicKeyTokenSize) - { - return false; - } + return false; } publicKeyToken = value; @@ -615,23 +611,201 @@ internal static bool TryParseVersion(string str, out ulong result, out AssemblyI } } - private static ImmutableArray ParseKey(string value) + private static class PublicKeyDecoder { - byte[] result = new byte[value.Length / 2]; - for (int i = 0; i < result.Length; i++) + private enum AlgorithmClass + { + Signature = 1, + Hash = 4, + } + + private enum AlgorithmSubId + { + Sha1Hash = 4, + MacHash = 5, + RipeMdHash = 6, + RipeMd160Hash = 7, + Ssl3ShaMD5Hash = 8, + HmacHash = 9, + Tls1PrfHash = 10, + HashReplacOwfHash = 11, + Sha256Hash = 12, + Sha384Hash = 13, + Sha512Hash = 14, + } + + private struct AlgorithmId + { + // From wincrypt.h + private const int AlgorithmClassOffset = 13; + private const int AlgorithmClassMask = 0x7; + private const int AlgorithmSubIdOffset = 0; + private const int AlgorithmSubIdMask = 0x1ff; + + private readonly uint flags; + + public bool IsSet + { + get { return flags != 0; } + } + + public AlgorithmClass Class + { + get { return (AlgorithmClass)((flags >> AlgorithmClassOffset) & AlgorithmClassMask); } + } + + public AlgorithmSubId SubId + { + get { return (AlgorithmSubId)((flags >> AlgorithmSubIdOffset) & AlgorithmSubIdMask); } + } + + public AlgorithmId(uint flags) + { + this.flags = flags; + } + } + + // From ECMAKey.h + private static readonly ImmutableArray ecmaKey = ImmutableArray.Create(new byte[] { 0, 0, 0, 0, 0, 0, 0, 0, 4, 0, 0, 0, 0, 0, 0, 0 }); + + // From strongname.h + [StructLayout(LayoutKind.Sequential, Pack = 0)] + private unsafe struct PublicKeyHeader + { + public const int SigAlgIdOffset = 0; + public const int HashAlgIdOffset = SigAlgIdOffset + sizeof(uint); + public const int PublicKeySizeOffset = HashAlgIdOffset + sizeof(uint); + public const int PublicKeyDataOffset = PublicKeySizeOffset + sizeof(uint); + public const int Size = PublicKeyDataOffset; + + uint SigAlgId; + uint HashAlgId; + uint PublicKeySize; + } + + // From wincrypt.h + private const byte PublicKeyBlob = 0x06; + + // From StrongNameInternal.cpp + public static bool TryDecode(byte[] bytes, out ImmutableArray key) + { + // The number of public key bytes must be at least large enough for the header and one byte of data. + if (bytes.Length < PublicKeyHeader.Size + 1) + { + key = default(ImmutableArray); + return false; + } + + // The number of public key bytes must be the same as the size of the header plus the size of the public key data. + var dataSize = (uint)BitConverter.ToInt32(bytes, PublicKeyHeader.PublicKeySizeOffset); + if (bytes.Length != PublicKeyHeader.Size + dataSize) + { + key = default(ImmutableArray); + return false; + } + + // Check for ECMA key + if (bytes.Length == ecmaKey.Length) + { + for (int i = 0; i < bytes.Length; i++) + { + if (bytes[i] != ecmaKey[i]) + { + goto notEcmaKey; + } + } + + key = ecmaKey; + return true; + } + + notEcmaKey: + var signatureAlgorithmId = new AlgorithmId((uint)BitConverter.ToInt32(bytes, 0)); + if (signatureAlgorithmId.IsSet && signatureAlgorithmId.Class != AlgorithmClass.Signature) + { + key = default(ImmutableArray); + return false; + } + + var hashAlgorithmId = new AlgorithmId((uint)BitConverter.ToInt32(bytes, 4)); + if (hashAlgorithmId.IsSet && (hashAlgorithmId.Class != AlgorithmClass.Hash || hashAlgorithmId.SubId < AlgorithmSubId.Sha1Hash)) + { + key = default(ImmutableArray); + return false; + } + + if (bytes[PublicKeyHeader.PublicKeyDataOffset] != PublicKeyBlob) + { + key = default(ImmutableArray); + return false; + } + + key = bytes.AsImmutable(); + return true; + } + } + + const int MaxPublicKeyBytes = 2048; + + private static bool TryParsePublicKey(string value, out ImmutableArray key) + { + byte[] result; + if (value.Length > (MaxPublicKeyBytes * 2) || !TryParseHexBytes(value, out result)) + { + key = default(ImmutableArray); + return false; + } + + return PublicKeyDecoder.TryDecode(result, out key); + } + + const int PublicKeyTokenBytes = 8; + + private static bool TryParsePublicKeyToken(string value, out ImmutableArray token) + { + if (string.Equals(value, "null", StringComparison.OrdinalIgnoreCase) || + string.Equals(value, "neutral", StringComparison.OrdinalIgnoreCase)) + { + token = ImmutableArray.Empty; + return true; + } + + byte[] result; + if (value.Length != (PublicKeyTokenBytes * 2) || !TryParseHexBytes(value, out result)) + { + token = default(ImmutableArray); + return false; + } + + token = result.AsImmutable(); + return true; + } + + private static bool TryParseHexBytes(string value, out byte[] result) + { + if (value.Length == 0 || (value.Length % 2) != 0) + { + result = null; + return false; + } + + var bytes = new byte[value.Length / 2]; + for (int i = 0; i < bytes.Length; i++) { int hi = HexValue(value[i * 2]); int lo = HexValue(value[i * 2 + 1]); if (hi < 0 || lo < 0) { - return ImmutableArray.Create(); + result = null; + return false; } - result[i] = (byte)((hi << 4) | lo); + bytes[i] = (byte)((hi << 4) | lo); } - return result.AsImmutable(); + result = bytes; + return true; } internal static int HexValue(char c) diff --git a/Src/Compilers/VisualBasic/Test/Emit/Attributes/InternalsVisibleToAndStrongNameTests.vb b/Src/Compilers/VisualBasic/Test/Emit/Attributes/InternalsVisibleToAndStrongNameTests.vb index 8d141c91033..a725adef08e 100644 --- a/Src/Compilers/VisualBasic/Test/Emit/Attributes/InternalsVisibleToAndStrongNameTests.vb +++ b/Src/Compilers/VisualBasic/Test/Emit/Attributes/InternalsVisibleToAndStrongNameTests.vb @@ -1616,4 +1616,17 @@ End Class]]> CompileAndVerify(cb, expectedOutput:="42", emitOptions:=TestEmitters.CCI).Diagnostics.Verify() End Sub + + Public Sub Bug1095618() + Dim source As XElement = _ + + + ]]> + + + CreateCompilationWithMscorlib(source).VerifyDiagnostics( + Diagnostic(ERRID.ERR_FriendAssemblyNameInvalid, "Assembly: System.Runtime.CompilerServices.InternalsVisibleTo(""System.Runtime.Serialization, PublicKey = 10000000000000000400000000000000"")").WithArguments("System.Runtime.Serialization, PublicKey = 10000000000000000400000000000000").WithLocation(1, 2)) + End Sub + End Class -- GitLab