Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Go: Add Rs Cors Support #14873

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
+285 −16
Open

Go: Add Rs Cors Support #14873

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3b78477
Basics
Kwstubbs Nov 6, 2023
28288e0
basic2
Kwstubbs Nov 6, 2023
9958ad9
thesame
Kwstubbs Nov 6, 2023
d7e2fbc
Finish
Kwstubbs Nov 21, 2023
8277c60
depstubber
Kwstubbs Nov 21, 2023
4b95ea0
Merge branch 'main' into go-rs-cors
Kwstubbs Dec 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions go/ql/lib/change-notes/2023-10-31-add-rs-cors-framework.md
View file
Open in desktop
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
Kwstubbs marked this conversation as resolved.
Show resolved Hide resolved
2 changes: 2 additions & 0 deletions go/ql/lib/go.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ import semmle.go.frameworks.Afero
import semmle.go.frameworks.AwsLambda
import semmle.go.frameworks.Beego
import semmle.go.frameworks.BeegoOrm
import semmle.go.frameworks.Chi
import semmle.go.frameworks.RsCors
Kwstubbs marked this conversation as resolved.
Show resolved Hide resolved
import semmle.go.frameworks.Couchbase
import semmle.go.frameworks.Echo
import semmle.go.frameworks.ElazarlGoproxy
Expand Down
18 changes: 9 additions & 9 deletions go/ql/lib/semmle/go/frameworks/GinCors.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ module GinCors {
/**
* A write to the value of Access-Control-Allow-Credentials header
*/
class AllowCredentialsWrite extends DataFlow::ExprNode {
class AllowCredentialsWrite extends UniversalAllowCredentialsWrite {
DataFlow::Node base;

AllowCredentialsWrite() {
Expand All @@ -35,12 +35,12 @@ module GinCors {
/**
* Get config struct holding header values
*/
DataFlow::Node getBase() { result = base }
override DataFlow::Node getBase() { result = base }

/**
* Get config variable holding header values
*/
GinConfig getConfig() {
override GinConfig getConfig() {
exists(GinConfig gc |
(
gc.getV().getBaseVariable().getDefinition().(SsaExplicitDefinition).getRhs() =
Expand All @@ -55,7 +55,7 @@ module GinCors {
/**
* A write to the value of Access-Control-Allow-Origins header
*/
class AllowOriginsWrite extends DataFlow::ExprNode {
class AllowOriginsWrite extends UniversalOriginWrite {
DataFlow::Node base;

AllowOriginsWrite() {
Expand All @@ -69,12 +69,12 @@ module GinCors {
/**
* Get config struct holding header values
*/
DataFlow::Node getBase() { result = base }
override DataFlow::Node getBase() { result = base }

/**
* Get config variable holding header values
*/
GinConfig getConfig() {
override GinConfig getConfig() {
exists(GinConfig gc |
(
gc.getV().getBaseVariable().getDefinition().(SsaExplicitDefinition).getRhs() =
Expand All @@ -89,7 +89,7 @@ module GinCors {
/**
* A write to the value of Access-Control-Allow-Origins of value "*", overriding AllowOrigins
*/
class AllowAllOriginsWrite extends DataFlow::ExprNode {
class AllowAllOriginsWrite extends UniversalAllowAllOriginsWrite {
DataFlow::Node base;

AllowAllOriginsWrite() {
Expand All @@ -103,12 +103,12 @@ module GinCors {
/**
* Get config struct holding header values
*/
DataFlow::Node getBase() { result = base }
override DataFlow::Node getBase() { result = base }

/**
* Get config variable holding header values
*/
GinConfig getConfig() {
override GinConfig getConfig() {
exists(GinConfig gc |
(
gc.getV().getBaseVariable().getDefinition().(SsaExplicitDefinition).getRhs() =
Expand Down
166 changes: 166 additions & 0 deletions go/ql/lib/semmle/go/frameworks/RsCors.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,166 @@
/**
* Provides classes for modeling the `github.com/rs/cors` package.
*/

import go

/**
* Provides abstract class for modeling the Go CORS handler model origin write.
*/
Kwstubbs marked this conversation as resolved.
Show resolved Hide resolved
abstract class UniversalOriginWrite extends DataFlow::ExprNode {
abstract DataFlow::Node getBase();

abstract Variable getConfig();
}

/**
* Provides abstract class for modeling the Go CORS handler model allow all origins write.
*/
Comment on lines +16 to +18

Check warning

Code scanning / CodeQL

Class QLDoc style. Warning

The QLDoc for a class should start with 'A', 'An', or 'The'.
abstract class UniversalAllowAllOriginsWrite extends DataFlow::ExprNode {
abstract DataFlow::Node getBase();

abstract Variable getConfig();
}

/**
* Provides abstract class for modeling the Go CORS handler model allow credentials write.
*/
Comment on lines +25 to +27

Check warning

Code scanning / CodeQL

Class QLDoc style. Warning

The QLDoc for a class should start with 'A', 'An', or 'The'.
abstract class UniversalAllowCredentialsWrite extends DataFlow::ExprNode {
abstract DataFlow::Node getBase();

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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't actually correct: if v is a.b.c then this is the Variable a and the thing that has type Options is a.b.c. I didn't notice this when you originally modeled GinCors. Can I ask why you are doing it this way? Is there a particular case where just using Variable or just using SsaWithFields doesn't work?

Copy link
Contributor

@owen-mc owen-mc Nov 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at it more carefully, I see that, because of the way that they are modeled, RsOptions and GinConfig are confined to be local variables defined in functions. Is that what you intended? Might you ever want to reason about one that is defined at package scope?

Copy link
Contributor Author

@Kwstubbs Kwstubbs Mar 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@owen-mc

This isn't actually correct: if v is a.b.c then this is the Variable a and the thing that has type Options is a.b.c. I didn't notice this when you originally modeled GinCors. Can I ask why you are doing it this way? Is there a particular case where just using Variable or just using SsaWithFields doesn't work?

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 go/ql/src/experimental/CWE-1004/AuthCookie.qll, but I'm not sure it's necessarily one that I'd recommend.

I'll ask the rest of the codeql-go team if they can think of a better way of doing it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@owen-mc let me know if you have any updates on this :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, sorry I didn't get back to you. The best way to include global variables is to copy this code which defines SsaWithFieldsand replace the root case with a read from a global variable. I guess GlobalWithFields would be the obvious name.

*/
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 }
}
}
14 changes: 7 additions & 7 deletions go/ql/src/experimental/CWE-942/CorsMisconfiguration.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ module UntrustedToAllowOriginHeaderConfig implements DataFlow::ConfigSig {
module UntrustedToAllowOriginConfigConfig implements DataFlow::ConfigSig {
predicate isSource(DataFlow::Node source) { source instanceof ActiveThreatModelSource }

additional predicate isSinkWrite(DataFlow::Node sink, GinCors::AllowOriginsWrite w) { sink = w }
additional predicate isSinkWrite(DataFlow::Node sink, UniversalOriginWrite w) { sink = w }

predicate isSink(DataFlow::Node sink) { isSinkWrite(sink, _) }
}
Expand Down Expand Up @@ -102,17 +102,17 @@ predicate allowCredentialsIsSetToTrue(DataFlow::ExprNode allowOriginHW) {
allowCredentialsHW.getResponseWriter()
)
or
exists(GinCors::AllowCredentialsWrite allowCredentialsGin |
exists(UniversalAllowCredentialsWrite allowCredentialsGin |
allowCredentialsGin.getExpr().getBoolValue() = true
|
allowCredentialsGin.getConfig() = allowOriginHW.(GinCors::AllowOriginsWrite).getConfig() and
not exists(GinCors::AllowAllOriginsWrite allowAllOrigins |
allowCredentialsGin.getConfig() = allowOriginHW.(UniversalOriginWrite).getConfig() and
not exists(UniversalAllowAllOriginsWrite allowAllOrigins |
allowAllOrigins.getExpr().getBoolValue() = true and
allowCredentialsGin.getConfig() = allowAllOrigins.getConfig()
)
or
allowCredentialsGin.getBase() = allowOriginHW.(GinCors::AllowOriginsWrite).getBase() and
not exists(GinCors::AllowAllOriginsWrite allowAllOrigins |
allowCredentialsGin.getBase() = allowOriginHW.(UniversalOriginWrite).getBase() and
not exists(UniversalAllowAllOriginsWrite allowAllOrigins |
allowAllOrigins.getExpr().getBoolValue() = true and
allowCredentialsGin.getBase() = allowAllOrigins.getBase()
)
Expand Down Expand Up @@ -150,7 +150,7 @@ predicate allowOriginIsNull(DataFlow::ExprNode allowOriginHW, string message) {
+ " is set to `true`"
or
allowOriginHW
.(GinCors::AllowOriginsWrite)
.(UniversalOriginWrite)
.asExpr()
.(SliceLit)
.getAnElement()
Expand Down
1 change: 1 addition & 0 deletions go/ql/test/experimental/CWE-942/CorsMisconfiguration.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,4 @@
| CorsMisconfiguration.go:53:4:53:44 | call to Set | access-control-allow-origin header is set to a user-defined value, and access-control-allow-credentials is set to `true` |
| CorsMisconfiguration.go:60:4:60:56 | call to Set | access-control-allow-origin header is set to a user-defined value, and access-control-allow-credentials is set to `true` |
| CorsMisconfiguration.go:67:5:67:57 | call to Set | access-control-allow-origin header is set to a user-defined value, and access-control-allow-credentials is set to `true` |
| RsCors.go:11:21:11:59 | slice literal | access-control-allow-origin header is set to `null`, and access-control-allow-credentials is set to `true` |
39 changes: 39 additions & 0 deletions go/ql/test/experimental/CWE-942/RsCors.go
View file
Open in desktop
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))
}
1 change: 1 addition & 0 deletions go/ql/test/experimental/CWE-942/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ go 1.21
require (
github.com/gin-contrib/cors v1.4.0
github.com/gin-gonic/gin v1.9.1
github.com/rs/cors v1.10.1
)

require (
Expand Down
Loading
Loading