More cleanup

change-notes/2020-05-22-websocket-model.md

@@ -0,0 +1,3 @@
+lgtm,codescanning
+* Modeling of several WebSocket libraries has been added, which may lead to more results from the
+  security queries.

ql/src/experimental/CWE-807/SensitiveConditionBypass.qhelp

@@ -6,25 +6,26 @@ Testing untrusted user input against a fixed constant results in
 a bypass of the conditional check as the attacker may alter the input to match the constant.
 When an incorrect check of this type is used to guard a potentially sensitive block, 
 it results an attacker gaining access to the sensitive block.
-</p>
+        </p>
     </overview>
     <recommendation>
         <p>
  Never decide whether to authenticate a user based on data that may be controlled by that user. 
  If necessary, ensure that the data is validated extensively when it is input before any 
  authentication checks are performed.
-</p>
-<p>
+        </p>
+        <p>
 It is still possible to have a system that "remembers" users, thus not requiring 
 the user to login on every interaction. For example, personalization settings can be applied 
 without authentication because this is not sensitive information. However, users
-should be allowed to take sensitive actions only when they have been fully authenticated. 
+should be allowed to take sensitive actions only when they have been fully authenticated.
+        </p>
     </recommendation>
     <example>
         <p>
 The following example shows a comparison where an user controlled 
 expression is used to guard a sensitive method. This should be avoided.:
-</p>
+        </p>
         <sample src="SensitiveConditionBypassBad.go" />
     </example>
 </qhelp>

ql/src/go.qll

@@ -17,23 +17,23 @@ import semmle.go.Scopes
 import semmle.go.Stmt
 import semmle.go.StringOps
 import semmle.go.Types
+import semmle.go.Util
 import semmle.go.controlflow.BasicBlocks
 import semmle.go.controlflow.ControlFlowGraph
 import semmle.go.controlflow.IR
 import semmle.go.dataflow.DataFlow
 import semmle.go.dataflow.GlobalValueNumbering
-import semmle.go.dataflow.TaintTracking
 import semmle.go.dataflow.SSA
+import semmle.go.dataflow.TaintTracking
 import semmle.go.frameworks.Email
 import semmle.go.frameworks.HTTP
 import semmle.go.frameworks.Macaron
 import semmle.go.frameworks.Mux
 import semmle.go.frameworks.NoSQL
-import semmle.go.frameworks.SystemCommandExecutors
 import semmle.go.frameworks.SQL
-import semmle.go.frameworks.XPath
 import semmle.go.frameworks.Stdlib
+import semmle.go.frameworks.SystemCommandExecutors
 import semmle.go.frameworks.Testing
-import semmle.go.frameworks.Websocket
+import semmle.go.frameworks.WebSocket
+import semmle.go.frameworks.XPath
 import semmle.go.security.FlowSources
-import semmle.go.Util

ql/src/semmle/go/Packages.qll

@@ -26,10 +26,13 @@ class Package extends @package {
 }
 
 /**
- * Gets the Go import string that may identify a package in module `mod` with the given path,
- * possibly modulo semantic import versioning.
+ * Gets an import path that identifies a package in module `mod` with the given path,
+ * possibly modulo [semantic import versioning](https://github.com/golang/go/wiki/Modules#semantic-import-versioning).
+ *
+ * For example, `package("github.com/go-pg/pg", "types")` gets an import path that can
+ * refer to `"github.com/go-pg/pg/types"`, but also to `"github.com/go-pg/pg/v10/types"`.
  */
 bindingset[result, mod, path]
 string package(string mod, string path) {
-  result.regexpMatch("\\Q" + mod + "\\E([/.]v[^/]+)?/\\Q" + path + "\\E")
+  result.regexpMatch("\\Q" + mod + "\\E([/.]v[^/]+)?($|/)\\Q" + path + "\\E")
 }

ql/src/semmle/go/dataflow/BarrierGuardUtil.qll

@@ -1,6 +1,5 @@
 /**
- * Contains implementations of some commonly used barrier
- * guards for sanitizing untrusted URLs.
+ * Provides implementations of some commonly used barrier guards for sanitizing untrusted URLs.
  */
 
 import go

ql/src/semmle/go/frameworks/SQL.qll

