Go: fasthttp #14123

am0o0 · 2023-09-02T12:43:43Z

Added fasthttp model.
Queries that this model supports contain SSRF, Access system file Queries like path traversal, All queries that are using UntrustedFlowSource, XSS-related queries that are using SharedXss::Sink, and queries that are using Http::Redirect
I directly extended the XSS Sink but as I understand now, it seems that I should work with HTTP::ResponseBody and somehow specify the content-type that is set for a related response. I couldn't implement this after many attempts in Codeql with my current knowledge.

github-advanced-security

CodeQL found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

mbg · 2023-09-18T08:20:57Z

Hi @amammad! Thank you so much for putting together models for the fasthttp library 🙏🏻

Is this PR ready for review by us? For starters, I have approved the workflow runs and there seem to be some failures related to your new tests there that you may want to look into.

If the PR is ready for review, could you update the description with details of your expectations regarding which queries the new models do and do not support?

am0o0 · 2023-09-18T13:32:37Z

@mbg :) Thanks for the clarification. I don't know if I understood exactly what you mean about the description, so please let me know if I didn't update the description of this pull request correctly.

go/ql/lib/semmle/go/frameworks/Fasthttp.qll

am0o0 · 2023-09-18T16:51:24Z

@mbg tests are ok on my side I don't have any idea why it failed here.

mbg · 2023-09-18T16:56:17Z

@amammad can you try rebasing your branch on main? It looks like the test failure is a result of us upgrading to Go 1.21 in CI in #13867 which also updated the expected test output for Go 1.21.

am0o0 · 2023-09-18T17:49:18Z

@mbg I think it is OK now.

mbg

It looks like the tests still aren't quite right. See my comment about making sure that you are using Go 1.21 and that you have built the autobuilder/extractor with Go 1.21.

I have also made a suggestion for the approach taken by the tests and a minor comment about the format of the changelog entry.

I have given the models a quick glance over, but still need to review them thoroughly.

go/ql/lib/change-notes/2023-18-09--add-support-for-fasthttp-framework.md

go/ql/test/consistency/UnexpectedFrontendErrors.expected

go/ql/test/library-tests/semmle/go/dataflow/FlowSteps/LocalFlowStep.expected

go/ql/test/library-tests/semmle/go/frameworks/Fasthttp/fasthttp.ql

go/ql/lib/semmle/go/frameworks/Fasthttp.qll

owen-mc · 2023-12-07T10:39:30Z

go/ql/lib/semmle/go/frameworks/Fasthttp.qll

+      exists(Method m |
+        m.hasQualifiedName(packagePath(), "Response", methodName) and
+        call = m.getACall() and
+        this = call.getArgument(0)
+        or
+        m.hasQualifiedName(packagePath(), "RequestCtx", ["Success", "SuccessString"]) and
+        call = m.getACall() and
+        this = call.getArgument(1)
+      ) and
+      methodName =
+        [
+          "AppendBody", "AppendBodyString", "SetBody", "SetBodyRaw", "SetBodyStream",
+          "SetBodyString", "Success", "SuccessString"
+        ]
+      or
+      exists(Method write, DataFlow::CallNode writeCall |
+        write.hasQualifiedName("io", "Writer", "Write") and
+        writeCall = write.getACall() and
+        ResponseBodyWriterFlow::flowsTo(writeCall.getReceiver()) and
+        this = writeCall.getArgument(0)
+      ) and
+      methodName = "BodyWriter"


Should writeCall be renamed to call, to use the field? Does that make sense with how call is used in getResponseWriter?

I wanted to ask you about this part which is related to DataFlow::SimpleGlobal which you wrote before, I couldn't use call in this part, I must do something like this for the isSource predicate of DataFlow::SimpleGlobal

private predicate responseBodyWriterResult(DataFlow::Node src) { exists(Method responseBodyWriter | responseBodyWriter.hasQualifiedName(packagePath(), "Response", "BodyWriter") and src = responseBodyWriter.getACall().getResult(0) ) }

to

private predicate responseBodyWriterResult(DataFlow::Node src) { exists(Method responseBodyWriter | responseBodyWriter.hasQualifiedName(packagePath(), "Response", "BodyWriter") and call = responseBodyWriter.getACall() and src = call.getResult(0) ) }

and somehow connect this call to the call inside the ResponseBody class.

I see. DataFlow::SimpleGlobal isn't suitable then, as you need the beginning and the end of the path, and I don't think it can give that. I will double check. The other options are DataFlow::Global, which seems like too much, or local flow, which stays within a function. I see that the other libraries that do this kind of thing use local flow. But I remember from when I recommended SimpleGlobal that I searched GitHub and found lots of examples of the return value of Response.BodyWriter being passed to other functions before Write was called on it.

