From 39b5dbfaf798d4b588b0930fb59d4a827cc9461c Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Wed, 10 Jul 2024 13:13:21 +0200 Subject: [PATCH] C#: Perform fewer `regexpCapture`s when matching version numbers --- csharp/ql/lib/semmle/code/csharp/Location.qll | 76 ++++++++++++++++--- .../csharp/security/xml/InsecureXMLQuery.qll | 28 ++++--- 2 files changed, 82 insertions(+), 22 deletions(-) diff --git a/csharp/ql/lib/semmle/code/csharp/Location.qll b/csharp/ql/lib/semmle/code/csharp/Location.qll index eb6a30e7d35d..9b2cea470ed7 100644 --- a/csharp/ql/lib/semmle/code/csharp/Location.qll +++ b/csharp/ql/lib/semmle/code/csharp/Location.qll @@ -110,12 +110,21 @@ class SourceLocation extends Location, @location_default { bindingset[version] private int versionField(string version, int field) { - exists(string format | - format = "(\\d+)\\.(\\d+)\\.(\\d+)\\.(\\d+)" or - format = "(\\d+)\\.(\\d+)\\.(\\d+)" or - format = "(\\d+)\\.(\\d+)" + exists(string format, int i | + format = "(\\d+)\\.(\\d+)\\.(\\d+)\\.(\\d+)|" + "(\\d+)\\.(\\d+)\\.(\\d+)|" + "(\\d+)\\.(\\d+)" and + result = version.regexpCapture(format, i).toInt() | - result = version.regexpCapture(format, field).toInt() + i = [1, 5, 8] and + field = 1 + or + i = [2, 6, 9] and + field = 2 + or + i = [3, 7] and + field = 3 + or + i = 4 and + field = 4 ) and result >= 0 and result <= 255 @@ -123,8 +132,19 @@ private int versionField(string version, int field) { /** An assembly version, for example `4.0.0.0` or `4.5`. */ class Version extends string { + private int major; + bindingset[this] - Version() { exists(versionField(this, 1)) } + Version() { major = versionField(this, 1) } + + bindingset[this] + private int getVersionField(int field) { + field = 1 and + result = major + or + field in [2 .. 4] and + result = versionField(this, field) + } /** * Gets field `field` of this version. @@ -132,13 +152,47 @@ class Version extends string { */ bindingset[this] int getField(int field) { - field in [1 .. 4] and - if exists(versionField(this, field)) then result = versionField(this, field) else result = 0 + result = this.getVersionField(field) + or + field in [2 .. 4] and + not exists(this.getVersionField(field)) and + result = 0 + } + + bindingset[this] + private string getCanonicalizedField(int field) { + exists(string s, int length | + s = this.getVersionField(field).toString() and + length = s.length() + | + // make each field consist of 3 digits + result = concat(int i | i in [1 .. 3 - length] | "0") + s + ) + } + + /** + * Gets a canonicalized version of this string, where lexicographical ordering + * corresponds to version ordering. + */ + bindingset[this] + string getCanonicalizedVersion() { + exists(string res, int length | + res = + strictconcat(int field, string s | + s = this.getCanonicalizedField(field) + | + s, "." order by field + ) and + length = res.length() + | + // make each canonicalized version consist of 4 chunks of 3 digits separated by a dot + result = res + concat(int i | i = [1 .. 15 - length] / 4 and i > 0 | ".000") + ) } /** Gets the major version, for example `1` in `1.2.3.4`. */ bindingset[this] - int getMajor() { result = this.getField(1) } + int getMajor() { result = major } /** Gets the major revision, for example `2` in `1.2.3.4`. */ bindingset[this] @@ -164,9 +218,7 @@ class Version extends string { */ bindingset[this, other] predicate isEarlierThan(Version other) { - exists(int i | this.getField(i) < other.getField(i) | - forall(int j | j in [1 .. i - 1] | this.getField(j) = other.getField(j)) - ) + this.getCanonicalizedVersion() < other.getCanonicalizedVersion() } /** diff --git a/csharp/ql/lib/semmle/code/csharp/security/xml/InsecureXMLQuery.qll b/csharp/ql/lib/semmle/code/csharp/security/xml/InsecureXMLQuery.qll index 25793a8a71cc..ba98888fa6fc 100644 --- a/csharp/ql/lib/semmle/code/csharp/security/xml/InsecureXMLQuery.qll +++ b/csharp/ql/lib/semmle/code/csharp/security/xml/InsecureXMLQuery.qll @@ -5,6 +5,19 @@ import csharp private import semmle.code.csharp.commons.TargetFramework +pragma[nomagic] +private float getAssemblyVersion(Assembly a) { + result = a.getVersion().regexpCapture("([0-9]+\\.[0-9]+).*", 1).toFloat() and + // This method is only accurate when we're looking at versions before 4.0. + result < 4.0 +} + +pragma[nomagic] +private Version getTargetFrameworkVersion(TargetFrameworkAttribute tfa) { + tfa.isNetFramework() and + result = tfa.getFrameworkVersion() +} + /** * Holds if the type `t` is in an assembly that has been compiled against a .NET framework version * before the given version. @@ -14,21 +27,16 @@ private predicate isNetFrameworkBefore(Type t, string version) { // For assemblies compiled against framework versions before 4 the TargetFrameworkAttribute // will not be present. In this case, we can revert back to the assembly version, which may not // contain full minor version information. - exists(string assemblyVersion | - assemblyVersion = - t.getALocation().(Assembly).getVersion().regexpCapture("([0-9]+\\.[0-9]+).*", 1) - | - assemblyVersion.toFloat() < version.toFloat() and - // This method is only accurate when we're looking at versions before 4.0. - assemblyVersion.toFloat() < 4.0 + exists(float assemblyVersion | + assemblyVersion = getAssemblyVersion(t.getALocation()) and + assemblyVersion < version.toFloat() ) or // For 4.0 and above the TargetFrameworkAttribute should be present to provide detailed version // information. exists(TargetFrameworkAttribute tfa | tfa.hasElement(t) and - tfa.isNetFramework() and - tfa.getFrameworkVersion().isEarlierThan(version) + getTargetFrameworkVersion(tfa).isEarlierThan(version) ) } @@ -173,7 +181,7 @@ module XmlReader { reason = "DTD processing is enabled by default in versions < 4.0" and evidence = this and not exists(this.getSettings()) and - isNetFrameworkBefore(this.(MethodCall).getTarget().getDeclaringType(), "4.0") + isNetFrameworkBefore(this.getTarget().getDeclaringType(), "4.0") or // bad settings flow here exists(ObjectCreation settings |