Model websocket read and write functions.

[ghost]

No description provided.

[ghost]

I need a little help on handling the io.Reader and io.Writer types. See comments below.

[ghost]

This module would be merged with the websocket.qll from the other PR. Since that has not been merged yet, I created a new file for this PR.

[ghost]

The hardcoded golang.org/x/net/websocket would be replaced before merge.

[max-schaefer]

#129 might help with this, though more API modeling is probably needed.

[ghost]

I have re-modeled the api. The getSink calls are now gone. I just return the source/sink directly.

I have also added library tests for the functions. As of now, all but one test cases are detected.

The query fails to detect the following block of code.

codeql-go/ql/test/library-tests/semmle/go/frameworks/Websocket/WebsocketReadWrite.go

Lines 40 to 42 in 0023c06

	_, nhooyrReader, _ := n.Reader(context.TODO())
	writer, _ := n.Writer(context.TODO(), 0)
	io.Copy(writer, nhooyrReader)

This is because I haven't modelled the io package here. I am sending in a separate PR for the same soon.

I think this is ready for a review otherwise.

[ghost]

The test fail is expected as I haven't included the .expected file yet. I am waiting #131 to be merged. Once that is merged, I would get the necessary io modelling for this to detect all cases.

[sauyon]

I've stubbed dependencies for you and removed the changes to the top-level go.mod.

[sauyon]

Why is gobwas imported twice here?

[sauyon]

Some initial comments.

[sauyon]

Suggested change

[ghost]

By definition, a file is a valid file only if it has a newline at the end. Most applications don't depend on this but the auto-formatter added so I kept it as is.

[sauyon]

Right, but you've added trailing whitespace to that line, which is what I wanted to get rid of.

[ghost]

I tried removing that and running auto format running it adds it again. I can see there's only one new line at the end of the file.

[ghost]

Nvm, I figured it out. It was a issue on my side. I have fixed it now

[sauyon]

Maybe this should be called WebsocketMessage instead?

[sauyon]