@@ -76,11 +76,11 @@ module SQL {
 
     /** A string that might identify package `go-pg/pg` or a specific version of it. */
     bindingset[result]
-    private string gopg() { result.regexpMatch("github.com/go-pg/pg(/v[^/]+)?") }
+    private string gopg() { result = package("github.com/go-pg/pg", "") }
 
     /** A string that might identify package `go-pg/pg/orm` or a specific version of it. */
     bindingset[result]
-    private string gopgorm() { result.regexpMatch("github.com/go-pg/pg(/v[^/]+)?/orm") }
+    private string gopgorm() { result = package("github.com/go-pg/pg", "orm") }
 
     /**
      * A string argument to an API of `go-pg/pg` that is directly interpreted as SQL without

ql/src/semmle/go/frameworks/Websocket.qll → ql/src/semmle/go/frameworks/WebSocket.qll

@@ -3,7 +3,7 @@
 import go
 
 /**
- * A data-flow node that establishes a new WebSocket connection.
+ * A function call that establishes a new WebSocket connection.
  *
  * Extend this class to refine existing API models. If you want to model new APIs,
  * extend `WebSocketRequestCall::Range` instead.
@@ -20,7 +20,7 @@ class WebSocketRequestCall extends DataFlow::CallNode {
 /** Provides classes for working with WebSocket request functions. */
 module WebSocketRequestCall {
   /**
-   * A data-flow node that establishes a new WebSocket connection.
+   * A function call that establishes a new WebSocket connection.
    *
    * Extend this class to model new APIs. If you want to refine existing
    * API models, extend `WebSocketRequestCall` instead.
@@ -31,8 +31,7 @@ module WebSocketRequestCall {
   }
 
   /**
-   * A WebSocket request expression string used in an API function of the
-   * `golang.org/x/net/websocket` package.
+   * A call to the `Dial` function of the `golang.org/x/net/websocket` package.
    */
   private class GolangXNetDialFunc extends Range {
     GolangXNetDialFunc() {
@@ -44,8 +43,7 @@ module WebSocketRequestCall {
   }
 
   /**
-   * A WebSocket DialConfig expression string used in an API function
-   * of the `golang.org/x/net/websocket` package.
+   * A call to the `DialConfig` function of the `golang.org/x/net/websocket` package.
    */
   private class GolangXNetDialConfigFunc extends Range {
     GolangXNetDialConfigFunc() {
@@ -64,13 +62,12 @@ module WebSocketRequestCall {
   }
 
   /**
-   * A WebSocket request expression string used in an API function
-   * of the `github.com/gorilla/websocket` package.
+   * A call to the `Dialer` or `DialContext` function of the `github.com/gorilla/websocket` package.
    */
-  private class GorillaWebsocketDialFunc extends Range {
+  private class GorillaWebSocketDialFunc extends Range {
     DataFlow::Node url;
 
-    GorillaWebsocketDialFunc() {
+    GorillaWebSocketDialFunc() {
       // func (d *Dialer) Dial(urlStr string, requestHeader http.Header) (*Conn, *http.Response, error)
       // func (d *Dialer) DialContext(ctx context.Context, urlStr string, requestHeader http.Header) (*Conn, *http.Response, error)
       exists(string name, Method f |
@@ -87,8 +84,7 @@ module WebSocketRequestCall {
   }
 
   /**
-   * A WebSocket request expression string used in an API function
-   * of the `github.com/gobwas/ws` package.
+   * A call to the `Dialer.Dial` method of the `github.com/gobwas/ws` package.
    */
   private class GobwasWsDialFunc extends Range {
     GobwasWsDialFunc() {
@@ -106,11 +102,10 @@ module WebSocketRequestCall {
   }
 
   /**
-   * A WebSocket request expression string used in an API function
-   * of the `nhooyr.io/websocket` package.
+   * A call to the `Dial` function of the `nhooyr.io/websocket` package.
    */
-  private class NhooyrWebsocketDialFunc extends Range {
-    NhooyrWebsocketDialFunc() {
+  private class NhooyrWebSocketDialFunc extends Range {
+    NhooyrWebSocketDialFunc() {
       // func Dial(ctx context.Context, u string, opts *DialOptions) (*Conn, *http.Response, error)
       this.getTarget().hasQualifiedName(package("nhooyr.io", "websocket"), "Dial")
     }
@@ -119,26 +114,24 @@ module WebSocketRequestCall {
   }
 
   /**
-   * A WebSocket request expression string used in an API function
-   * of the `github.com/sacOO7/gowebsocket` package.
+   * A call to the `BuildProxy` or `New` function of the `github.com/sacOO7/gowebsocket` package.
    */
   private class SacOO7DialFunc extends Range {
     SacOO7DialFunc() {
       // func BuildProxy(Url string) func(*http.Request) (*url.URL, error)
       // func New(url string) Socket
-      this.getTarget().hasQualifiedName("github.com/sacOO7/gowebsocket", ["New", "BuildProxy"])
+      this.getTarget().hasQualifiedName("github.com/sacOO7/gowebsocket", ["BuildProxy", "New"])
     }
 
     override DataFlow::Node getRequestUrl() { result = this.getArgument(0) }
   }
 }
 
-/*
+/**
  * A message written to a WebSocket, considered as a flow sink for reflected XSS.
  */
-
-class WebsocketReaderAsSource extends UntrustedFlowSource::Range {
-  WebsocketReaderAsSource() {
+class WebSocketReaderAsSource extends UntrustedFlowSource::Range {
+  WebSocketReaderAsSource() {
     exists(WebSocketReader r | this = r.getAnOutput().getNode(r.getACall()))
   }
 }
@@ -154,7 +147,7 @@ class WebSocketReader extends Function {
 
   WebSocketReader() { this = self }
 
-  /** Gets an output of this function that is read from a WebSocket connection. */
+  /** Gets an output of this function containing data that is read from a WebSocket connection. */
   FunctionOutput getAnOutput() { result = self.getAnOutput() }
 }
 
@@ -167,12 +160,12 @@ module WebSocketReader {
    * extend `WebSocketReader` instead.
    */
   abstract class Range extends Function {
-    /**Returns the parameter in which the function stores the message read. */
+    /** Gets an output of this function containing data that is read from a WebSocket connection. */
     abstract FunctionOutput getAnOutput();
   }
 
   /**
-   * Models the `Receive` method of the `golang.org/x/net/websocket` package.
+   * The `Codec.Receive` method of the `golang.org/x/net/websocket` package.
    */
   private class GolangXNetCodecRecv extends Range, Method {
     GolangXNetCodecRecv() {
@@ -184,7 +177,7 @@ module WebSocketReader {
   }
 
   /**
-   * Models the `Read` method of the `golang.org/x/net/websocket` package.
+   * The `Conn.Read` method of the `golang.org/x/net/websocket` package.
    */
   private class GolangXNetConnRead extends Range, Method {
     GolangXNetConnRead() {
@@ -196,10 +189,10 @@ module WebSocketReader {
   }
 
   /**
-   * Models the `Read` method of the `nhooyr.io/websocket` package.
+   * The `Conn.Read` method of the `nhooyr.io/websocket` package.
    */
-  private class NhooyrWebsocketRead extends Range, Method {
-    NhooyrWebsocketRead() {
+  private class NhooyrWebSocketRead extends Range, Method {
+    NhooyrWebSocketRead() {
       // func (c *Conn) Read(ctx context.Context) (MessageType, []byte, error)
       this.hasQualifiedName("nhooyr.io/websocket", "Conn", "Read")
     }
@@ -208,10 +201,10 @@ module WebSocketReader {
   }
 
   /**
-   * Models the `Reader` method of the `nhooyr.io/websocket` package.
+   * The `Conn.Reader` method of the `nhooyr.io/websocket` package.
    */
-  private class NhooyrWebsocketReader extends Range, Method {
-    NhooyrWebsocketReader() {
+  private class NhooyrWebSocketReader extends Range, Method {
+    NhooyrWebSocketReader() {
       // func (c *Conn) Reader(ctx context.Context) (MessageType, io.Reader, error)
       this.hasQualifiedName("nhooyr.io/websocket", "Conn", "Reader")
     }
@@ -220,7 +213,7 @@ module WebSocketReader {
   }
 
   /**
-   * Models the `ReadFrame`function of the `github.com/gobwas/ws` package.
+   * The `ReadFrame` function of the `github.com/gobwas/ws` package.
    */
   private class GobwasWsReadFrame extends Range {
     GobwasWsReadFrame() {
@@ -232,7 +225,7 @@ module WebSocketReader {
   }
 
   /**
-   * Models the `ReadHeader`function of the `github.com/gobwas/ws` package.
+   * The `ReadHeader` function of the `github.com/gobwas/ws` package.
    */
   private class GobwasWsReadHeader extends Range {
     GobwasWsReadHeader() {
@@ -244,10 +237,10 @@ module WebSocketReader {
   }
 
   /**
-   * Models the `ReadJson` function of the `github.com/gorilla/websocket` package.
+   * The `ReadJson` function of the `github.com/gorilla/websocket` package.
    */
-  private class GorillaWebsocketReadJson extends Range {
-    GorillaWebsocketReadJson() {
+  private class GorillaWebSocketReadJson extends Range {
+    GorillaWebSocketReadJson() {
       // func ReadJSON(c *Conn, v interface{}) error
       this.hasQualifiedName("github.com/gorilla/websocket", "ReadJSON")
     }
@@ -256,10 +249,10 @@ module WebSocketReader {
   }
 
   /**
-   * Models the `ReadJson` method of the `github.com/gorilla/websocket` package.
+   * The `Conn.ReadJson` method of the `github.com/gorilla/websocket` package.
    */
-  private class GorillaWebsocketConnReadJson extends Range, Method {
-    GorillaWebsocketConnReadJson() {
+  private class GorillaWebSocketConnReadJson extends Range, Method {
+    GorillaWebSocketConnReadJson() {
       // func (c *Conn) ReadJSON(v interface{}) error
       this.hasQualifiedName("github.com/gorilla/websocket", "Conn", "ReadJSON")
     }
@@ -268,10 +261,10 @@ module WebSocketReader {
   }
 
   /**
-   * Models the `ReadMessage` method of the `github.com/gorilla/websocket` package.
+   * The `Conn.ReadMessage` method of the `github.com/gorilla/websocket` package.
    */
-  private class GorillaWebsocketReadMessage extends Range, Method {
-    GorillaWebsocketReadMessage() {
+  private class GorillaWebSocketReadMessage extends Range, Method {
+    GorillaWebSocketReadMessage() {
       // func (c *Conn) ReadMessage() (messageType int, p []byte, err error)
       this.hasQualifiedName("github.com/gorilla/websocket", "Conn", "ReadMessage")
     }

ql/test/library-tests/semmle/go/frameworks/Websocket/DialFunction.expected → ql/test/library-tests/semmle/go/frameworks/WebSocket/DialFunction.expected

@@ -7,7 +7,7 @@
 | DialFunction.go:36:2:36:45 | call to Dial | DialFunction.go:36:31:36:44 | untrustedInput |
 | DialFunction.go:38:2:38:31 | call to BuildProxy | DialFunction.go:38:17:38:30 | untrustedInput |
 | DialFunction.go:39:2:39:24 | call to New | DialFunction.go:39:10:39:23 | untrustedInput |
-| WebsocketReadWrite.go:30:12:30:42 | call to Dial | WebsocketReadWrite.go:30:27:30:29 | uri |
-| WebsocketReadWrite.go:40:14:40:50 | call to Dial | WebsocketReadWrite.go:40:42:40:44 | uri |
-| WebsocketReadWrite.go:50:17:50:37 | call to Dial | WebsocketReadWrite.go:50:29:50:31 | uri |
-| WebsocketReadWrite.go:66:20:66:51 | call to Dial | WebsocketReadWrite.go:66:48:66:50 | uri |
+| WebSocketReadWrite.go:30:12:30:42 | call to Dial | WebSocketReadWrite.go:30:27:30:29 | uri |
+| WebSocketReadWrite.go:40:14:40:50 | call to Dial | WebSocketReadWrite.go:40:42:40:44 | uri |
+| WebSocketReadWrite.go:50:17:50:37 | call to Dial | WebSocketReadWrite.go:50:29:50:31 | uri |
+| WebSocketReadWrite.go:66:20:66:51 | call to Dial | WebSocketReadWrite.go:66:48:66:50 | uri |

ql/test/library-tests/semmle/go/frameworks/WebSocket/Read.expected

@@ -0,0 +1,8 @@
+| WebSocketReadWrite.go:31:7:31:10 | definition of xnet |
+| WebSocketReadWrite.go:35:3:35:7 | definition of xnet2 |
+| WebSocketReadWrite.go:41:3:41:40 | ... := ...[1] |
+| WebSocketReadWrite.go:44:3:44:48 | ... := ...[1] |
+| WebSocketReadWrite.go:51:7:51:16 | definition of gorillaMsg |
+| WebSocketReadWrite.go:55:3:55:10 | definition of gorilla2 |
+| WebSocketReadWrite.go:61:3:61:38 | ... := ...[1] |
+| WebSocketReadWrite.go:67:3:67:36 | ... := ...[0] |

ql/test/library-tests/semmle/go/frameworks/Websocket/Read.ql → ql/test/library-tests/semmle/go/frameworks/WebSocket/Read.ql

@@ -1,5 +1,5 @@
 import go
-import semmle.go.frameworks.Websocket
+import semmle.go.frameworks.WebSocket
 
 from WebSocketReader r, DataFlow::Node nd
 where nd = r.getAnOutput().getNode(r.getACall())

ql/test/library-tests/semmle/go/frameworks/Websocket/go.mod → ql/test/library-tests/semmle/go/frameworks/WebSocket/go.mod

@@ -1,4 +1,4 @@
-module codeql-go-tests/frameworks/Websocket
+module codeql-go-tests/frameworks/WebSocket
 
 go 1.14