add support for getASavePath() to js/insecure-download

erik-krogh · erik-krogh · commit 8f5a3e9f4f4c · 2020-06-25T15:18:31.000+02:00
diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/InsecureDownloadCustomizations.qll b/javascript/ql/src/semmle/javascript/security/dataflow/InsecureDownloadCustomizations.qll
@@ -68,13 +68,21 @@ module InsecureDownload {
     override DataFlow::FlowLabel getALabel() {
       result instanceof Label::InsecureURL
       or
-      exists(string suffix | suffix = unsafeExtension() |
-        str.suffix(str.length() - suffix.length() - 1).toLowerCase() = "." + suffix
-      ) and
+      hasUnsafeExtension(str) and
       result instanceof Label::SensitiveInsecureURL
     }
   }
 
+  /**
+   * Holds if `str` is a string that ends with an unsafe file extension.
+   */
+  bindingset[str]
+  predicate hasUnsafeExtension(string str) {
+    exists(string suffix | suffix = unsafeExtension() |
+      str.suffix(str.length() - suffix.length() - 1).toLowerCase() = "." + suffix
+    )
+  }
+
   /**
    * Gets a file-extension that can potentially be dangerous.
    *
@@ -99,6 +107,9 @@ module InsecureDownload {
 
     override DataFlow::FlowLabel getALabel() {
       result instanceof Label::SensitiveInsecureURL // TODO: Also non-sensitive.
+      or
+      hasUnsafeExtension(request.getASavePath().getStringValue()) and
+      result instanceof Label::InsecureURL
     }
   }
 }
diff --git a/javascript/ql/test/query-tests/Security/CWE-829/InsecureDownload.expected b/javascript/ql/test/query-tests/Security/CWE-829/InsecureDownload.expected
@@ -17,6 +17,9 @@ nodes
 | insecure-download.js:41:12:41:41 | "ftp:// ... fe.APK" |
 | insecure-download.js:41:12:41:41 | "ftp:// ... fe.APK" |
 | insecure-download.js:41:12:41:41 | "ftp:// ... fe.APK" |
+| insecure-download.js:46:12:46:38 | "http:/ ... unsafe" |
+| insecure-download.js:46:12:46:38 | "http:/ ... unsafe" |
+| insecure-download.js:46:12:46:38 | "http:/ ... unsafe" |
 edges
 | insecure-download.js:9:27:9:138 | 'http:/ ... ll.exe' | insecure-download.js:15:18:15:40 | buildTo ... llerUrl |
 | insecure-download.js:9:27:9:138 | 'http:/ ... ll.exe' | insecure-download.js:15:18:15:40 | buildTo ... llerUrl |
@@ -30,9 +33,11 @@ edges
 | insecure-download.js:36:15:36:45 | "http:/ ... fe.APK" | insecure-download.js:36:9:36:45 | url |
 | insecure-download.js:36:15:36:45 | "http:/ ... fe.APK" | insecure-download.js:36:9:36:45 | url |
 | insecure-download.js:41:12:41:41 | "ftp:// ... fe.APK" | insecure-download.js:41:12:41:41 | "ftp:// ... fe.APK" |
+| insecure-download.js:46:12:46:38 | "http:/ ... unsafe" | insecure-download.js:46:12:46:38 | "http:/ ... unsafe" |
 #select
 | insecure-download.js:5:16:5:28 | installer.url | insecure-download.js:9:27:9:138 | 'http:/ ... ll.exe' | insecure-download.js:5:16:5:28 | installer.url | $@ of sensitive file from $@. | insecure-download.js:5:9:5:44 | nugget( ... => { }) | Download | insecure-download.js:9:27:9:138 | 'http:/ ... ll.exe' | HTTP source |
 | insecure-download.js:30:12:30:42 | "http:/ ... fe.APK" | insecure-download.js:30:12:30:42 | "http:/ ... fe.APK" | insecure-download.js:30:12:30:42 | "http:/ ... fe.APK" | $@ of sensitive file from $@. | insecure-download.js:30:5:30:43 | nugget( ... e.APK") | Download | insecure-download.js:30:12:30:42 | "http:/ ... fe.APK" | HTTP source |
 | insecure-download.js:37:23:37:25 | url | insecure-download.js:36:15:36:45 | "http:/ ... fe.APK" | insecure-download.js:37:23:37:25 | url | $@ of sensitive file from $@. | insecure-download.js:37:5:37:42 | cp.exec ...  () {}) | Download | insecure-download.js:36:15:36:45 | "http:/ ... fe.APK" | HTTP source |
 | insecure-download.js:39:26:39:28 | url | insecure-download.js:36:15:36:45 | "http:/ ... fe.APK" | insecure-download.js:39:26:39:28 | url | $@ of sensitive file from $@. | insecure-download.js:39:5:39:46 | cp.exec ...  () {}) | Download | insecure-download.js:36:15:36:45 | "http:/ ... fe.APK" | HTTP source |
 | insecure-download.js:41:12:41:41 | "ftp:// ... fe.APK" | insecure-download.js:41:12:41:41 | "ftp:// ... fe.APK" | insecure-download.js:41:12:41:41 | "ftp:// ... fe.APK" | $@ of sensitive file from $@. | insecure-download.js:41:5:41:42 | nugget( ... e.APK") | Download | insecure-download.js:41:12:41:41 | "ftp:// ... fe.APK" | HTTP source |
+| insecure-download.js:46:12:46:38 | "http:/ ... unsafe" | insecure-download.js:46:12:46:38 | "http:/ ... unsafe" | insecure-download.js:46:12:46:38 | "http:/ ... unsafe" | $@ of sensitive file from $@. | insecure-download.js:46:5:46:71 | nugget( ... => { }) | Download | insecure-download.js:46:12:46:38 | "http:/ ... unsafe" | HTTP source |
diff --git a/javascript/ql/test/query-tests/Security/CWE-829/insecure-download.js b/javascript/ql/test/query-tests/Security/CWE-829/insecure-download.js
@@ -46,4 +46,6 @@ var writeFileAtomic = require("write-file-atomic");
 
 function test() {
     nugget("http://example.org/unsafe", {target: "foo.exe"}, () => { }) // NOT OK
+
+    nugget("http://example.org/unsafe", {target: "foo.safe"}, () => { }) // OK
 }

Original file line number	Diff line number	Diff line change
`@@ -68,13 +68,21 @@ module InsecureDownload {`
`68`	`68`	`override DataFlow::FlowLabel getALabel() {`
`69`	`69`	`result instanceof Label::InsecureURL`
`70`	`70`	`or`
`71`		`- exists(string suffix \| suffix = unsafeExtension() \|`
`72`		`- str.suffix(str.length() - suffix.length() - 1).toLowerCase() = "." + suffix`
`73`		`- ) and`
	`71`	`+ hasUnsafeExtension(str) and`
`74`	`72`	`result instanceof Label::SensitiveInsecureURL`
`75`	`73`	`}`
`76`	`74`	`}`
`77`	`75`
	`76`	`+ /**`
	`77`	+ * Holds if `str` is a string that ends with an unsafe file extension.
	`78`	`+ */`
	`79`	`+ bindingset[str]`
	`80`	`+ predicate hasUnsafeExtension(string str) {`
	`81`	`+ exists(string suffix \| suffix = unsafeExtension() \|`
	`82`	`+ str.suffix(str.length() - suffix.length() - 1).toLowerCase() = "." + suffix`
	`83`	`+ )`
	`84`	`+ }`
	`85`	`+`
`78`	`86`	`/**`
`79`	`87`	`* Gets a file-extension that can potentially be dangerous.`
`80`	`88`	`*`
`@@ -99,6 +107,9 @@ module InsecureDownload {`
`99`	`107`
`100`	`108`	`override DataFlow::FlowLabel getALabel() {`
`101`	`109`	`result instanceof Label::SensitiveInsecureURL // TODO: Also non-sensitive.`
	`110`	`+ or`
	`111`	`+ hasUnsafeExtension(request.getASavePath().getStringValue()) and`
	`112`	`+ result instanceof Label::InsecureURL`
`102`	`113`	`}`
`103`	`114`	`}`
`104`	`115`	`}`
Original file line number	Diff line number	Diff line change
`@@ -46,4 +46,6 @@ var writeFileAtomic = require("write-file-atomic");`
`46`	`46`
`47`	`47`	`function test() {`
`48`	`48`	`nugget("http://example.org/unsafe", {target: "foo.exe"}, () => { }) // NOT OK`
	`49`	`+`
	`50`	`+ nugget("http://example.org/unsafe", {target: "foo.safe"}, () => { }) // OK`
`49`	`51`	`}`