stylistic improvements after review

Author: Stephan Brandauer

javascript/ql/src/Security/CWE-830/FunctionalityFromUntrustedSource.qhelp

@@ -5,7 +5,7 @@
 	<overview>
 		<p>
 
-			Including functionality from an external source via an http URL may
+			Including functionality from an external source via an <code>http</code> URL may
 			allow an attacker to inject malicious code via a MITM (man-in-the-middle) attack.
 
 		</p>
@@ -15,24 +15,25 @@
 	<recommendation>
 		<p>
 
-			When including external pages or behaviour, use <em>https</em> URLs to make sure you're
-			getting the intended data, or users will be vulnerable to MITM attacks.
+			When including external pages or behaviour, use <code>https</code> URLs 
+			to make sure you're getting the intended data, or users will be vulnerable
+			to MITM attacks.
 
 		</p>
 
 		<p>
 
-			When including external behaviour in iframe or script elements, using http URLs is
-			unsafe because the request sent may be intercepted by an attacker, and malicious data
-			may be sent back in reply.
+			When including external behaviour in iframe or script elements, using 
+			<code>http</code> URLs is unsafe because the request sent may be intercepted
+			by an attacker, and malicious data may be sent back in reply.
 
 		</p>
 
 
 		<p>
 
-			Even when https is used, an attacker might still compromise the server the page is
-			receving data from.
+			Even when <code>https</code> is used, an attacker might still compromise the
+			server the page is receving data from.
 
 			When including scripts from a CDN (content-delivery network), it is therefore recommended
 			to set the integrity-attribute on the script tag to the hash of the script you're expecting

javascript/ql/src/Security/CWE-830/FunctionalityFromUntrustedSource.ql

@@ -22,24 +22,13 @@ module Generic {
     call.getArgument(0).getStringValue().toLowerCase() = name
   }
 
-  /**
-   * True if `rhs` is being assigned to the `propName` property of an HTML
-   * element created by `createElementCall`.
-   */
-  predicate isPropWrite(DataFlow::CallNode createElementCall, string propName, DataFlow::Node rhs) {
-    exists(DataFlow::PropWrite assignment |
-      isCreateElementNode(createElementCall, _) and
-      assignment.writes(createElementCall.getALocalUse(), propName, rhs)
-    )
-  }
-
   /**
    * A `createElement` call that creates a `<script ../>` element which never
    * has its `integrity` attribute set locally.
    */
   predicate isCreateScriptNodeWoIntegrityCheck(DataFlow::CallNode createCall) {
     isCreateElementNode(createCall, "script") and
-    not exists(DataFlow::Node rhs | isPropWrite(createCall, "integrity", rhs))
+    not exists(createCall.getAPropertyWrite("integrity"))
   }
 
   /** A location that adds a reference to an untrusted source. */