I think you should start with local flow (either getASuccessor* or DataFlow::localFlow() and maybe use MRVA to see if that means you are missing a lot. (I mean run MRVA with a query like can't find local flow to a call to Write().

Btw, looking at the search results I see that the writer is often passed as the first argument to fmt.Fprintf, so you might want to include that as an alternative destination to io.Writer.Write().

Thanks! I see a lot of Response.BodyWriter() Also are using other than write(sink) or fmt.Fprintf(Response.BodyWriter(),template,sink..) so I don't know how to handle all of these now :)

An idea I just had: if a function has arguments which are a Writer w and some other things, then the odds are pretty good that it's going to pass (some of) the other arguments to w.Write(). So instead of fmt.Fprintf() you could say "the return value flows to an argument of a call to any function, and the response body is any other syntactic argument of that call." You would have to test out how good this heuristic is, either by looking through the search results I linked earlier and seeing if you think it works most of the time, or by coding it up and running MRVA on it. (And you'd have to keep a case for w.Write() as well.)

Ah, I have just found out that there is a way to adapt the SimpleGlobal approach to give you a source and a sink. I'll suggest it in a new review.

owen-mc

I have added a way of using SimpleGlobal with a source and a sink as two suggestions. Try it out. That will deal with the writer flowing to another function in the same project. It may also be looking for other external functions that the writer often goes to, like io.Writer.Write() and fmt.Fprintf(), and adding any that come up a lot in the relevant place.

go/ql/lib/semmle/go/frameworks/Fasthttp.qll

owen-mc · 2023-12-08T10:41:13Z

go/ql/lib/semmle/go/frameworks/Fasthttp.qll

+      exists(Method responseBodyWriter, DataFlow::CallNode writerCall |
+        responseBodyWriter.hasQualifiedName(packagePath(), "Response", "BodyWriter") and
+        call = responseBodyWriter.getACall() and
+        writerCall = any(Method write | write.hasQualifiedName("io", "Writer", "Write")).getACall() and
+        this = writerCall.getArgument(0) and
+        DataFlow::localFlow(call.getResult(0), writerCall.getReceiver())
+      )
+      or
+      exists(Method responseBodyWriter, DataFlow::CallNode writerCall |
+        responseBodyWriter.hasQualifiedName(packagePath(), "Response", "BodyWriter") and
+        call = responseBodyWriter.getACall() and
+        writerCall = any(Function fprintf | fprintf.hasQualifiedName("fmt", "Fprintf")).getACall() and
+        this = writerCall.getSyntacticArgument(any(int i | i > 1)) and
+        DataFlow::localFlow(call.getResult(0), writerCall.getArgument(0))
+      )


Suggested change

exists(Method responseBodyWriter, DataFlow::CallNode writerCall |

responseBodyWriter.hasQualifiedName(packagePath(), "Response", "BodyWriter") and

call = responseBodyWriter.getACall() and

writerCall = any(Method write | write.hasQualifiedName("io", "Writer", "Write")).getACall() and

this = writerCall.getArgument(0) and

DataFlow::localFlow(call.getResult(0), writerCall.getReceiver())

)

or

exists(Method responseBodyWriter, DataFlow::CallNode writerCall |

responseBodyWriter.hasQualifiedName(packagePath(), "Response", "BodyWriter") and

call = responseBodyWriter.getACall() and

writerCall = any(Function fprintf | fprintf.hasQualifiedName("fmt", "Fprintf")).getACall() and

this = writerCall.getSyntacticArgument(any(int i | i > 1)) and

DataFlow::localFlow(call.getResult(0), writerCall.getArgument(0))

)

exists(ResponseBodyWriterFlow::PathNode source, ResponseBodyWriterFlow::PathNode sink |

ResponseBodyWriterFlow::flowPath(source, sink) and

call = source.getNode() and

writerSinkAndBody(sink.getNode(), this)

)

owen-mc · 2023-12-08T10:44:46Z

go/ql/lib/semmle/go/frameworks/Fasthttp.qll

+  }
+
+  private class ResponseBody extends Http::ResponseBody::Range {
+    DataFlow::MethodCallNode call;


Please give call a better name, which describes what it does. Better yet, replace this field with DataFlow::Node responseWriter, set it to the response writer in the first place and return it in getResponseWriter.

I couldn't implement the better one.

…lobally

owen-mc · 2023-12-10T20:50:59Z

go/ql/lib/semmle/go/frameworks/Fasthttp.qll

+    )
+    or
+    exists(DataFlow::CallNode writerCall |
+      writerCall = any(Function fprintf | fprintf.hasQualifiedName("fmt", "Fprintf")).getACall() and


Suggested change

writerCall = any(Function fprintf | fprintf.hasQualifiedName("fmt", "Fprintf")).getACall() and

writerCall = any(Function fprintf | fprintf.hasQualifiedName("fmt", ["Fprint", "Fprintf", "Fprintln"])).getACall() and

May as well include all 3.

Hi, I've also added some more sinks according to the search that you gave me before, please wait until I submit a new commit.

I've submitted the new sinks just now :)

am0o0 · 2023-12-14T09:01:13Z

Hi @owen-mc, Can I please add more content types for extending the XSS query scope in this PR? I want to increase the quality of this PR as much as possible because this PR is related to XSS too.
according to this research I can extend this regex.

go/ql/test/library-tests/semmle/go/frameworks/Fasthttp/fasthttp.go

owen-mc · 2023-12-14T11:45:26Z

I've asked the security lab if those changes are better done in this PR or separately. Apart from those, this PR is pretty much ready to merge. I just asked one question.

owen-mc · 2023-12-14T12:00:58Z

If the things you want to add are related to fasthttp then it is best to include them in this PR. If the change to that regex is needed for modeling fasthttp then do it in this PR. If it's unrelated then you'd need to make a separate submission.

go/ql/test/library-tests/semmle/go/frameworks/Fasthttp/fasthttp.go

am0o0 · 2023-12-14T19:23:55Z

If the things you want to add are related to fasthttp then it is best to include them in this PR. If the change to that regex is needed for modeling fasthttp then do it in this PR. If it's unrelated then you'd need to make a separate submission.

I think it is related because I want to add two tests for the XSS sink with different content types.

am0o0 · 2023-12-23T15:19:14Z

@owen-mc Is there anything left in this PR that I can help with?

owen-mc · 2024-01-02T16:02:10Z

You said you wanted to "add more content types for extending the XSS query scope in this PR". Have you done that?

am0o0 · 2024-01-02T16:24:22Z

Hi, I thought I should wait for the security lab response first.

owen-mc · 2024-01-02T21:09:10Z

I should have been clearer. This comment is a summary of their response to me.

owen-mc · 2024-01-04T11:50:25Z

To answer your question: this PR can be merged as is, or you can add a bit more to it first if you want.

am0o0 · 2024-01-14T19:40:48Z

@owen-mc I think we can merge this PR as is because I'm going to add the additional content-types in another separate PR and I don't want to keep this PR open longer than this.

owen-mc

Great work!

am0o0 · 2024-01-14T20:22:12Z

@owen-mc Thank you so much for your support during this PR. I made many mistakes, but you handled it patiently and professionally. The good news is that I learned a lot and will add more frameworks to Codeql Golang this year (with fewer mistakes).

am0o0 added 3 commits September 2, 2023 22:27

V1

a8a9edc

add tests

6af8252

remove tmp test

9b8f43b

github-actions bot added the Go label Sep 2, 2023

am0o0 added 2 commits September 2, 2023 22:47

fix go.mod

8f11543

add proper test cases

b5a257c

am0o0 mentioned this pull request Sep 3, 2023

[Go]: fasthttp model for XSS, SSRF, open redirect github/securitylab#786

Closed

2 tasks

fix some mistakes:(

0d3b944

ghsecuritylab marked this pull request as ready for review September 15, 2023 10:02

ghsecuritylab requested a review from a team as a code owner September 15, 2023 10:02

smowton requested review from mbg and owen-mc September 15, 2023 11:54

github-advanced-security bot found potential problems Sep 18, 2023

View reviewed changes

fix library-tests

09aee30

add change note

6f5aa58

github-actions bot added the documentation label Sep 18, 2023

github-advanced-security bot found potential problems Sep 18, 2023

View reviewed changes

go/ql/lib/semmle/go/frameworks/Fasthttp.qll Fixed Show fixed Hide fixed

am0o0 added 2 commits September 19, 2023 03:07

Merge branch 'main' into amammad-go-fastHttp

c195a9f

fix tests

a6996c5

fix qhelps

3585459

mbg reviewed Sep 19, 2023

View reviewed changes

am0o0 added 4 commits September 19, 2023 22:24

fix change notes

69c2d9e

fix qlDOC one missed mistake

8758172

fix stub

c1398f2

upgrade tests

c3213e4

github-advanced-security bot found potential problems Dec 6, 2023

View reviewed changes

owen-mc reviewed Dec 6, 2023

View reviewed changes

go/ql/lib/semmle/go/frameworks/Fasthttp.qll Outdated Show resolved Hide resolved

fix an issue in ResponseBody, change isHTMLEscape to isHtmlEscape

dbf01a9

github-advanced-security bot found potential problems Dec 7, 2023

View reviewed changes

fix ResponseBody Class issues

a3fbc3c

owen-mc reviewed Dec 8, 2023

View reviewed changes

use SimpleGlobal with source and sink to find BodyWriter successors g…

b6aaff2

…lobally

owen-mc reviewed Dec 10, 2023

View reviewed changes

am0o0 added 2 commits December 10, 2023 22:06

added more sinks related to io.Writer of BodyWriter

cc54164

remove a duplicate test

bfa0fb6

owen-mc reviewed Dec 14, 2023

View reviewed changes

go/ql/test/library-tests/semmle/go/frameworks/Fasthttp/fasthttp.go Outdated Show resolved Hide resolved

added *ReadBody* Methods as UntrustedFlowSource

d84333d

owen-mc reviewed Dec 14, 2023

View reviewed changes

go/ql/test/library-tests/semmle/go/frameworks/Fasthttp/fasthttp.go Outdated Show resolved Hide resolved

remove a duplicate test

4d9aad9

Bambi66669 mentioned this pull request Dec 24, 2023

If I want to support querying a new language X, what kind of adaptations and modifications do I need to make in which modules or subfolders? Are there any development manuals and documentation? #15201

Closed

owen-mc approved these changes Jan 14, 2024

View reviewed changes

owen-mc merged commit 057ee85 into github:main Jan 14, 2024
14 checks passed

owen-mc mentioned this pull request Jan 15, 2024

Go: add fasthttp to frameworks for coverage #15324

Merged

am0o0 deleted the amammad-go-fastHttp branch September 14, 2024 11:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Go: fasthttp #14123

Go: fasthttp #14123

am0o0 commented Sep 2, 2023 •

edited

Loading

github-advanced-security bot left a comment

mbg commented Sep 18, 2023

am0o0 commented Sep 18, 2023

am0o0 commented Sep 18, 2023

mbg commented Sep 18, 2023

am0o0 commented Sep 18, 2023

mbg left a comment

owen-mc Dec 7, 2023

am0o0 Dec 7, 2023

owen-mc Dec 7, 2023

am0o0 Dec 7, 2023

owen-mc Dec 7, 2023

owen-mc Dec 8, 2023

owen-mc left a comment

owen-mc Dec 8, 2023

owen-mc Dec 8, 2023

am0o0 Dec 10, 2023

owen-mc Dec 10, 2023

am0o0 Dec 10, 2023

am0o0 Dec 10, 2023

am0o0 commented Dec 14, 2023

owen-mc commented Dec 14, 2023

owen-mc commented Dec 14, 2023

am0o0 commented Dec 14, 2023

am0o0 commented Dec 23, 2023

owen-mc commented Jan 2, 2024

am0o0 commented Jan 2, 2024

owen-mc commented Jan 2, 2024

owen-mc commented Jan 4, 2024

am0o0 commented Jan 14, 2024

owen-mc left a comment

am0o0 commented Jan 14, 2024

	writerCall = any(Function fprintf \| fprintf.hasQualifiedName("fmt", "Fprintf")).getACall() and
	writerCall = any(Function fprintf \| fprintf.hasQualifiedName("fmt", ["Fprint", "Fprintf", "Fprintln"])).getACall() and

Go: fasthttp #14123

Go: fasthttp #14123

Conversation

am0o0 commented Sep 2, 2023 • edited Loading

github-advanced-security bot left a comment

Choose a reason for hiding this comment

mbg commented Sep 18, 2023

am0o0 commented Sep 18, 2023

am0o0 commented Sep 18, 2023

mbg commented Sep 18, 2023

am0o0 commented Sep 18, 2023

mbg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

owen-mc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

am0o0 commented Dec 14, 2023

owen-mc commented Dec 14, 2023

owen-mc commented Dec 14, 2023

am0o0 commented Dec 14, 2023

am0o0 commented Dec 23, 2023

owen-mc commented Jan 2, 2024

am0o0 commented Jan 2, 2024

owen-mc commented Jan 2, 2024

owen-mc commented Jan 4, 2024

am0o0 commented Jan 14, 2024

owen-mc left a comment

Choose a reason for hiding this comment

am0o0 commented Jan 14, 2024

am0o0 commented Sep 2, 2023 •

edited

Loading