fix: CSRF vulnerability #44
Conversation
brianchennn
force-pushed
the
main
branch
2 times, most recently
from
cb32912 to
565c233
Compare
|
Hi @brianchennn |
backend/WebUI/api_webui.go
Outdated
| mu.Lock() | ||
|
|
||
| if jwtKey == "" { | ||
| rand.Seed(time.Now().UnixNano()) |
There was a problem hiding this comment.
You are using math/rand here, which is not cryptographically secure (especially when seeded with the current time). Since this is supposed to be a secret, it should not be guessable. I recommend to use crypto/rand here.
backend/WebUI/api_webui.go
Outdated
| @@ -530,7 +551,7 @@ func GetTenants(c *gin.Context) { | |||
| setCorsHeader(c) | |||
|
|
|||
| if !CheckAuth(c) { | |||
| c.JSON(http.StatusNotFound, bson.M{}) | |||
| c.JSON(http.StatusNotFound, gin.H{"cause": "Illegal Token"}) | |||
There was a problem hiding this comment.
I think http.StatusUnauthorized would be more appropriate here.
There was a problem hiding this comment.
Or http.StatusForbidden, but that depends on why exactly CheckAuth failed. If it failed because the token is expired or invalid, it should be http.StatusUnauthorized. If it failed because the token does not include the required claims, it should be http.StatusForbidden.
backend/WebUI/api_webui.go
Outdated
| randomBytes := make([]byte, 128) | ||
| _, err := rand.Read(randomBytes) | ||
| if err != nil { | ||
| logger.ProcLog.Warnln("Generate JWT Private Key error.") |
There was a problem hiding this comment.
Does this mean that if creating a random key fails, the signature is done with an empty string as key?
There was a problem hiding this comment.
Yes, but I think that it would not happened. This err checking scope is added to pass the golangci-lint.
backend/WebUI/api_webui.go
Outdated
| @@ -47,6 +53,7 @@ func init() { | |||
| TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, | |||
| }, | |||
| } | |||
| mu = new(sync.Mutex) | |||
There was a problem hiding this comment.
generate jwtKey here and no need mu
| @@ -401,7 +411,7 @@ func JWT(email, userId, tenantId string) string { | |||
| claims["email"] = email | |||
| claims["tenantId"] = tenantId | |||
|
|
|||
| tokenString, err := token.SignedString([]byte(os.Getenv("SIGNINGKEY"))) | |||
| tokenString, err := token.SignedString([]byte(jwtKey)) | |||
There was a problem hiding this comment.
if jwtKey == "" {
return ""
}
| "username:", login.Username, | ||
| ", userid:", userId, | ||
| ", tenantid:", tenantId, | ||
| "}") | ||
|
|
||
| token := JWT(login.Username, userId, tenantId) |
There was a problem hiding this comment.
check if token is "", and return error rsp
| @@ -401,7 +411,7 @@ func JWT(email, userId, tenantId string) string { | |||
| claims["email"] = email | |||
| claims["tenantId"] = tenantId | |||
|
|
|||
| tokenString, err := token.SignedString([]byte(os.Getenv("SIGNINGKEY"))) | |||
| tokenString, err := token.SignedString([]byte(jwtKey)) | |||
| if err != nil { | |||
| logger.ProcLog.Errorf("JWT err: %+v", err) | |||
brianchennn
force-pushed
the
main
branch
4 times, most recently
from
0adfc51 to
47120d1
Compare
backend/WebUI/api_webui.go
Outdated
| randomBytes := make([]byte, 256) | ||
| _, err := rand.Read(randomBytes) | ||
| if err != nil { | ||
| logger.ProcLog.Errorln("Generate JWT Private Key error.") |
There was a problem hiding this comment.
return errors.Wrap(err, "Init JWT key error")
| @@ -80,6 +80,10 @@ func (a *WebuiApp) Start(tlsKeyLogPath string) { | |||
| logger.InitLog.Infoln("Server started") | |||
|
|
|||
| router := WebUI.NewRouter() | |||
| WebUI.SetAdmin() | |||
| if err := WebUI.InitJwtKey(); err != nil { | |||
| return | |||
There was a problem hiding this comment.
logger.InitLog.Errorln(err)
return
This PR resolve the CSRF vulnerability by JWT (Json Web Token).
Now, if clients want to request the RESTful API, they could not use "admin" as token.
The only way to get the JWT token is to login through Webconsole's Login page.