Fix overeager finding of signatures.

vcsjones · vcsjones · commit 5972e41f34fa · 2016-06-10T14:02:07.000-04:00
The new implementation of VisitAll in #22 walked the signature tree as deep as possible. The problem with this is that counter signatures from nested signatures will be included as counter signatures from the root signature. This change changes the behavior so that VisitAll only follows nesting of the same type. A new flag on SignatureKind has been added, Universal, so that the visitation keeps the eager behavior when it's really wanted.
diff --git a/AuthenticodeLint/Properties/AssemblyInfo.cs b/AuthenticodeLint/Properties/AssemblyInfo.cs
@@ -12,5 +12,5 @@
 [assembly: ComVisible(false)]
 [assembly: Guid("ff77faed-3274-4c0e-bda0-98d8a5fa831e")]
 
-[assembly: AssemblyVersion("0.4.0.0")]
-[assembly: AssemblyFileVersion("0.4.0.0")]
+[assembly: AssemblyVersion("0.7.0.0")]
+[assembly: AssemblyFileVersion("0.7.0.0")]
diff --git a/AuthenticodeLint/Rules/10002-NoWeakFileDigestAlgorithmsRule.cs b/AuthenticodeLint/Rules/10002-NoWeakFileDigestAlgorithmsRule.cs
@@ -12,7 +12,7 @@ public class NoWeakFileDigestAlgorithmsRule : IAuthenticodeSignatureRule
 
         public RuleResult Validate(IReadOnlyList<ISignature> graph, SignatureLogger verboseWriter, CheckConfiguration configuration)
         {
-            var signatures = graph.VisitAll(SignatureKind.AnySignature);
+            var signatures = graph.VisitAll(SignatureKind.AnySignature | SignatureKind.Deep);
             var result = RuleResult.Pass;
             foreach(var signature in signatures)
             {
diff --git a/AuthenticodeLint/Rules/10003-TimestampedRule.cs b/AuthenticodeLint/Rules/10003-TimestampedRule.cs
@@ -1,5 +1,6 @@
 ﻿using System;
 using System.Collections.Generic;
+using System.Linq;
 
 namespace AuthenticodeLint.Rules
 {
@@ -17,7 +18,7 @@ public unsafe RuleResult Validate(IReadOnlyList<ISignature> graph, SignatureLogg
             var pass = true;
             foreach (var signature in signatures)
             {
-                var counterSignatures = signature.VisitAll(SignatureKind.AnyCounterSignature);
+                var counterSignatures = signature.VisitAll(SignatureKind.AnyCounterSignature).ToList();
                 var isSigned = false;
                 var strongSign = false;
                 foreach (var counterSignature in counterSignatures)
diff --git a/AuthenticodeLint/Rules/10004-PublisherInformationRule.cs b/AuthenticodeLint/Rules/10004-PublisherInformationRule.cs
@@ -13,7 +13,7 @@ public class PublisherInformationPresentRule : IAuthenticodeSignatureRule
 
         public RuleResult Validate(IReadOnlyList<ISignature> graph, SignatureLogger verboseWriter, CheckConfiguration configuration)
         {
-            var signatures = graph.VisitAll(SignatureKind.AnySignature);
+            var signatures = graph.VisitAll(SignatureKind.AnySignature | SignatureKind.Deep);
             var result = RuleResult.Pass;
             foreach (var signature in signatures)
             {
diff --git a/AuthenticodeLint/Rules/10005-PublisherInformationUrlHttpsRule.cs b/AuthenticodeLint/Rules/10005-PublisherInformationUrlHttpsRule.cs
@@ -14,7 +14,7 @@ public class PublisherInformationUrlHttpsRule : IAuthenticodeSignatureRule
 
         public RuleResult Validate(IReadOnlyList<ISignature> graph, SignatureLogger verboseWriter, CheckConfiguration configuration)
         {
-            var signatures = graph.VisitAll(SignatureKind.AnySignature);
+            var signatures = graph.VisitAll(SignatureKind.AnySignature | SignatureKind.Deep);
             var result = RuleResult.Pass;
             foreach(var signature in signatures)
             {
diff --git a/AuthenticodeLint/Rules/10010-NoUnknownCertificatesRule.cs b/AuthenticodeLint/Rules/10010-NoUnknownCertificatesRule.cs
@@ -17,7 +17,7 @@ public RuleResult Validate(IReadOnlyList<ISignature> graph, SignatureLogger verb
             var result = RuleResult.Pass;
             //We exclude Authenticode timestamps because they cannot contain "additional" certificates but rather
             //Use their parent. Including Authenticode timestamps will produce duplicate warnings.
-            var signatures = graph.VisitAll(SignatureKind.AnySignature | SignatureKind.Rfc3161Timestamp);
+            var signatures = graph.VisitAll(SignatureKind.AnySignature | SignatureKind.Rfc3161Timestamp | SignatureKind.Deep);
             foreach (var signature in signatures)
             {
                 var allEmbeddedCertificates = signature.AdditionalCertificates.Cast<X509Certificate2>().ToList();
diff --git a/AuthenticodeLint/Rules/10011-StrongKeyLengthRule.cs b/AuthenticodeLint/Rules/10011-StrongKeyLengthRule.cs
@@ -15,7 +15,7 @@ public class StrongKeyLengthRule : IAuthenticodeSignatureRule
 
         public RuleResult Validate(IReadOnlyList<ISignature> graph, SignatureLogger verboseWriter, CheckConfiguration configuration)
         {
-            var signatures = graph.VisitAll(SignatureKind.Any);
+            var signatures = graph.VisitAll(SignatureKind.Any | SignatureKind.Any);
             var result = RuleResult.Pass;
             foreach (var signature in signatures)
             {
diff --git a/AuthenticodeLint/Rules/10013-MaxKeyLengthRule.cs b/AuthenticodeLint/Rules/10013-MaxKeyLengthRule.cs
@@ -17,7 +17,7 @@ public class MaxKeyLengthRule : IAuthenticodeSignatureRule
 
         public RuleResult Validate(IReadOnlyList<ISignature> graph, SignatureLogger verboseWriter, CheckConfiguration configuration)
         {
-            var signatures = graph.VisitAll(SignatureKind.Any);
+            var signatures = graph.VisitAll(SignatureKind.Any | SignatureKind.Deep);
             var result = RuleResult.Pass;
             foreach (var signature in signatures)
             {
diff --git a/AuthenticodeLint/Rules/CertificateChainRuleBase.cs b/AuthenticodeLint/Rules/CertificateChainRuleBase.cs
@@ -13,7 +13,7 @@ public abstract class CertificateChainRuleBase : IAuthenticodeSignatureRule
 
         public RuleResult Validate(IReadOnlyList<ISignature> graph, SignatureLogger verboseWriter, CheckConfiguration configuration)
         {
-            var signatures = graph.VisitAll(SignatureKind.AnySignature);
+            var signatures = graph.VisitAll(SignatureKind.AnySignature | SignatureKind.Deep);
             var result = RuleResult.Pass;
             foreach (var signature in signatures)
             {
diff --git a/AuthenticodeLint/Signature.cs b/AuthenticodeLint/Signature.cs
@@ -121,6 +121,7 @@ public class AuthenticodeSignature : SignatureBase
         public unsafe AuthenticodeSignature(AsnEncodedData data, ISignature owningSignature)
         {
             OwningSignature = owningSignature;
+            Kind = SignatureKind.AuthenticodeTimestamp;
             AdditionalCertificates = owningSignature.AdditionalCertificates;
             fixed (byte* dataPtr = data.RawData)
             {
diff --git a/AuthenticodeLint/SignatureExtensions.cs b/AuthenticodeLint/SignatureExtensions.cs
@@ -1,19 +1,30 @@
 ﻿using System;
 using System.Collections.Generic;
+using System.Linq;
 
 namespace AuthenticodeLint
 {
     public static class SignatureExtensions
     {
         public static IEnumerable<ISignature> VisitAll(this ISignature signature, SignatureKind kind)
         {
-            if ((signature.Kind & kind) > 0)
+            foreach (var nested in signature.GetNestedSignatures())
             {
-                yield return signature;
-            }
-            foreach (var nested in signature.GetNestedSignatures().VisitAll(kind))
-            {
-                yield return nested;
+                if ((nested.Kind & kind) > 0)
+                {
+                    yield return nested;
+                    foreach(var nestVisit in nested.VisitAll(kind))
+                    {
+                        yield return nestVisit;
+                    }
+                }
+                else if ((kind & SignatureKind.Deep) == SignatureKind.Deep)
+                {
+                    foreach (var nestVisit in nested.VisitAll(kind))
+                    {
+                        yield return nestVisit;
+                    }
+                }
             }
         }
 
@@ -25,7 +36,7 @@ public static IEnumerable<ISignature> VisitAll(this IReadOnlyList<ISignature> si
                 {
                     yield return signature;
                 }
-                foreach (var nested in signature.GetNestedSignatures().VisitAll(kind))
+                foreach (var nested in VisitAll(signature, kind))
                 {
                     yield return nested;
                 }
@@ -43,6 +54,7 @@ public enum SignatureKind
         Rfc3161Timestamp = 0x8,
         AnySignature = NestedSignature | Signature,
         AnyCounterSignature = AuthenticodeTimestamp | Rfc3161Timestamp,
-        Any = AnySignature | AnyCounterSignature
+        Any = AnySignature | AnyCounterSignature,
+        Deep = 0x1000
     }
 }

Original file line number	Diff line number	Diff line change
`@@ -12,7 +12,7 @@ public class NoWeakFileDigestAlgorithmsRule : IAuthenticodeSignatureRule`
`12`	`12`
`13`	`13`	`public RuleResult Validate(IReadOnlyList<ISignature> graph, SignatureLogger verboseWriter, CheckConfiguration configuration)`
`14`	`14`	`{`
`15`		`- var signatures = graph.VisitAll(SignatureKind.AnySignature);`
	`15`	`+ var signatures = graph.VisitAll(SignatureKind.AnySignature \| SignatureKind.Deep);`
`16`	`16`	`var result = RuleResult.Pass;`
`17`	`17`	`foreach(var signature in signatures)`
`18`	`18`	`{`
Original file line number	Diff line number	Diff line change
`@@ -1,5 +1,6 @@`
`1`	`1`	`using System;`
`2`	`2`	`using System.Collections.Generic;`
	`3`	`+using System.Linq;`
`3`	`4`
`4`	`5`	`namespace AuthenticodeLint.Rules`
`5`	`6`	`{`
`@@ -17,7 +18,7 @@ public unsafe RuleResult Validate(IReadOnlyList<ISignature> graph, SignatureLogg`
`17`	`18`	`var pass = true;`
`18`	`19`	`foreach (var signature in signatures)`
`19`	`20`	`{`
`20`		`- var counterSignatures = signature.VisitAll(SignatureKind.AnyCounterSignature);`
	`21`	`+ var counterSignatures = signature.VisitAll(SignatureKind.AnyCounterSignature).ToList();`
`21`	`22`	`var isSigned = false;`
`22`	`23`	`var strongSign = false;`
`23`	`24`	`foreach (var counterSignature in counterSignatures)`
Original file line number	Diff line number	Diff line change
`@@ -13,7 +13,7 @@ public class PublisherInformationPresentRule : IAuthenticodeSignatureRule`
`13`	`13`
`14`	`14`	`public RuleResult Validate(IReadOnlyList<ISignature> graph, SignatureLogger verboseWriter, CheckConfiguration configuration)`
`15`	`15`	`{`
`16`		`- var signatures = graph.VisitAll(SignatureKind.AnySignature);`
	`16`	`+ var signatures = graph.VisitAll(SignatureKind.AnySignature \| SignatureKind.Deep);`
`17`	`17`	`var result = RuleResult.Pass;`
`18`	`18`	`foreach (var signature in signatures)`
`19`	`19`	`{`
Original file line number	Diff line number	Diff line change
`@@ -14,7 +14,7 @@ public class PublisherInformationUrlHttpsRule : IAuthenticodeSignatureRule`
`14`	`14`
`15`	`15`	`public RuleResult Validate(IReadOnlyList<ISignature> graph, SignatureLogger verboseWriter, CheckConfiguration configuration)`
`16`	`16`	`{`
`17`		`- var signatures = graph.VisitAll(SignatureKind.AnySignature);`
	`17`	`+ var signatures = graph.VisitAll(SignatureKind.AnySignature \| SignatureKind.Deep);`
`18`	`18`	`var result = RuleResult.Pass;`
`19`	`19`	`foreach(var signature in signatures)`
`20`	`20`	`{`
Original file line number	Diff line number	Diff line change
`@@ -17,7 +17,7 @@ public RuleResult Validate(IReadOnlyList<ISignature> graph, SignatureLogger verb`
`17`	`17`	`var result = RuleResult.Pass;`
`18`	`18`	`//We exclude Authenticode timestamps because they cannot contain "additional" certificates but rather`
`19`	`19`	`//Use their parent. Including Authenticode timestamps will produce duplicate warnings.`
`20`		`- var signatures = graph.VisitAll(SignatureKind.AnySignature \| SignatureKind.Rfc3161Timestamp);`
	`20`	`+ var signatures = graph.VisitAll(SignatureKind.AnySignature \| SignatureKind.Rfc3161Timestamp \| SignatureKind.Deep);`
`21`	`21`	`foreach (var signature in signatures)`
`22`	`22`	`{`
`23`	`23`	`var allEmbeddedCertificates = signature.AdditionalCertificates.Cast<X509Certificate2>().ToList();`
Original file line number	Diff line number	Diff line change
`@@ -15,7 +15,7 @@ public class StrongKeyLengthRule : IAuthenticodeSignatureRule`
`15`	`15`
`16`	`16`	`public RuleResult Validate(IReadOnlyList<ISignature> graph, SignatureLogger verboseWriter, CheckConfiguration configuration)`
`17`	`17`	`{`
`18`		`- var signatures = graph.VisitAll(SignatureKind.Any);`
	`18`	`+ var signatures = graph.VisitAll(SignatureKind.Any \| SignatureKind.Any);`
`19`	`19`	`var result = RuleResult.Pass;`
`20`	`20`	`foreach (var signature in signatures)`
`21`	`21`	`{`
Original file line number	Diff line number	Diff line change
`@@ -17,7 +17,7 @@ public class MaxKeyLengthRule : IAuthenticodeSignatureRule`
`17`	`17`
`18`	`18`	`public RuleResult Validate(IReadOnlyList<ISignature> graph, SignatureLogger verboseWriter, CheckConfiguration configuration)`
`19`	`19`	`{`
`20`		`- var signatures = graph.VisitAll(SignatureKind.Any);`
	`20`	`+ var signatures = graph.VisitAll(SignatureKind.Any \| SignatureKind.Deep);`
`21`	`21`	`var result = RuleResult.Pass;`
`22`	`22`	`foreach (var signature in signatures)`
`23`	`23`	`{`
Original file line number	Diff line number	Diff line change
`@@ -13,7 +13,7 @@ public abstract class CertificateChainRuleBase : IAuthenticodeSignatureRule`
`13`	`13`
`14`	`14`	`public RuleResult Validate(IReadOnlyList<ISignature> graph, SignatureLogger verboseWriter, CheckConfiguration configuration)`
`15`	`15`	`{`
`16`		`- var signatures = graph.VisitAll(SignatureKind.AnySignature);`
	`16`	`+ var signatures = graph.VisitAll(SignatureKind.AnySignature \| SignatureKind.Deep);`
`17`	`17`	`var result = RuleResult.Pass;`
`18`	`18`	`foreach (var signature in signatures)`
`19`	`19`	`{`
Original file line number	Diff line number	Diff line change
`@@ -121,6 +121,7 @@ public class AuthenticodeSignature : SignatureBase`
`121`	`121`	`public unsafe AuthenticodeSignature(AsnEncodedData data, ISignature owningSignature)`
`122`	`122`	`{`
`123`	`123`	`OwningSignature = owningSignature;`
	`124`	`+ Kind = SignatureKind.AuthenticodeTimestamp;`
`124`	`125`	`AdditionalCertificates = owningSignature.AdditionalCertificates;`
`125`	`126`	`fixed (byte* dataPtr = data.RawData)`
`126`	`127`	`{`