X Tutup
The Wayback Machine - https://web.archive.org/web/20240213201118/https://github.com/github/codeql/pull/15585
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.

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: Promote go/missing-jwt-signature-check from experimental #15585

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

atorralba
Copy link
Contributor

Promotes the query go/missing-jwt-signature-check (previously go/parse-jwt-without-verification) from experimental. Where possible, plain CodeQL models have been migrated to models-as-data.

Copy link
Contributor

github-actions bot commented Feb 12, 2024

QHelp previews:

go/ql/src/Security/CWE-347/MissingJwtSignatureCheck.qhelp

Missing JWT signature check

Applications decoding a JSON Web Token (JWT) may be vulnerable when the signature is not correctly verified in the process.

Recommendation

Always verify the signature by using the appropriate methods depending on the JWT library, or use a library that verifies it by default.

Example

The following example shows a case where a JWT is parsed without verifying the signature.

package main

import (
	"fmt"
	"log"

	"github.com/golang-jwt/jwt/v5"
)

type User struct{}

func decodeJwt(token string) {
	// BAD: JWT is only decoded without signature verification
	fmt.Println("only decoding JWT")
	DecodedToken, _, err := jwt.NewParser().ParseUnverified(token, &User{})
	if claims, ok := DecodedToken.Claims.(*User); ok {
		fmt.Printf("DecodedToken:%v\n", claims)
	} else {
		log.Fatal("error", err)
	}
}

In the example below, the appropriate function for parsing a JWT and verifying its signature is used.

package main

import (
	"fmt"
	"log"

	"github.com/golang-jwt/jwt/v5"
)

type User struct{}

func parseJwt(token string, jwtKey []byte) {
	// GOOD: JWT is parsed with signature verification using jwtKey
	DecodedToken, err := jwt.ParseWithClaims(token, &User{}, func(token *jwt.Token) (interface{}, error) {
		return jwtKey, nil
	})
	if claims, ok := DecodedToken.Claims.(*User); ok && DecodedToken.Valid && !err {
		fmt.Printf("DecodedToken:%v\n", claims)
	} else {
		log.Fatal(err)
	}
}

References

  • Common Weakness Enumeration: CWE-347.

Copy link
Contributor

⚠️ The head of this PR and the base branch were compared for differences in the framework coverage reports. The generated reports are available in the artifacts of this workflow run. The differences will be picked up by the nightly job after the PR gets merged.

Click to show differences in coverage

go

Generated file changes for go

  • Changes to framework-coverage-go.rst:
+    Others,"``github.com/dgrijalva/jwt-go/$ANYVERSION``, ``github.com/go-jose/go-jose/$ANYVERSION/jwt``, ``github.com/golang-jwt/jwt/$ANYVERSION``, ``gopkg.in/square/go-jose/$ANYVERSION/jwt``",,28,4
-    Totals,,8,842,
+    Totals,,8,870,4
  • Changes to framework-coverage-go.csv:
