Merge remote-tracking branch 'upstream/main' into fixmodel

geoffw0 · geoffw0 · commit 296dee90dd9e · 2023-11-14T09:38:14.000Z
diff --git a/cpp/autobuilder/Semmle.Autobuild.Cpp.Tests/BuildScripts.cs b/cpp/autobuilder/Semmle.Autobuild.Cpp.Tests/BuildScripts.cs
@@ -145,9 +145,9 @@ IEnumerable<string> IBuildActions.EnumerateDirectories(string dir)
 
         bool IBuildActions.IsMacOs() => IsMacOs;
 
-        public bool IsArm { get; set; }
+        public bool IsRunningOnAppleSilicon { get; set; }
 
-        bool IBuildActions.IsArm() => IsArm;
+        bool IBuildActions.IsRunningOnAppleSilicon() => IsRunningOnAppleSilicon;
 
         string IBuildActions.PathCombine(params string[] parts)
         {
diff --git a/csharp/autobuilder/Semmle.Autobuild.CSharp.Tests/BuildScripts.cs b/csharp/autobuilder/Semmle.Autobuild.CSharp.Tests/BuildScripts.cs
@@ -159,9 +159,9 @@ IEnumerable<string> IBuildActions.EnumerateDirectories(string dir)
 
         bool IBuildActions.IsMacOs() => IsMacOs;
 
-        public bool IsArm { get; set; }
+        public bool IsRunningOnAppleSilicon { get; set; }
 
-        bool IBuildActions.IsArm() => IsArm;
+        bool IBuildActions.IsRunningOnAppleSilicon() => IsRunningOnAppleSilicon;
 
         public string PathCombine(params string[] parts)
         {
diff --git a/csharp/autobuilder/Semmle.Autobuild.Shared/BuildActions.cs b/csharp/autobuilder/Semmle.Autobuild.Shared/BuildActions.cs
@@ -3,6 +3,7 @@
 using System.Diagnostics;
 using System.Diagnostics.CodeAnalysis;
 using System.IO;
+using System.Linq;
 using System.Runtime.InteropServices;
 using System.Xml;
 using Semmle.Util;
@@ -119,10 +120,10 @@ public interface IBuildActions
         bool IsMacOs();
 
         /// <summary>
-        /// Gets a value indicating whether we are running on arm.
+        /// Gets a value indicating whether we are running on Apple Silicon.
         /// </summary>
-        /// <returns>True if we are running on arm.</returns>
-        bool IsArm();
+        /// <returns>True if we are running on Apple Silicon.</returns>
+        bool IsRunningOnAppleSilicon();
 
         /// <summary>
         /// Combine path segments, Path.Combine().
@@ -240,9 +241,25 @@ int IBuildActions.RunProcess(string cmd, string args, string? workingDirectory,
 
         bool IBuildActions.IsMacOs() => RuntimeInformation.IsOSPlatform(OSPlatform.OSX);
 
-        bool IBuildActions.IsArm() =>
-            RuntimeInformation.ProcessArchitecture == Architecture.Arm64 ||
-            RuntimeInformation.ProcessArchitecture == Architecture.Arm;
+        bool IBuildActions.IsRunningOnAppleSilicon()
+        {
+            var thisBuildActions = (IBuildActions)this;
+
+            if (!thisBuildActions.IsMacOs())
+            {
+                return false;
+            }
+
+            try
+            {
+                thisBuildActions.RunProcess("sysctl", "machdep.cpu.brand_string", workingDirectory: null, env: null, out var stdOut);
+                return stdOut?.Any(s => s?.ToLowerInvariant().Contains("apple") == true) ?? false;
+            }
+            catch (Exception)
+            {
+                return false;
+            }
+        }
 
         string IBuildActions.PathCombine(params string[] parts) => Path.Combine(parts);
 
diff --git a/csharp/autobuilder/Semmle.Autobuild.Shared/MsBuildRule.cs b/csharp/autobuilder/Semmle.Autobuild.Shared/MsBuildRule.cs
@@ -15,14 +15,12 @@ internal static class MsBuildCommandExtensions
         /// <returns></returns>
         public static CommandBuilder MsBuildCommand(this CommandBuilder cmdBuilder, IAutobuilder<AutobuildOptionsShared> builder)
         {
-            var isArmMac = builder.Actions.IsMacOs() && builder.Actions.IsArm();
-
             // mono doesn't ship with `msbuild` on Arm-based Macs, but we can fall back to
             // msbuild that ships with `dotnet` which can be invoked with `dotnet msbuild`
             // perhaps we should do this on all platforms?
-            return isArmMac ?
-                cmdBuilder.RunCommand("dotnet").Argument("msbuild") :
-                cmdBuilder.RunCommand("msbuild");
+            return builder.Actions.IsRunningOnAppleSilicon()
+                ? cmdBuilder.RunCommand("dotnet").Argument("msbuild")
+                : cmdBuilder.RunCommand("msbuild");
         }
     }
 
diff --git a/ql/Cargo.lock b/ql/Cargo.lock
diff --git a/ql/extractor/Cargo.toml b/ql/extractor/Cargo.toml
@@ -14,7 +14,7 @@ tree-sitter-blame = {path = "../buramu/tree-sitter-blame"}
 tree-sitter-json = {git = "https://github.com/tausbn/tree-sitter-json.git", rev = "745663ee997f1576fe1e7187e6347e0db36ec7a9"}
 clap = { version = "4.2", features = ["derive"] }
 tracing = "0.1"
-tracing-subscriber = { version = "0.3.17", features = ["env-filter"] }
+tracing-subscriber = { version = "0.3.18", features = ["env-filter"] }
 rayon = "1.8.0"
 regex = "1.10.2"
 codeql-extractor = { path = "../../shared/tree-sitter-extractor" }
diff --git a/swift/ql/lib/codeql/swift/frameworks/StandardLibrary/NsString.qll b/swift/ql/lib/codeql/swift/frameworks/StandardLibrary/NsString.qll
@@ -103,8 +103,8 @@ private class NsStringSummaries extends SummaryModelCsv {
         ";NSString;true;data(using:);;;Argument[-1];ReturnValue;taint",
         ";NSString;true;data(using:allowLossyConversion:);;;Argument[-1];ReturnValue;taint",
         ";NSString;true;path(withComponents:);;;Argument[0].CollectionElement;ReturnValue;taint",
-        ";NSString;true;completePath(into:caseSensitive:matchesInto:filterTypes:);;;Argument[-1];Argument[0];taint",
-        ";NSString;true;completePath(into:caseSensitive:matchesInto:filterTypes:);;;Argument[-1];Argument[2];taint",
+        ";NSString;true;completePath(into:caseSensitive:matchesInto:filterTypes:);;;Argument[-1];Argument[0].CollectionElement;taint",
+        ";NSString;true;completePath(into:caseSensitive:matchesInto:filterTypes:);;;Argument[-1];Argument[2].CollectionElement.CollectionElement;taint",
         ";NSString;true;getFileSystemRepresentation(_:maxLength:);;;Argument[-1];Argument[0];taint",
         ";NSString;true;appendingPathComponent(_:);;;Argument[-1..0];ReturnValue;taint",
         ";NSString;true;appendingPathComponent(_:conformingTo:);;;Argument[-1..0];ReturnValue;taint",
diff --git a/swift/ql/lib/codeql/swift/frameworks/StandardLibrary/NsUrl.qll b/swift/ql/lib/codeql/swift/frameworks/StandardLibrary/NsUrl.qll
@@ -3,13 +3,34 @@
  */
 
 import swift
+private import codeql.swift.dataflow.DataFlow
 private import codeql.swift.dataflow.ExternalFlow
+private import codeql.swift.dataflow.FlowSteps
+
+/**
+ * A content implying that, if an `NSURL` is tainted, then all its fields are tainted.
+ */
+private class NSUrlFieldsInheritTaint extends TaintInheritingContent,
+  DataFlow::Content::FieldContent
+{
+  NSUrlFieldsInheritTaint() {
+    this.getField().getEnclosingDecl().asNominalTypeDecl().getFullName() = "NSURL"
+  }
+}
 
 /**
  * A model for `NSURL` members that permit taint flow.
  */
 private class NsUrlSummaries extends SummaryModelCsv {
   override predicate row(string row) {
-    row = ";NSURL;true;init(string:);(String);;Argument[0];ReturnValue.OptionalSome;taint"
+    row =
+      [
+        ";NSURL;true;init(string:);(String);;Argument[0];ReturnValue.OptionalSome;taint",
+        ";NSURL;true;appendingPathComponent(_:);;;Argument[-1..0];ReturnValue;taint",
+        ";NSURL;true;appendingPathComponent(_:isDirectory:);;;Argument[-1..0];ReturnValue;taint",
+        ";NSURL;true;appendingPathComponent(_:conformingTo:);;;Argument[-1..0];ReturnValue;taint",
+        ";NSURL;true;appendingPathExtension(_:);;;Argument[-1..0];ReturnValue;taint",
+        ";NSURL;true;appendingPathExtension(for:);;;Argument[-1];ReturnValue;taint",
+      ]
   }
 }
diff --git a/swift/ql/lib/codeql/swift/frameworks/StandardLibrary/String.qll b/swift/ql/lib/codeql/swift/frameworks/StandardLibrary/String.qll
@@ -45,8 +45,8 @@ private class StringSummaries extends SummaryModelCsv {
         ";StringProtocol;true;applyingTransform(_:reverse:);;;Argument[-1];ReturnValue;taint",
         ";StringProtocol;true;cString(using:);;;Argument[-1];ReturnValue;taint",
         ";StringProtocol;true;capitalized(with:);;;Argument[-1];ReturnValue;taint",
-        ";StringProtocol;true;completePath(into:caseSensitive:matchesInto:filterTypes:);;;Argument[-1];Argument[0].OptionalSome.CollectionElement;taint",
-        ";StringProtocol;true;completePath(into:caseSensitive:matchesInto:filterTypes:);;;Argument[-1];Argument[2].OptionalSome.CollectionElement.CollectionElement;taint",
+        ";StringProtocol;true;completePath(into:caseSensitive:matchesInto:filterTypes:);;;Argument[-1];Argument[0].CollectionElement;taint",
+        ";StringProtocol;true;completePath(into:caseSensitive:matchesInto:filterTypes:);;;Argument[-1];Argument[2].CollectionElement.CollectionElement;taint",
         ";StringProtocol;true;components(separatedBy:);;;Argument[-1];ReturnValue.CollectionElement;taint",
         ";StringProtocol;true;data(using:allowLossyConversion:);;;Argument[-1];ReturnValue;taint",
         ";StringProtocol;true;folding(options:locale:);;;Argument[-1];ReturnValue;taint",
diff --git a/swift/ql/lib/codeql/swift/frameworks/StandardLibrary/Url.qll b/swift/ql/lib/codeql/swift/frameworks/StandardLibrary/Url.qll
@@ -15,8 +15,8 @@ class UrlDecl extends StructDecl {
 /**
  * A content implying that, if a `URL` is tainted, then all its fields are tainted.
  */
-private class UriFieldsInheritTaint extends TaintInheritingContent, DataFlow::Content::FieldContent {
-  UriFieldsInheritTaint() {
+private class UrlFieldsInheritTaint extends TaintInheritingContent, DataFlow::Content::FieldContent {
+  UrlFieldsInheritTaint() {
     this.getField().getEnclosingDecl().asNominalTypeDecl() instanceof UrlDecl
   }
 }
@@ -108,6 +108,8 @@ private class UrlSummaries extends SummaryModelCsv {
         ";URL;true;init(dataRepresentation:relativeTo:isAbsolute:);;;Argument[0];ReturnValue;taint",
         ";URL;true;init(dataRepresentation:relativeTo:isAbsolute:);;;Argument[1].OptionalSome;ReturnValue;taint",
         ";URL;true;init(_:strategy:);;;Argument[0];ReturnValue;taint",
+        ";URL;true;init(filePath:);;;Argument[0];ReturnValue.OptionalSome;taint",
+        ";URL;true;init(filePath:isDirectory:);;;Argument[0];ReturnValue.OptionalSome;taint",
         ";URL;true;init(filePath:directoryHint:);;;Argument[0];ReturnValue.OptionalSome;taint",
         ";URL;true;init(filePath:directoryHint:relativeTo:);;;Argument[0];ReturnValue;taint",
         ";URL;true;init(filePath:directoryHint:relativeTo:);;;Argument[2].OptionalSome;ReturnValue;taint",
@@ -128,6 +130,7 @@ private class UrlSummaries extends SummaryModelCsv {
         ";URL;true;appendingPathComponent(_:conformingTo:);;;Argument[-1..0];ReturnValue;taint",
         ";URL;true;appendPathExtension(_:);;;Argument[-1..0];Argument[-1];taint",
         ";URL;true;appendingPathExtension(_:);;;Argument[-1..0];ReturnValue;taint",
+        ";URL;true;appendingPathExtension(for:);;;Argument[-1];ReturnValue;taint",
         ";URL;true;deletingLastPathComponent();;;Argument[-1];ReturnValue;taint",
         ";URL;true;deletingPathExtension();;;Argument[-1];ReturnValue;taint",
         ";URL;true;bookmarkData(options:includingResourceValuesForKeys:relativeTo:);;;Argument[-1];ReturnValue;taint",
diff --git a/swift/ql/lib/codeql/swift/security/PathInjectionExtensions.qll b/swift/ql/lib/codeql/swift/security/PathInjectionExtensions.qll
@@ -61,6 +61,41 @@ private class EnumConstructorPathInjectionSink extends PathInjectionSink {
   }
 }
 
+/**
+ * A string that might be a label for a path argument.
+ */
+pragma[inline]
+private predicate pathLikeHeuristic(string label) {
+  label =
+    [
+      "atFile", "atPath", "atDirectory", "toFile", "toPath", "toDirectory", "inFile", "inPath",
+      "inDirectory", "contentsOfFile", "contentsOfPath", "contentsOfDirectory", "filePath",
+      "directory", "directoryPath"
+    ]
+}
+
+/**
+ * A path injection sink that is determined by imprecise methods.
+ */
+private class HeuristicPathInjectionSink extends PathInjectionSink {
+  HeuristicPathInjectionSink() {
+    // by parameter name
+    exists(CallExpr ce, int ix, ParamDecl pd |
+      pathLikeHeuristic(pragma[only_bind_into](pd.getName())) and
+      pd.getType().getUnderlyingType().getName() = ["String", "NSString"] and
+      pd = ce.getStaticTarget().getParam(ix) and
+      this.asExpr() = ce.getArgument(ix).getExpr()
+    )
+    or
+    // by argument name
+    exists(Argument a |
+      pathLikeHeuristic(pragma[only_bind_into](a.getLabel())) and
+      a.getExpr().getType().getUnderlyingType().getName() = ["String", "NSString"] and
+      this.asExpr() = a.getExpr()
+    )
+  }
+}
+
 private class DefaultPathInjectionBarrier extends PathInjectionBarrier {
   DefaultPathInjectionBarrier() {
     // This is a simplified implementation.
@@ -87,7 +122,14 @@ private class PathInjectionSinks extends SinkModelCsv {
   override predicate row(string row) {
     row =
       [
+        ";Data;true;init(contentsOf:options:);;;Argument[0];path-injection",
         ";Data;true;write(to:options:);;;Argument[0];path-injection",
+        ";NSData;true;init(contentsOfFile:);;;Argument[0];path-injection",
+        ";NSData;true;init(contentsOfFile:options:);;;Argument[0];path-injection",
+        ";NSData;true;init(contentsOf:);;;Argument[0];path-injection",
+        ";NSData;true;init(contentsOf:options:);;;Argument[0];path-injection",
+        ";NSData;true;init(contentsOfMappedFile:);;;Argument[0];path-injection",
+        ";NSData;true;dataWithContentsOfMappedFile(_:);;;Argument[0];path-injection",
         ";NSData;true;write(to:atomically:);;;Argument[0];path-injection",
         ";NSData;true;write(to:options:);;;Argument[0];path-injection",
         ";NSData;true;write(toFile:atomically:);;;Argument[0];path-injection",
@@ -118,12 +160,14 @@ private class PathInjectionSinks extends SinkModelCsv {
         ";FileManager;true;fileExists(atPath:);;;Argument[0];path-injection",
         ";FileManager;true;fileExists(atPath:isDirectory:);;;Argument[0];path-injection",
         ";FileManager;true;setAttributes(_:ofItemAtPath:);;;Argument[1];path-injection",
+        ";FileManager;true;attributesOfItem(atPath:);;;Argument[0];path-injection",
         ";FileManager;true;contents(atPath:);;;Argument[0];path-injection",
         ";FileManager;true;contentsEqual(atPath:andPath:);;;Argument[0..1];path-injection",
         ";FileManager;true;changeCurrentDirectoryPath(_:);;;Argument[0];path-injection",
         ";FileManager;true;unmountVolume(at:options:completionHandler:);;;Argument[0];path-injection",
         // Deprecated FileManager methods:
         ";FileManager;true;changeFileAttributes(_:atPath:);;;Argument[1];path-injection",
+        ";FileManager;true;fileAttributes(atPath:traverseLink:);;;Argument[0];path-injection",
         ";FileManager;true;directoryContents(atPath:);;;Argument[0];path-injection",
         ";FileManager;true;createDirectory(atPath:attributes:);;;Argument[0];path-injection",
         ";FileManager;true;createSymbolicLink(atPath:pathContent:);;;Argument[0..1];path-injection",
@@ -146,6 +190,7 @@ private class PathInjectionSinks extends SinkModelCsv {
         ";ArchiveByteStream;true;withFileStream(path:mode:options:permissions:_:);;;Argument[0];path-injection",
         ";Bundle;true;init(url:);;;Argument[0];path-injection",
         ";Bundle;true;init(path:);;;Argument[0];path-injection",
+        ";NSURL;writeBookmarkData(_:to:options:);;;Argument[1];path-injection",
         // GRDB
         ";Database;true;init(path:description:configuration:);;;Argument[0];path-injection",
         ";DatabasePool;true;init(path:configuration:);;;Argument[0];path-injection",
diff --git a/swift/ql/src/change-notes/2023-11-10-path-injection.md b/swift/ql/src/change-notes/2023-11-10-path-injection.md
@@ -0,0 +1,5 @@
+---
+category: minorAnalysis
+---
+
+* Added additional sinks for the "Uncontrolled data used in path expression" (`swift/path-injection`) query. Some of these sinks are heuristic (imprecise) in nature.
diff --git a/swift/ql/test/query-tests/Security/CWE-022/testPathInjection.swift b/swift/ql/test/query-tests/Security/CWE-022/testPathInjection.swift

Original file line number	Diff line number	Diff line change
`@@ -145,9 +145,9 @@ IEnumerable<string> IBuildActions.EnumerateDirectories(string dir)`
`145`	`145`
`146`	`146`	`bool IBuildActions.IsMacOs() => IsMacOs;`
`147`	`147`
`148`		`- public bool IsArm { get; set; }`
	`148`	`+ public bool IsRunningOnAppleSilicon { get; set; }`
`149`	`149`
`150`		`- bool IBuildActions.IsArm() => IsArm;`
	`150`	`+ bool IBuildActions.IsRunningOnAppleSilicon() => IsRunningOnAppleSilicon;`
`151`	`151`
`152`	`152`	`string IBuildActions.PathCombine(params string[] parts)`
`153`	`153`	`{`
Original file line number	Diff line number	Diff line change
`@@ -159,9 +159,9 @@ IEnumerable<string> IBuildActions.EnumerateDirectories(string dir)`
`159`	`159`
`160`	`160`	`bool IBuildActions.IsMacOs() => IsMacOs;`
`161`	`161`
`162`		`- public bool IsArm { get; set; }`
	`162`	`+ public bool IsRunningOnAppleSilicon { get; set; }`
`163`	`163`
`164`		`- bool IBuildActions.IsArm() => IsArm;`
	`164`	`+ bool IBuildActions.IsRunningOnAppleSilicon() => IsRunningOnAppleSilicon;`
`165`	`165`
`166`	`166`	`public string PathCombine(params string[] parts)`
`167`	`167`	`{`
Original file line number	Diff line number	Diff line change
`@@ -15,14 +15,12 @@ internal static class MsBuildCommandExtensions`
`15`	`15`	`/// <returns></returns>`
`16`	`16`	`public static CommandBuilder MsBuildCommand(this CommandBuilder cmdBuilder, IAutobuilder<AutobuildOptionsShared> builder)`
`17`	`17`	`{`
`18`		`- var isArmMac = builder.Actions.IsMacOs() && builder.Actions.IsArm();`
`19`		`-`
`20`	`18`	// mono doesn't ship with `msbuild` on Arm-based Macs, but we can fall back to
`21`	`19`	// msbuild that ships with `dotnet` which can be invoked with `dotnet msbuild`
`22`	`20`	`// perhaps we should do this on all platforms?`
`23`		`- return isArmMac ?`
`24`		`- cmdBuilder.RunCommand("dotnet").Argument("msbuild") :`
`25`		`- cmdBuilder.RunCommand("msbuild");`
	`21`	`+ return builder.Actions.IsRunningOnAppleSilicon()`
	`22`	`+ ? cmdBuilder.RunCommand("dotnet").Argument("msbuild")`
	`23`	`+ : cmdBuilder.RunCommand("msbuild");`
`26`	`24`	`}`
`27`	`25`	`}`
`28`	`26`