@@ -54,14 +43,14 @@ module StaticCreation {
   predicate isLocalhostPrefix(string host) {
     host.toLowerCase()
         .regexpMatch([
-            "localhost(:[0-9]+)?/.*", "127.0.0.1(:[0-9]+)?/.*", "::1/.*", "\\[::1\\]:[0-9]+/.*"
+            "(?i)localhost(:[0-9]+)?/.*", "127.0.0.1(:[0-9]+)?/.*", "::1/.*", "\\[::1\\]:[0-9]+/.*"
           ])
   }
 
   /** A path that is vulnerable to a MITM attack. */
   bindingset[url]
   predicate isUntrustedSourceUrl(string url) {
-    exists(string hostPath | hostPath = url.regexpCapture("http://(.*)", 1) |
+    exists(string hostPath | hostPath = url.regexpCapture("(?i)http://(.*)", 1) |
       not isLocalhostPrefix(hostPath)
     )
   }
@@ -71,10 +60,11 @@ module StaticCreation {
   predicate isCdnUrlWithCheckingRequired(string url) {
     // Some CDN URLs are required to have an integrity attribute. We only add CDNs to that list
     // that recommend integrity-checking.
-    url.regexpMatch([
-        "^https?://code\\.jquery\\.com/.*\\.js$", "^https?://cdnjs\\.cloudflare\\.com/.*\\.js$",
-        "^https?://cdnjs\\.com/.*\\.js$"
-      ])
+    url.regexpMatch("(?i)" +
+        [
+          "^https?://code\\.jquery\\.com/.*\\.js$", "^https?://cdnjs\\.cloudflare\\.com/.*\\.js$",
+          "^https?://cdnjs\\.com/.*\\.js$"
+        ])
   }
 
   /** A script element that refers to untrusted content. */
@@ -85,7 +75,7 @@ module StaticCreation {
     }
 
     override string getProblem() {
-      result = "script elements should use an HTTPS url and/or use the integrity attribute"
+      result = "script elements should use an 'https:' URL and/or use the integrity attribute"
     }
   }
 
@@ -98,25 +88,23 @@ module StaticCreation {
 
     override string getProblem() {
       result =
-        "script elements that depend on this CDN should use an HTTPS url and use the integrity attribute"
+        "script elements that depend on this CDN should use an 'https:' URL and use the integrity attribute"
     }
   }
 
   /** An iframe element that includes untrusted content. */
   class IframeElementWithUntrustedContent extends HTML::IframeElement, Generic::AddsUntrustedUrl {
     IframeElementWithUntrustedContent() { isUntrustedSourceUrl(this.getSourcePath()) }
 
-    override string getProblem() { result = "iframe elements should use an HTTPS url" }
+    override string getProblem() { result = "iframe elements should use an 'https:' URL" }
   }
 }
 
 module DynamicCreation {
   import DataFlow::TypeTracker
 
   predicate isUnsafeSourceLiteral(DataFlow::Node source) {
-    exists(StringLiteral s | source = s.flow() |
-      s.getValue().toLowerCase() = "http:" + any(string rest)
-    )
+    exists(StringLiteral s | source = s.flow() | s.getValue().regexpMatch("(?i)http:.*"))
   }
 
   DataFlow::Node urlTrackedFromUnsafeSourceLiteral(DataFlow::TypeTracker t) {
@@ -150,11 +138,11 @@ module DynamicCreation {
     exists(DataFlow::CallNode createElementCall |
       name = "script" and
       Generic::isCreateScriptNodeWoIntegrityCheck(createElementCall) and
-      Generic::isPropWrite(createElementCall, "src", sink)
+      sink = createElementCall.getAPropertyWrite("src").getRhs()
       or
       name = "iframe" and
       Generic::isCreateElementNode(createElementCall, "iframe") and
-      Generic::isPropWrite(createElementCall, "src", sink)
+      sink = createElementCall.getAPropertyWrite("src").getRhs()
     )
   }
 
@@ -170,9 +158,9 @@ module DynamicCreation {
 
     override string getProblem() {
       name = "script" and
-      result = "script elements should use an HTTPS url and/or use the integrity attribute"
+      result = "script elements should use an 'https:' URL and/or use the integrity attribute"
       or
-      name = "iframe" and result = "iframe elements should use an HTTPS url"
+      name = "iframe" and result = "iframe elements should use an 'https:' URL"
     }
   }
 }

javascript/ql/test/query-tests/Security/CWE-830/FunctionalityFromUntrustedSource.expected

@@ -1,5 +1,5 @@
-| DynamicCreationOfUntrustedSourceUse.html:18:26:18:50 | 'http:/ ... e.com/' | HTML-element uses untrusted content (iframe elements should use an HTTPS url) |
-| DynamicCreationOfUntrustedSourceUse.html:29:27:29:40 | getUrl('v123') | HTML-element uses untrusted content (iframe elements should use an HTTPS url) |
-| StaticCreationOfUntrustedSourceUse.html:6:9:6:56 | <script>...</> | HTML-element uses untrusted content (script elements should use an HTTPS url and/or use the integrity attribute) |
-| StaticCreationOfUntrustedSourceUse.html:9:9:9:58 | <iframe>...</> | HTML-element uses untrusted content (iframe elements should use an HTTPS url) |
-| StaticCreationOfUntrustedSourceUse.html:21:9:21:155 | <script>...</> | HTML-element uses untrusted content (script elements that depend on this CDN should use an HTTPS url and use the integrity attribute) |
+| DynamicCreationOfUntrustedSourceUse.html:18:26:18:50 | 'http:/ ... e.com/' | HTML-element uses untrusted content (iframe elements should use an 'https:' URL) |
+| DynamicCreationOfUntrustedSourceUse.html:29:27:29:40 | getUrl('v123') | HTML-element uses untrusted content (iframe elements should use an 'https:' URL) |
+| StaticCreationOfUntrustedSourceUse.html:6:9:6:56 | <script>...</> | HTML-element uses untrusted content (script elements should use an 'https:' URL and/or use the integrity attribute) |
+| StaticCreationOfUntrustedSourceUse.html:9:9:9:58 | <iframe>...</> | HTML-element uses untrusted content (iframe elements should use an 'https:' URL) |
+| StaticCreationOfUntrustedSourceUse.html:21:9:21:155 | <script>...</> | HTML-element uses untrusted content (script elements that depend on this CDN should use an 'https:' URL and use the integrity attribute) |