- package,source,summary,source:remote,summary:taint,summary:value
+ package,sink,source,summary,sink:jwt,source:remote,summary:taint,summary:value
- ,,2,,,2
+ ,,,2,,,,2
- archive/tar,,5,,5,
+ archive/tar,,,5,,,5,
- archive/zip,,6,,6,
+ archive/zip,,,6,,,6,
- bufio,,17,,17,
+ bufio,,,17,,,17,
- bytes,,43,,43,
+ bytes,,,43,,,43,
- compress/bzip2,,1,,1,
+ compress/bzip2,,,1,,,1,
- compress/flate,,4,,4,
+ compress/flate,,,4,,,4,
- compress/gzip,,3,,3,
+ compress/gzip,,,3,,,3,
- compress/lzw,,1,,1,
+ compress/lzw,,,1,,,1,
- compress/zlib,,4,,4,
+ compress/zlib,,,4,,,4,
- container/heap,,5,,5,
+ container/heap,,,5,,,5,
- container/list,,20,,20,
+ container/list,,,20,,,20,
- container/ring,,5,,5,
+ container/ring,,,5,,,5,
- context,,5,,5,
+ context,,,5,,,5,
- crypto,,1,,1,
+ crypto,,,1,,,1,
- crypto/cipher,,3,,3,
+ crypto/cipher,,,3,,,3,
- crypto/rsa,,2,,2,
+ crypto/rsa,,,2,,,2,
- crypto/tls,,3,,3,
+ crypto/tls,,,3,,,3,
- crypto/x509,,1,,1,
+ crypto/x509,,,1,,,1,
- database/sql,,7,,7,
+ database/sql,,,7,,,7,
- database/sql/driver,,4,,4,
+ database/sql/driver,,,4,,,4,
- encoding,,4,,4,
+ encoding,,,4,,,4,
- encoding/ascii85,,2,,2,
+ encoding/ascii85,,,2,,,2,
- encoding/asn1,,8,,8,
+ encoding/asn1,,,8,,,8,
- encoding/base32,,3,,3,
+ encoding/base32,,,3,,,3,
- encoding/base64,,3,,3,
+ encoding/base64,,,3,,,3,
- encoding/binary,,2,,2,
+ encoding/binary,,,2,,,2,
- encoding/csv,,5,,5,
+ encoding/csv,,,5,,,5,
- encoding/gob,,7,,7,
+ encoding/gob,,,7,,,7,
- encoding/hex,,3,,3,
+ encoding/hex,,,3,,,3,
- encoding/json,,14,,14,
+ encoding/json,,,14,,,14,
- encoding/pem,,3,,3,
+ encoding/pem,,,3,,,3,
- encoding/xml,,23,,23,
+ encoding/xml,,,23,,,23,
- errors,,3,,3,
+ errors,,,3,,,3,
- expvar,,6,,6,
+ expvar,,,6,,,6,
- fmt,,16,,16,
+ fmt,,,16,,,16,
- github.com/astaxie/beego,,7,,7,
+ github.com/astaxie/beego,,,7,,,7,
- github.com/astaxie/beego/context,,1,,1,
+ github.com/astaxie/beego/context,,,1,,,1,
- github.com/astaxie/beego/utils,,13,,13,
+ github.com/astaxie/beego/utils,,,13,,,13,
- github.com/beego/beego/core/utils,,13,,13,
+ github.com/beego/beego/core/utils,,,13,,,13,
- github.com/beego/beego/server/web,,7,,7,
+ github.com/beego/beego/server/web,,,7,,,7,
- github.com/beego/beego/server/web/context,,1,,1,
+ github.com/beego/beego/server/web/context,,,1,,,1,
- github.com/couchbase/gocb,,18,,18,
+ github.com/couchbase/gocb,,,18,,,18,
- github.com/couchbaselabs/gocb,,18,,18,
+ github.com/couchbaselabs/gocb,,,18,,,18,
+ github.com/dgrijalva/jwt-go/$ANYVERSION,1,,9,1,,9,
- github.com/elazarl/goproxy,,2,,2,
+ github.com/elazarl/goproxy,,,2,,,2,
- github.com/evanphx/json-patch,,12,,12,
+ github.com/evanphx/json-patch,,,12,,,12,
- github.com/gin-gonic/gin,,2,,2,
+ github.com/gin-gonic/gin,,,2,,,2,
+ github.com/go-jose/go-jose/$ANYVERSION/jwt,1,,4,1,,4,
- github.com/go-pg/pg/$ANYVERSION/orm,,6,,6,
+ github.com/go-pg/pg/$ANYVERSION/orm,,,6,,,6,
+ github.com/golang-jwt/jwt/$ANYVERSION,1,,11,1,,11,
- github.com/golang/protobuf/$ANYVERSION/proto,,4,,4,
+ github.com/golang/protobuf/$ANYVERSION/proto,,,4,,,4,
- github.com/json-iterator/go,,4,,4,
+ github.com/json-iterator/go,,,4,,,4,
- github.com/labstack/echo,,2,,2,
+ github.com/labstack/echo,,,2,,,2,
- github.com/revel/revel,,10,,10,
+ github.com/revel/revel,,,10,,,10,
- github.com/robfig/revel,,10,,10,
+ github.com/robfig/revel,,,10,,,10,
- github.com/sendgrid/sendgrid-go/$ANYVERSION/helpers/mail,,1,,1,
+ github.com/sendgrid/sendgrid-go/$ANYVERSION/helpers/mail,,,1,,,1,
- github.com/valyala/fasthttp,,5,,5,
+ github.com/valyala/fasthttp,,,5,,,5,
- go.uber.org/zap,,11,,11,
+ go.uber.org/zap,,,11,,,11,
- golang.org/x/net/$ANYVERSION/html,,16,,16,
+ golang.org/x/net/$ANYVERSION/html,,,16,,,16,
- golang.org/x/net/context,,5,,5,
+ golang.org/x/net/context,,,5,,,5,
- google.golang.org/protobuf/$ANYVERSION/internal/encoding/text,,1,,1,
+ google.golang.org/protobuf/$ANYVERSION/internal/encoding/text,,,1,,,1,
- google.golang.org/protobuf/$ANYVERSION/internal/impl,,2,,2,
+ google.golang.org/protobuf/$ANYVERSION/internal/impl,,,2,,,2,
- google.golang.org/protobuf/$ANYVERSION/proto,,8,,8,
+ google.golang.org/protobuf/$ANYVERSION/proto,,,8,,,8,
- google.golang.org/protobuf/$ANYVERSION/reflect/protoreflect,,1,,1,
+ google.golang.org/protobuf/$ANYVERSION/reflect/protoreflect,,,1,,,1,
- gopkg.in/couchbase/gocb,,18,,18,
+ gopkg.in/couchbase/gocb,,,18,,,18,
- gopkg.in/macaron,,1,,1,
+ gopkg.in/macaron,,,1,,,1,
+ gopkg.in/square/go-jose/$ANYVERSION/jwt,1,,4,1,,4,
- gopkg.in/yaml,,9,,9,
+ gopkg.in/yaml,,,9,,,9,
- html,,2,,2,
+ html,,,2,,,2,
- html/template,,6,,6,
+ html/template,,,6,,,6,
- io,,19,,19,
+ io,,,19,,,19,
- io/fs,,12,,12,
+ io/fs,,,12,,,12,
- io/ioutil,,2,,2,
+ io/ioutil,,,2,,,2,
- k8s.io/api/core,,10,,10,
+ k8s.io/api/core,,,10,,,10,
- k8s.io/apimachinery/$ANYVERSION/pkg/runtime,,47,,47,
+ k8s.io/apimachinery/$ANYVERSION/pkg/runtime,,,47,,,47,
- log,,3,,3,
+ log,,,3,,,3,
- mime,,5,,5,
+ mime,,,5,,,5,
- mime/multipart,,8,,8,
+ mime/multipart,,,8,,,8,
- mime/quotedprintable,,1,,1,
+ mime/quotedprintable,,,1,,,1,
- net,,20,,20,
+ net,,,20,,,20,
- net/http,8,22,8,22,
+ net/http,,8,22,,8,22,
- net/http/httputil,,10,,10,
+ net/http/httputil,,,10,,,10,
- net/mail,,6,,6,
+ net/mail,,,6,,,6,
- net/textproto,,19,,19,
+ net/textproto,,,19,,,19,
- net/url,,23,,23,
+ net/url,,,23,,,23,
- os,,4,,4,
+ os,,,4,,,4,
- path,,5,,5,
+ path,,,5,,,5,
- path/filepath,,13,,13,
+ path/filepath,,,13,,,13,
- reflect,,37,,37,
+ reflect,,,37,,,37,
- regexp,,20,,20,
+ regexp,,,20,,,20,
- sort,,1,,1,
+ sort,,,1,,,1,
- strconv,,9,,9,
+ strconv,,,9,,,9,
- strings,,34,,34,
+ strings,,,34,,,34,
- sync,,10,,10,
+ sync,,,10,,,10,
- sync/atomic,,24,,24,
+ sync/atomic,,,24,,,24,
- syscall,,8,,8,
+ syscall,,,8,,,8,
- text/scanner,,3,,3,
+ text/scanner,,,3,,,3,
- text/tabwriter,,1,,1,
+ text/tabwriter,,,1,,,1,
- text/template,,6,,6,
+ text/template,,,6,,,6,

Comment on lines +17 to +21
from MissingJwtSignatureCheck::Flow::PathNode source, MissingJwtSignatureCheck::Flow::PathNode sink
where MissingJwtSignatureCheck::Flow::flowPath(source, sink)
select sink.getNode(), source, sink,
"This JWT is parsed without verification and received from $@.", source.getNode(),
"this user-controlled source"

Check warning

Code scanning / CodeQL

Consistent alert message Warning

The go/missing-jwt-signature-check query does not have the same alert message as java.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can dismiss this. The alert in Java is:

"This parser sets a $@, but the signature is not verified.",
  source.getNode(), "JWT signing key"

But the parsers in Go, particularly the unsafe ones, don't necessarily set signing keys.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant
X Tutup