diff --git a/RELEASE-NOTES.md b/RELEASE-NOTES.md index cd285abe7..be4882f1f 100644 --- a/RELEASE-NOTES.md +++ b/RELEASE-NOTES.md @@ -40,6 +40,7 @@ END TEMPLATE--> ### New features * `System.Collections.IList` and `System.Collections.ICollection` are now sandbox safe, this fixes some collection expression cases. +* The sandboxing system will now report the methods responsible for references to illegal items. ### Bugfixes diff --git a/Robust.Shared/ContentPack/AssemblyTypeChecker.ReportBadReferences.cs b/Robust.Shared/ContentPack/AssemblyTypeChecker.ReportBadReferences.cs new file mode 100644 index 000000000..07dd839b2 --- /dev/null +++ b/Robust.Shared/ContentPack/AssemblyTypeChecker.ReportBadReferences.cs @@ -0,0 +1,442 @@ +#if TOOLS + +using System; +using System.Buffers.Binary; +using System.Collections.Generic; +using System.IO; +using System.Linq; +using System.Reflection.Metadata; +using System.Reflection.PortableExecutable; +using System.Runtime.CompilerServices; + +namespace Robust.Shared.ContentPack; + +internal sealed partial class AssemblyTypeChecker +{ + // This part of the code tries to find the originator of bad sandbox references. + + private void ReportBadReferences(PEReader peReader, MetadataReader reader, IEnumerable reference) + { + _sawmill.Info("Started search for originator of bad references..."); + + var refs = reference.ToHashSet(); + ExpandReferences(reader, refs); + + foreach (var methodDefHandle in reader.MethodDefinitions) + { + var methodDef = reader.GetMethodDefinition(methodDefHandle); + if (methodDef.RelativeVirtualAddress == 0) + continue; + + var methodName = reader.GetString(methodDef.Name); + + var body = peReader.GetMethodBody(methodDef.RelativeVirtualAddress); + var bytes = body.GetILBytes()!; + + var ilReader = new ILReader(bytes); + var prefPosition = 0; + while (ilReader.MoveNext(out var instruction)) + { + if (instruction.TryGetEntityHandle(out var handle)) + { + if (refs.Contains(handle)) + { + var type = GetTypeFromDefinition(reader, methodDef.GetDeclaringType()); + _sawmill.Error( + $"Found reference to {DisplayHandle(reader, handle)} in method {type}.{methodName} at IL 0x{prefPosition:X4}"); + } + } + + prefPosition = ilReader.Position; + } + } + } + + private static string DisplayHandle(MetadataReader reader, EntityHandle handle) + { + switch (handle.Kind) + { + case HandleKind.MemberReference: + var memberRef = reader.GetMemberReference((MemberReferenceHandle)handle); + var name = reader.GetString(memberRef.Name); + var parent = DisplayHandle(reader, memberRef.Parent); + return $"{parent}.{name}"; + + case HandleKind.TypeReference: + return $"{ParseTypeReference(reader, (TypeReferenceHandle)handle)}"; + + case HandleKind.TypeSpecification: + var typeSpec = reader.GetTypeSpecification((TypeSpecificationHandle)handle); + var provider = new TypeProvider(); + var type = typeSpec.DecodeSignature(provider, 0); + return $"{type}"; + + default: + return $"({handle.Kind} handle)"; + } + } + + private static void ExpandReferences(MetadataReader reader, HashSet handles) + { + var toAdd = new List(); + + foreach (var memberRefHandle in reader.MemberReferences) + { + var memberRef = reader.GetMemberReference(memberRefHandle); + if (handles.Contains(memberRef.Parent)) + { + toAdd.Add(memberRefHandle); + } + } + + handles.UnionWith(toAdd); + } + + private readonly struct ILInstruction + { + public readonly ILOpCode OpCode; + public readonly long Argument; + public readonly int[]? SwitchTargets; + + public ILInstruction(ILOpCode opCode) + { + OpCode = opCode; + } + + public ILInstruction(ILOpCode opCode, long argument) + { + OpCode = opCode; + Argument = argument; + } + + public ILInstruction(ILOpCode opCode, long argument, int[] switchTargets) + { + OpCode = opCode; + Argument = argument; + SwitchTargets = switchTargets; + } + + public bool TryGetEntityHandle(out EntityHandle handle) + { + switch (OpCode) + { + case ILOpCode.Call: + case ILOpCode.Callvirt: + case ILOpCode.Newobj: + case ILOpCode.Jmp: + case ILOpCode.Box: + case ILOpCode.Castclass: + case ILOpCode.Cpobj: + case ILOpCode.Initobj: + case ILOpCode.Isinst: + case ILOpCode.Ldelem: + case ILOpCode.Ldelema: + case ILOpCode.Ldfld: + case ILOpCode.Ldflda: + case ILOpCode.Ldobj: + case ILOpCode.Ldstr: + case ILOpCode.Ldtoken: + case ILOpCode.Ldvirtftn: + case ILOpCode.Mkrefany: + case ILOpCode.Newarr: + case ILOpCode.Refanyval: + case ILOpCode.Sizeof: + case ILOpCode.Stelem: + case ILOpCode.Stfld: + case ILOpCode.Stobj: + case ILOpCode.Stsfld: + case ILOpCode.Throw: + case ILOpCode.Unbox_any: + handle = Unsafe.BitCast((int)Argument); + return true; + + default: + handle = default; + return false; + } + } + } + + private sealed class ILReader(byte[] body) + { + public int Position; + + public bool MoveNext(out ILInstruction instruction) + { + if (Position >= body.Length) + { + instruction = default; + return false; + } + + var firstByte = body[Position++]; + var opCode = (ILOpCode)firstByte; + if (firstByte == 0xFE) + opCode = 0xFE00 + (ILOpCode)body[Position++]; + + switch (opCode) + { + // no args. + case ILOpCode.Readonly: + case ILOpCode.Tail: + case ILOpCode.Volatile: + case ILOpCode.Add: + case ILOpCode.Add_ovf: + case ILOpCode.Add_ovf_un: + case ILOpCode.And: + case ILOpCode.Arglist: + case ILOpCode.Break: + case ILOpCode.Ceq: + case ILOpCode.Cgt: + case ILOpCode.Cgt_un: + case ILOpCode.Ckfinite: + case ILOpCode.Clt: + case ILOpCode.Clt_un: + case ILOpCode.Conv_i1: + case ILOpCode.Conv_i2: + case ILOpCode.Conv_i4: + case ILOpCode.Conv_i8: + case ILOpCode.Conv_r4: + case ILOpCode.Conv_r8: + case ILOpCode.Conv_u1: + case ILOpCode.Conv_u2: + case ILOpCode.Conv_u4: + case ILOpCode.Conv_u8: + case ILOpCode.Conv_i: + case ILOpCode.Conv_u: + case ILOpCode.Conv_r_un: + case ILOpCode.Conv_ovf_i1: + case ILOpCode.Conv_ovf_i2: + case ILOpCode.Conv_ovf_i4: + case ILOpCode.Conv_ovf_i8: + case ILOpCode.Conv_ovf_u4: + case ILOpCode.Conv_ovf_u8: + case ILOpCode.Conv_ovf_i: + case ILOpCode.Conv_ovf_u: + case ILOpCode.Conv_ovf_i1_un: + case ILOpCode.Conv_ovf_i2_un: + case ILOpCode.Conv_ovf_i4_un: + case ILOpCode.Conv_ovf_i8_un: + case ILOpCode.Conv_ovf_u4_un: + case ILOpCode.Conv_ovf_u8_un: + case ILOpCode.Conv_ovf_i_un: + case ILOpCode.Conv_ovf_u_un: + case ILOpCode.Cpblk: + case ILOpCode.Div: + case ILOpCode.Div_un: + case ILOpCode.Dup: + case ILOpCode.Endfilter: + case ILOpCode.Endfinally: + case ILOpCode.Initblk: + case ILOpCode.Ldarg_0: + case ILOpCode.Ldarg_1: + case ILOpCode.Ldarg_2: + case ILOpCode.Ldarg_3: + case ILOpCode.Ldc_i4_0: + case ILOpCode.Ldc_i4_1: + case ILOpCode.Ldc_i4_2: + case ILOpCode.Ldc_i4_3: + case ILOpCode.Ldc_i4_4: + case ILOpCode.Ldc_i4_5: + case ILOpCode.Ldc_i4_6: + case ILOpCode.Ldc_i4_7: + case ILOpCode.Ldc_i4_8: + case ILOpCode.Ldc_i4_m1: + case ILOpCode.Ldind_i1: + case ILOpCode.Ldind_u1: + case ILOpCode.Ldind_i2: + case ILOpCode.Ldind_u2: + case ILOpCode.Ldind_i4: + case ILOpCode.Ldind_u4: + case ILOpCode.Ldind_i8: + case ILOpCode.Ldind_i: + case ILOpCode.Ldind_r4: + case ILOpCode.Ldind_r8: + case ILOpCode.Ldind_ref: + case ILOpCode.Ldloc_0: + case ILOpCode.Ldloc_1: + case ILOpCode.Ldloc_2: + case ILOpCode.Ldloc_3: + case ILOpCode.Ldnull: + case ILOpCode.Localloc: + case ILOpCode.Mul: + case ILOpCode.Mul_ovf: + case ILOpCode.Mul_ovf_un: + case ILOpCode.Neg: + case ILOpCode.Nop: + case ILOpCode.Not: + case ILOpCode.Or: + case ILOpCode.Pop: + case ILOpCode.Rem: + case ILOpCode.Rem_un: + case ILOpCode.Ret: + case ILOpCode.Shl: + case ILOpCode.Shr: + case ILOpCode.Shr_un: + case ILOpCode.Stind_i1: + case ILOpCode.Stind_i2: + case ILOpCode.Stind_i4: + case ILOpCode.Stind_i8: + case ILOpCode.Stind_r4: + case ILOpCode.Stind_r8: + case ILOpCode.Stind_i: + case ILOpCode.Stind_ref: + case ILOpCode.Stloc_0: + case ILOpCode.Stloc_1: + case ILOpCode.Stloc_2: + case ILOpCode.Stloc_3: + case ILOpCode.Sub: + case ILOpCode.Sub_ovf: + case ILOpCode.Sub_ovf_un: + case ILOpCode.Xor: + case ILOpCode.Ldelem_i1: + case ILOpCode.Ldelem_u1: + case ILOpCode.Ldelem_i2: + case ILOpCode.Ldelem_u2: + case ILOpCode.Ldelem_i4: + case ILOpCode.Ldelem_u4: + case ILOpCode.Ldelem_i8: + case ILOpCode.Ldelem_i: + case ILOpCode.Ldelem_r4: + case ILOpCode.Ldelem_r8: + case ILOpCode.Ldelem_ref: + case ILOpCode.Ldlen: + case ILOpCode.Refanytype: + case ILOpCode.Rethrow: + case ILOpCode.Stelem_i1: + case ILOpCode.Stelem_i2: + case ILOpCode.Stelem_i4: + case ILOpCode.Stelem_i8: + case ILOpCode.Stelem_i: + case ILOpCode.Stelem_r4: + case ILOpCode.Stelem_r8: + case ILOpCode.Stelem_ref: + case ILOpCode.Throw: + instruction = new ILInstruction(opCode); + break; + + // 1-byte arg. + case ILOpCode.Unaligned: + case ILOpCode.Beq_s: + case ILOpCode.Bge_s: + case ILOpCode.Bge_un_s: + case ILOpCode.Bgt_s: + case ILOpCode.Bgt_un_s: + case ILOpCode.Ble_s: + case ILOpCode.Ble_un_s: + case ILOpCode.Blt_s: + case ILOpCode.Blt_un_s: + case ILOpCode.Bne_un_s: + case ILOpCode.Br_s: + case ILOpCode.Brfalse_s: + case ILOpCode.Brtrue_s: + case ILOpCode.Ldarg_s: + case ILOpCode.Ldarga_s: + case ILOpCode.Ldc_i4_s: + case ILOpCode.Ldloc_s: + case ILOpCode.Ldloca_s: + case ILOpCode.Leave_s: + case ILOpCode.Starg_s: + case ILOpCode.Stloc_s: + instruction = new ILInstruction(opCode, body[Position]); + Position += 1; + break; + + // 2-byte value + case ILOpCode.Ldarg: + case ILOpCode.Ldarga: + case ILOpCode.Ldloc: + case ILOpCode.Ldloca: + case ILOpCode.Starg: + case ILOpCode.Stloc: + var shortValue = BinaryPrimitives.ReadInt16LittleEndian(body.AsSpan(Position, 2)); + Position += 2; + instruction = new ILInstruction(opCode, shortValue); + break; + + // 4-byte value + case ILOpCode.Constrained: + case ILOpCode.Beq: + case ILOpCode.Bge: + case ILOpCode.Bge_un: + case ILOpCode.Bgt: + case ILOpCode.Bgt_un: + case ILOpCode.Ble: + case ILOpCode.Ble_un: + case ILOpCode.Blt: + case ILOpCode.Blt_un: + case ILOpCode.Bne_un: + case ILOpCode.Br: + case ILOpCode.Brfalse: + case ILOpCode.Brtrue: + case ILOpCode.Call: + case ILOpCode.Calli: + case ILOpCode.Jmp: + case ILOpCode.Ldc_i4: + case ILOpCode.Ldc_r4: + case ILOpCode.Ldftn: + case ILOpCode.Leave: + case ILOpCode.Box: + case ILOpCode.Callvirt: + case ILOpCode.Castclass: + case ILOpCode.Cpobj: + case ILOpCode.Initobj: + case ILOpCode.Isinst: + case ILOpCode.Ldelem: + case ILOpCode.Ldelema: + case ILOpCode.Ldfld: + case ILOpCode.Ldflda: + case ILOpCode.Ldobj: + case ILOpCode.Ldsfld: + case ILOpCode.Ldsflda: + case ILOpCode.Ldstr: + case ILOpCode.Ldtoken: + case ILOpCode.Ldvirtftn: + case ILOpCode.Mkrefany: + case ILOpCode.Newarr: + case ILOpCode.Newobj: + case ILOpCode.Refanyval: + case ILOpCode.Sizeof: + case ILOpCode.Stelem: + case ILOpCode.Stfld: + case ILOpCode.Stobj: + case ILOpCode.Stsfld: + case ILOpCode.Unbox: + case ILOpCode.Unbox_any: + var intValue = BinaryPrimitives.ReadInt32LittleEndian(body.AsSpan(Position, 4)); + Position += 4; + instruction = new ILInstruction(opCode, intValue); + break; + + // 8-byte value + case ILOpCode.Ldc_i8: + case ILOpCode.Ldc_r8: + var longValue = BinaryPrimitives.ReadInt64LittleEndian(body.AsSpan(Position, 8)); + Position += 8; + instruction = new ILInstruction(opCode, longValue); + break; + + // Switch + case ILOpCode.Switch: + var switchLength = BinaryPrimitives.ReadInt32LittleEndian(body.AsSpan(Position, 4)); + Position += 4; + var switchArgs = new int[switchLength]; + for (var i = 0; i < switchLength; i++) + { + switchArgs[i] = BinaryPrimitives.ReadInt32LittleEndian(body.AsSpan(Position, 4)); + Position += 4; + } + + instruction = new ILInstruction(opCode, switchLength, switchArgs); + break; + + default: + throw new InvalidDataException($"Unknown opcode: {opCode}"); + } + + return true; + } + } +} + +#endif diff --git a/Robust.Shared/ContentPack/AssemblyTypeChecker.cs b/Robust.Shared/ContentPack/AssemblyTypeChecker.cs index 5abc486cf..e00ba9f9c 100644 --- a/Robust.Shared/ContentPack/AssemblyTypeChecker.cs +++ b/Robust.Shared/ContentPack/AssemblyTypeChecker.cs @@ -148,7 +148,7 @@ namespace Robust.Shared.ContentPack if ((Dump & DumpFlags.Types) != 0) { - foreach (var mType in types) + foreach (var (_, mType) in types) { _sawmill.Debug($"RefType: {mType}"); } @@ -156,7 +156,7 @@ namespace Robust.Shared.ContentPack if ((Dump & DumpFlags.Members) != 0) { - foreach (var memberRef in members) + foreach (var (_, memberRef) in members) { _sawmill.Debug($"RefMember: {memberRef}"); } @@ -183,14 +183,17 @@ namespace Robust.Shared.ContentPack var loadedConfig = _config.Result; #pragma warning restore RA0004 + var badRefs = new ConcurrentBag(); + // We still do explicit type reference scanning, even though the actual whitelists work with raw members. // This is so that we can simplify handling of generic type specifications during member checking: // we won't have to check that any types in their type arguments are whitelisted. - foreach (var type in types) + foreach (var (handle, type) in types) { if (!IsTypeAccessAllowed(loadedConfig, type, out _)) { errors.Add(new SandboxError($"Access to type not allowed: {type}")); + badRefs.Add(handle); } } @@ -208,13 +211,20 @@ namespace Robust.Shared.ContentPack _sawmill.Debug($"Type abuse... {fullStopwatch.ElapsedMilliseconds}ms"); - CheckMemberReferences(loadedConfig, members, errors); + CheckMemberReferences(loadedConfig, members, errors, badRefs); foreach (var error in errors) { _sawmill.Error($"Sandbox violation: {error.Message}"); } +#if TOOLS + if (!badRefs.IsEmpty) + { + ReportBadReferences(peReader, reader, badRefs); + } +#endif + _sawmill.Debug($"Checked assembly in {fullStopwatch.ElapsedMilliseconds}ms"); return errors.IsEmpty; @@ -351,11 +361,13 @@ namespace Robust.Shared.ContentPack private void CheckMemberReferences( SandboxConfig sandboxConfig, - List members, - ConcurrentBag errors) + List<(MemberReferenceHandle handle, MMemberRef parsed)> members, + ConcurrentBag errors, + ConcurrentBag badReferences) { - Parallel.ForEach(members, memberRef => + Parallel.ForEach(members, entry => { + var (handle, memberRef) = entry; MType baseType = memberRef.ParentType; while (!(baseType is MTypeReferenced)) { @@ -416,6 +428,7 @@ namespace Robust.Shared.ContentPack } errors.Add(new SandboxError($"Access to field not allowed: {mMemberRefField}")); + badReferences.Add(handle); break; } case MMemberRefMethod mMemberRefMethod: @@ -444,6 +457,7 @@ namespace Robust.Shared.ContentPack } errors.Add(new SandboxError($"Access to method not allowed: {mMemberRefMethod}")); + badReferences.Add(handle); break; default: throw new ArgumentOutOfRangeException(nameof(memberRef)); @@ -458,18 +472,18 @@ namespace Robust.Shared.ContentPack { // This inheritance whitelisting primarily serves to avoid content doing funny stuff // by e.g. inheriting Type. - foreach (var (_, baseType, interfaces) in inherited) + foreach (var (type, baseType, interfaces) in inherited) { if (!CanInherit(baseType)) { - errors.Add(new SandboxError($"Inheriting of type not allowed: {baseType}")); + errors.Add(new SandboxError($"Inheriting of type not allowed: {baseType} (by {type})")); } foreach (var @interface in interfaces) { if (!CanInherit(@interface)) { - errors.Add(new SandboxError($"Implementing of interface not allowed: {@interface}")); + errors.Add(new SandboxError($"Implementing of interface not allowed: {@interface} (by {type})")); } } @@ -547,25 +561,25 @@ namespace Robust.Shared.ContentPack return nsDict.TryGetValue(type.Name, out cfg); } - private List GetReferencedTypes(MetadataReader reader, ConcurrentBag errors) + private List<(TypeReferenceHandle handle, MTypeReferenced parsed)> GetReferencedTypes(MetadataReader reader, ConcurrentBag errors) { return reader.TypeReferences.Select(typeRefHandle => { try { - return ParseTypeReference(reader, typeRefHandle); + return (typeRefHandle, ParseTypeReference(reader, typeRefHandle)); } catch (UnsupportedMetadataException e) { errors.Add(new SandboxError(e)); - return null; + return default; } }) - .Where(p => p != null) + .Where(p => p.Item2 != null) .ToList()!; } - private List GetReferencedMembers(MetadataReader reader, ConcurrentBag errors) + private List<(MemberReferenceHandle handle, MMemberRef parsed)> GetReferencedMembers(MetadataReader reader, ConcurrentBag errors) { return reader.MemberReferences.AsParallel() .Select(memRefHandle => @@ -586,7 +600,7 @@ namespace Robust.Shared.ContentPack catch (UnsupportedMetadataException u) { errors.Add(new SandboxError(u)); - return null; + return default; } break; @@ -600,7 +614,7 @@ namespace Robust.Shared.ContentPack catch (UnsupportedMetadataException u) { errors.Add(new SandboxError(u)); - return null; + return default; } break; @@ -616,7 +630,7 @@ namespace Robust.Shared.ContentPack { // Ensure this isn't a self-defined type. // This can happen due to generics since MethodSpec needs to point to MemberRef. - return null; + return default; } break; @@ -625,18 +639,18 @@ namespace Robust.Shared.ContentPack { errors.Add(new SandboxError( $"Module global variables and methods are unsupported. Name: {memName}")); - return null; + return default; } case HandleKind.MethodDefinition: { errors.Add(new SandboxError($"Vararg calls are unsupported. Name: {memName}")); - return null; + return default; } default: { errors.Add(new SandboxError( $"Unsupported member ref parent type: {memRef.Parent.Kind}. Name: {memName}")); - return null; + return default; } } @@ -667,9 +681,9 @@ namespace Robust.Shared.ContentPack throw new ArgumentOutOfRangeException(); } - return memberRef; + return (memRefHandle, memberRef); }) - .Where(p => p != null) + .Where(p => p.memberRef != null) .ToList()!; } @@ -780,7 +794,6 @@ namespace Robust.Shared.ContentPack } } - /// /// Thrown if the metadata does something funny we don't "support" like type forwarding. ///