Suggested change

	* the https://golang.org/x/net/websocket package.
	* the [golang.org/x/net/websocket](https://golang.org/x/net/websocket) package.

And similar below.

[sauyon]

Suggested change

	* Extends this class to model new APIs. If you want to refine existing API models,
	* Extend this class to model new APIs. If you want to refine existing API models,

[sauyon]

Suggested change

	* Extends this class to refine existing API models. If you want to model new APIs,
	* Extend this class to refine existing API models. If you want to model new APIs,

(It seems this is an error copy-pasted from a few other places in the codebase; maybe you could sed -i 's/Extends this class/Extend this class' or something like that?)

[sauyon]

I think these names are a bit misleading, since they're not actually methods or functions.

Maybe just call them Messages?

[max-schaefer]

I note, though, that in each case this is bound to an exit node of a FunctionOutput, so it seems to me like these classes really do want to be functions, not data-flow nodes.

[ghost]

I think, we can rename the classes from something like GolangXNetConnReadMethod to GolangXNetConnRead. It is,in principle true, they are not functions or methods but calling them *Message is also not ideal. They may represent writers or other types too. WDYT?

[sauyon]

Suggested change

	this.(DataFlow::SsaNode).getInit() = m.getResult(0)
	this = m.getResult(0)

I think this should suffice for simple values like these.

[max-schaefer]

And also abov: the SsaNode.getInit trick should only be needed for results that are writers, I think.

[max-schaefer]

As above, I wonder whether it might be easier to model functions instead of data-flow nodes here, and specify for each function which FunctionInput is written to a web socket.

[ghost]

I am not sure I understand. Do you mean having something like this?

class A extends Function {
    getSink(){result= argument}
}

or

class A extends FunctionNode{
    getSink(){
        result = parameter
    }
}

[sauyon]

Something like

class A extends Function {
  FunctionOutput getContent() {
    result.isParameter(1) // or similar
  }
}

I would think.

[max-schaefer]

What @sauyon says, though I think it should be FunctionInput.

Concretely, I would suggest rewriting WebsocketWrite as

class WebSocketWriter extends Function {
  WebSocketWriter::Range self;

  WebSocketWriter() { this = self }

  /** Gets an input to this function that is written to a WebSocket connection. */
  FunctionInput getAnInput() { result = self.getAnInput() }
}

and then define

module WebSocketWriter {
  abstract class Range extends Function {
    abstract FunctionInput getAnInput();
  }

  private class GolangXNetCodecSend extends Range, Method {
    GolangXNetCodecSend() { this.hasQualifiedName("golang.org/x/net/websocket", "Codec", "Send") }

    FunctionInput getAnInput() { result.isParameter(1) }
  }

  ...
}

Data-flow nodes that can be written to a WebSocket can then be identified as

exists(WebSocketWriter w |
  nd = w.getAnInput().getNode(w.getACall())
)

As you can see, this simplifies the definitions of the subclasses of WebSocketWriter::Range by separating the problem of identifying which function inputs get written to a WebSocket from finding the corresponding nodes for a concrete invocation.

The same comments apply to the modelling of WebSocket reads above, but with FunctionOutput instead of FunctionInput.

[ghost]

Hey! can someone check what's wrong with the CI? I run tests on my local node, they pass but on the CI they fail.
The expected file contains the correct results. However, it looks like the CI is taking a different earlier version of the file. Making the test fail.

[max-schaefer]

Hey! can someone check what's wrong with the CI? I run tests on my local node, they pass but on the CI they fail.
The expected file contains the correct results. However, it looks like the CI is taking a different earlier version of the file. Making the test fail.

I take it you have resolved this issue? The tests now seem to pass.

[max-schaefer]

A few more comments and clarifications. Thanks for persevering through what I realise is quite a drawn-out review process. We really appreciate your contributions!

@sauyon, it looks like you haven't added the LICENSE files for the stubbed libraries yet.

[max-schaefer]

Suggested change

	* the [https://golang.org/x/net/websocket](https://golang.org/x/net/websocket) package.
	* the https://golang.org/x/net/websocket package.

[max-schaefer]

There are a few other instances of spurious multi-whitespace after * below. I trust you can find them even though I have not flagged them up individually.

[max-schaefer]

What @sauyon says, though I think it should be FunctionInput.

Concretely, I would suggest rewriting WebsocketWrite as

class WebSocketWriter extends Function {
  WebSocketWriter::Range self;

  WebSocketWriter() { this = self }

  /** Gets an input to this function that is written to a WebSocket connection. */
  FunctionInput getAnInput() { result = self.getAnInput() }
}

and then define

module WebSocketWriter {
  abstract class Range extends Function {
    abstract FunctionInput getAnInput();
  }

  private class GolangXNetCodecSend extends Range, Method {
    GolangXNetCodecSend() { this.hasQualifiedName("golang.org/x/net/websocket", "Codec", "Send") }

    FunctionInput getAnInput() { result.isParameter(1) }
  }

  ...
}

Data-flow nodes that can be written to a WebSocket can then be identified as

exists(WebSocketWriter w |
  nd = w.getAnInput().getNode(w.getACall())
)

As you can see, this simplifies the definitions of the subclasses of WebSocketWriter::Range by separating the problem of identifying which function inputs get written to a WebSocket from finding the corresponding nodes for a concrete invocation.

The same comments apply to the modelling of WebSocket reads above, but with FunctionOutput instead of FunctionInput.

[ghost]

As I had mentioned above, #109 (comment) There was a test case involving io.Copy function which the query failed to detect. Now that #129 and #131 have been merged, this query correctly detects the flow through readers and writers and hence detects the test case I mentioned above.

I have updated the query and added the new result. I have also squashed and merged all the commits into a single one for easier merging.

Please note, while this can now merged, please don't run an evaluation against all lgtm projects just yet. I will model the bufio and encoding packages and turn in a PR soon which should ideally result in an increase in test results.

[max-schaefer]

Mostly LGTM, modulo a few typos and missing licenses for stubbed test dependencies (@sauyon). I have started an evaluation.

[max-schaefer]

@sauyon: I think you forgot to include the LICENSE file when stubbing. (Time to do something about that warning you mentioned?)

[max-schaefer]

This should presumably be Send, but I'm not sure about the function signature.

[sauyon]

Added licenses, sorry.

[max-schaefer]

Evaluation shows no performance regressions, but a few new results involving flow from a WebSocket read to a WebSocket write, for example from here to here. Thinking about this, I'm not entirely sure I see why that would lead to an XSS vulnerability. @porcupineyhairs, can you comment on that?

[ghost]

@max-schaefer The message is directly returned to the connection without sanitization. This allows an attacker to potentially send in JS code. When this is received again on the client side, the code will be execute resulting in an alert.
For ex, consider the following code.

$("html").append("<html>"+ws.receive()+"</html>")

Since the message is reflected back, ws.receive can be made to returned anything which you want resulting in an XSS.

You can see even in the corresponding non websocket XSS testcase, see that we alert it as an xss when the username parameter is written directly to the http response.

[sauyon]

We generally try to exclude things that require client-side code in order to be exploitable in order to avoid false positives. In this case in particular, I would guess most websocket responses are not written to an HTML body, and are therefore not exploitable. Is there some reason that isn't the case?

You can see even in the corresponding non websocket XSS testcase, see that we alert it as an xss when the username parameter is written directly to the http response.

For that example, a user clicking a malicious link to that page would immediately be vulnerable, with no client-side code needed.

[max-schaefer]

@porcupineyhairs, thank you for your explanation. I find it quite implausible that this is what's happening in this case, but maybe you have some evidence that it is?

I realise that there are scenarios where an unsuspecting user might be tricked into doing something that sends unsanitised data across a WebSocket, and the reflected data is then embedded into HTML. But detecting that based purely on server-side code (as we are doing here) seems difficult if not impossible, and you don't seem to have implemented any logic that even tries to do that.

As it stands, the potential for false positives seems way too high to me.

[ghost]

Please allow me to give you a more elaborate explaination.

First off, the query adds support for read as well as write functions. Since an attacker may control data which is read by a server, all reads on the server side should be marked as remote flow sources.

(I can see I have made an error and added WebSocket read as a source for only the XSS query. This was included initially but got lost somewhere durign the review. I have corrected it now and included with the other remote flow sources. You should try running an evaluation again now.)

most websocket responses are not written to an HTML body, and are therefore not exploitable

While technically you can use WebSockets to send other forms of data such as images or templates, it seems unlikely any application does it in real life. Traditional HTTP stack with caching usually in several layers beats WebSockets any time for serving images and other binary data. Plus browsers started supporting WebSockets only up until recently so you loose backwards compatibility for practially nothing. The case which Max pointed out is not an actual application but rather a test for RFC standard compliance.

As you may have noticed, some of the API's make do a distinction between binary and text messages. Most of the users of these API's from what I could see are chat applications and the like who use the binary mode to send encoded JSON/protobuf streams. The encoding is then decoded on the client side and the original message is obtained. The message in most cases at least in part, ends up in the html. Hence I made the decision to include writes to the websocket connection as well. However, I may be wrong here as I can't back my claims by any real stats.

I haven't seen the results of the eval yet. If there are not that many FP's, I would recommend that we keep the writes as is. If there are too many FP's, here's what I propose.

• We could try limiting to only text message but it is very unlikely that any real life application uses this means of messaging. All of the instances I have seen atleast json/protobuf encode before sending and hence use the binary messaging mode.
• WebSocket Writes can be refactored as sinks for a sensitive information leak query. There have been cases where PII was accessible over a simple Websocket connection. See here.
• I could try filtering the sinks only to non test files. Basically, mark any file with test anywhere in its path as a test query.

Please let me know what your thoughts are here.

[max-schaefer]

While technically you can use WebSockets to send other forms of data such as images or templates, it seems unlikely any application does it in real life.

A sweeping claim like that isn't very convincing without supporting data, I'm afraid.

I think @sauyon put it best: you can't tell from a WebSocket write how the data written to it will be used, so in the interest of avoiding false positives we will need to assume that it is safe.

I am, however, fine with treating data read from a WebSocket as untrusted, so I would suggest simply removing the extra XSS sink. What do you think, @sauyon?

[sauyon]

That sounds sensible to me.

[ghost]

I have rebased and squashed the changes. You can now start an evaluation.

[max-schaefer]

Since this isn't the first unrelated/nonsensical comment we're seeing from this user, I have reported them for abuse. (EDIT: I was referring to the comment Sauyon deleted.)

[max-schaefer]

With #107 having gone in, this now needs conflict resolution.

[max-schaefer]

Evaluation looks fine, so this is good to go in. Many thanks for your contribution!

[owen-mc]

No change note required as it's covered by change-notes/2020-05-22-websocket-model.md