Go: Add Rs Cors Support #14873
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| --- | ||
| category: minorAnalysis | ||
| --- | ||
| * Added the [rs cors](https://github.com/rs/cors) library to the CorsMisconfiguration.ql query | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,184 @@ | ||
| /** | ||
| * Provides classes for modeling the `github.com/rs/cors` package. | ||
| */ | ||
|
|
||
| import go | ||
|
|
||
| /** | ||
| * An abstract class for modeling the Go CORS handler model origin write. | ||
| */ | ||
| abstract class UniversalOriginWrite extends DataFlow::ExprNode { | ||
| /** | ||
| * Get config variable holding header values | ||
| */ | ||
| abstract DataFlow::Node getBase(); | ||
|
|
||
| /** | ||
| * Get config variable holding header values | ||
| */ | ||
| abstract Variable getConfig(); | ||
| } | ||
|
|
||
| /** | ||
| * An abstract class for modeling the Go CORS handler model allow all origins write. | ||
| */ | ||
| abstract class UniversalAllowAllOriginsWrite extends DataFlow::ExprNode { | ||
| /** | ||
| * Get config variable holding header values | ||
| */ | ||
| abstract DataFlow::Node getBase(); | ||
|
|
||
| /** | ||
| * Get config variable holding header values | ||
| */ | ||
| abstract Variable getConfig(); | ||
| } | ||
|
|
||
| /** | ||
| * An abstract class for modeling the Go CORS handler model allow credentials write. | ||
| */ | ||
| abstract class UniversalAllowCredentialsWrite extends DataFlow::ExprNode { | ||
| /** | ||
| * Get config struct holding header values | ||
| */ | ||
| abstract DataFlow::Node getBase(); | ||
|
|
||
| /** | ||
| * Get config variable holding header values | ||
| */ | ||
| abstract Variable getConfig(); | ||
| } | ||
|
|
||
| /** | ||
| * Provides classes for modeling the `github.com/rs/cors` package. | ||
| */ | ||
| module RsCors { | ||
| /** Gets the package name `github.com/gin-gonic/gin`. */ | ||
Kwstubbs marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| string packagePath() { result = package("github.com/rs/cors", "") } | ||
|
|
||
| /** | ||
| * A new function create a new rs Handler | ||
Kwstubbs marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| */ | ||
| class New extends Function { | ||
| New() { exists(Function f | f.hasQualifiedName(packagePath(), "New") | this = f) } | ||
Kwstubbs marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| /** | ||
| * A write to the value of Access-Control-Allow-Credentials header | ||
| */ | ||
| class AllowCredentialsWrite extends UniversalAllowCredentialsWrite { | ||
| DataFlow::Node base; | ||
|
|
||
| AllowCredentialsWrite() { | ||
| exists(Field f, Write w | | ||
| f.hasQualifiedName(packagePath(), "Options", "AllowCredentials") and | ||
| w.writesField(base, f, this) and | ||
| this.getType() instanceof BoolType | ||
| ) | ||
| } | ||
|
|
||
| /** | ||
| * Get options struct holding header values | ||
| */ | ||
| override DataFlow::Node getBase() { result = base } | ||
|
|
||
| /** | ||
| * Get options variable holding header values | ||
| */ | ||
| override RsOptions getConfig() { | ||
| exists(RsOptions gc | | ||
| ( | ||
| gc.getV().getBaseVariable().getDefinition().(SsaExplicitDefinition).getRhs() = | ||
| base.asInstruction() or | ||
| gc.getV().getAUse() = base | ||
| ) and | ||
| result = gc | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * A write to the value of Access-Control-Allow-Origins header | ||
| */ | ||
| class AllowOriginsWrite extends UniversalOriginWrite { | ||
| DataFlow::Node base; | ||
|
|
||
| AllowOriginsWrite() { | ||
| exists(Field f, Write w | | ||
| f.hasQualifiedName(packagePath(), "Options", "AllowedOrigins") and | ||
| w.writesField(base, f, this) and | ||
| this.asExpr() instanceof SliceLit | ||
| ) | ||
| } | ||
|
|
||
| /** | ||
| * Get options struct holding header values | ||
| */ | ||
| override DataFlow::Node getBase() { result = base } | ||
|
|
||
| /** | ||
| * Get options variable holding header values | ||
| */ | ||
| override RsOptions getConfig() { | ||
| exists(RsOptions gc | | ||
| ( | ||
| gc.getV().getBaseVariable().getDefinition().(SsaExplicitDefinition).getRhs() = | ||
| base.asInstruction() or | ||
| gc.getV().getAUse() = base | ||
| ) and | ||
| result = gc | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * A write to the value of Access-Control-Allow-Origins of value "*", overriding AllowOrigins | ||
| */ | ||
| class AllowAllOriginsWrite extends UniversalAllowAllOriginsWrite { | ||
| DataFlow::Node base; | ||
|
|
||
| AllowAllOriginsWrite() { | ||
| exists(Field f, Write w | | ||
| f.hasQualifiedName(packagePath(), "Options", "AllowAllOrigins") and | ||
| w.writesField(base, f, this) and | ||
| this.getType() instanceof BoolType | ||
| ) | ||
| } | ||
|
|
||
| /** | ||
| * Get options struct holding header values | ||
| */ | ||
| override DataFlow::Node getBase() { result = base } | ||
|
|
||
| /** | ||
| * Get options variable holding header values | ||
| */ | ||
| override RsOptions getConfig() { | ||
| exists(RsOptions gc | | ||
| ( | ||
| gc.getV().getBaseVariable().getDefinition().(SsaExplicitDefinition).getRhs() = | ||
| base.asInstruction() or | ||
| gc.getV().getAUse() = base | ||
| ) and | ||
| result = gc | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * A variable of type Options that holds the headers to be set. | ||
|
Contributor
There was a problem hiding this comment. This isn't actually correct: if
Contributor
There was a problem hiding this comment. Looking at it more carefully, I see that, because of the way that they are modeled,
Contributor
Author
There was a problem hiding this comment.
I can change this to just use SSAWithFields. I will ensure that the getBaseVariable() is removed. I see the problem with the current model is that since I am using SSAVariable, getSourceVariable's result is by definition a local variable. I would like to support package variables as well. I have tried to remove SSAs all together and just use Node, but cannot find a way for two nodes to know if they actually represent the same variable. If you can think of any way to do this that would be great. var opts cors.Options <----- the package opts variables
func rs_vulnerable() {
opts.AllowedMethods = []string{"POST"}
opts.AllowedOrigins = []string{"null"} <--- how to compare Node corresponding to this opts
opts.AllowCredentials = true <---- to Node corresponding to this opts and see that they both correspond to the package level opts
Contributor
There was a problem hiding this comment. Sorry I wasn't clearer about why the way they are modeled means they have to be local variables. Well done for figuring it out - coming back to this after four months, it took me a few minutes, even with your explanation. I would say that SSAWithFields works great as long as you don't mind missing out writes to global variables. You may decide that in practice, that pattern doesn't come up and you're okay with not spotting it. When dealing with just nodes, I think the normal way to think about them "representing the same variable" is whether there is value/taint flow from one to another. There is an approach to this kind of thing in I'll ask the rest of the codeql-go team if they can think of a better way of doing it.
Contributor
Author
There was a problem hiding this comment. @owen-mc let me know if you have any updates on this :)
Contributor
There was a problem hiding this comment. Yes, sorry I didn't get back to you. The best way to include global variables is to copy this code which defines
Contributor
Author
There was a problem hiding this comment. @owen-mc tbh this seems to be a bit complex for my CodeQL skills, but I have written some initial code in the spirit of what you suggested Let me know if I'm on a the right the path (ignore any of the comments). I need getDefinition but it uses getLocalDefinition but I'm not too familiar with basic blocks in CodeQL to create a version for global. Any advice would be great regarding if this was what you were thinking of and in regards to getLocalDefinition. Cheers |
||
| */ | ||
| class RsOptions extends Variable { | ||
| SsaWithFields v; | ||
|
|
||
| RsOptions() { | ||
| this = v.getBaseVariable().getSourceVariable() and | ||
| exists(Type t | t.hasQualifiedName(packagePath(), "Options") | v.getType() = t) | ||
Kwstubbs marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| /** | ||
| * Get variable declaration of Options | ||
| */ | ||
| SsaWithFields getV() { result = v } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| package main | ||
|
|
||
| import ( | ||
| "net/http" | ||
|
|
||
| "github.com/rs/cors" | ||
| ) | ||
|
|
||
| func rs_vulnerable() { | ||
| c := cors.New(cors.Options{ | ||
| AllowedOrigins: []string{"null", "http://foo.com:8080"}, | ||
| AllowCredentials: true, | ||
| // Enable Debugging for testing, consider disabling in production | ||
| Debug: true, | ||
| }) | ||
|
|
||
| handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| w.Header().Set("Content-Type", "application/json") | ||
| w.Write([]byte("{\"hello\": \"world\"}")) | ||
| }) | ||
|
|
||
| http.ListenAndServe(":8080", c.Handler(handler)) | ||
| } | ||
|
|
||
| func rs_safe() { | ||
| c := cors.New(cors.Options{ | ||
| AllowedOrigins: []string{"http://foo.com:8080"}, | ||
| AllowCredentials: true, | ||
| // Enable Debugging for testing, consider disabling in production | ||
| Debug: true, | ||
| }) | ||
|
|
||
| handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| w.Header().Set("Content-Type", "application/json") | ||
| w.Write([]byte("{\"hello\": \"world\"}")) | ||
| }) | ||
|
|
||
| http.ListenAndServe(":8080", c.Handler(handler)) | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.