diff --git a/go/ql/src/experimental/CWE-285/PamAuthBad.go b/go/ql/src/experimental/CWE-285/PamAuthBad.go new file mode 100644 index 000000000000..2e6b188a0bfe --- /dev/null +++ b/go/ql/src/experimental/CWE-285/PamAuthBad.go @@ -0,0 +1,16 @@ +func bad() error { + t, err := pam.StartFunc("", "username", func(s pam.Style, msg string) (string, error) { + switch s { + case pam.PromptEchoOff: + return string(pass), nil + } + return "", fmt.Errorf("unsupported message style") + }) + if err != nil { + return nil, err + } + + if err := t.Authenticate(0); err != nil { + return nil, fmt.Errorf("Authenticate: %w", err) + } +} diff --git a/go/ql/src/experimental/CWE-285/PamAuthBypass.qhelp b/go/ql/src/experimental/CWE-285/PamAuthBypass.qhelp new file mode 100644 index 000000000000..7bbe34c407a8 --- /dev/null +++ b/go/ql/src/experimental/CWE-285/PamAuthBypass.qhelp @@ -0,0 +1,52 @@ + + + +

+ Using only a call to + pam.Authenticate + to check the validity of a login can lead to authorization bypass vulnerabilities. +

+

+ A pam.Authenticate call + only verifies the credentials of a user. It does not check if a user has an + appropriate authorization to actually login. This means a user with an expired + login or a password can still access the system. +

+ +
+ + +

+ A call to + pam.Authenticate + should be followed by a call to + pam.AcctMgmt + to check if a user is allowed to login. +

+
+ + +

+ In the following example, the code only checks the credentials of a user. Hence, + in this case, a user with expired credentials can still login. This can be + verified by creating a new user account, expiring it with + chage -E0 `username` + and then trying to log in. +

+ + +

+ This can be avoided by calling + pam.AcctMgmt + call to verify access as has been done in the snippet shown below. +

+ +
+ + +
  • + Man-Page: + pam_acct_mgmt +
  • +
    +
    \ No newline at end of file diff --git a/go/ql/src/experimental/CWE-285/PamAuthBypass.ql b/go/ql/src/experimental/CWE-285/PamAuthBypass.ql new file mode 100644 index 000000000000..06f2904599ed --- /dev/null +++ b/go/ql/src/experimental/CWE-285/PamAuthBypass.ql @@ -0,0 +1,65 @@ +/** + * @name PAM authorization bypass due to incorrect usage + * @description Not using `pam.AcctMgmt` after `pam.Authenticate` to check the validity of a login can lead to authorization bypass. + * @kind problem + * @problem.severity warning + * @id go/unreachable-statement + * @tags maintainability + * correctness + * external/cwe/cwe-561 + * external/cwe/cwe-285 + * @precision very-high + */ + +import go + +predicate isInTestFile(Expr r) { + r.getFile().getAbsolutePath().matches("%test%") and + not r.getFile().getAbsolutePath().matches("%/ql/test/%") +} + +class PamAuthenticate extends Method { + PamAuthenticate() { + this.hasQualifiedName("github.com/msteinert/pam", "Transaction", "Authenticate") + } +} + +class PamAcctMgmt extends Method { + PamAcctMgmt() { this.hasQualifiedName("github.com/msteinert/pam", "Transaction", "AcctMgmt") } +} + +class PamStartFunc extends Function { + PamStartFunc() { this.hasQualifiedName("github.com/msteinert/pam", ["StartFunc", "Start"]) } +} + +class PamStartToAcctMgmtConfig extends TaintTracking::Configuration { + PamStartToAcctMgmtConfig() { this = "PAM auth bypass (Start to AcctMgmt)" } + + override predicate isSource(DataFlow::Node source) { + exists(PamStartFunc p | p.getACall().getResult(0) = source) + } + + override predicate isSink(DataFlow::Node sink) { + exists(PamAcctMgmt p | p.getACall().getReceiver() = sink) + } +} + +class PamStartToAuthenticateConfig extends TaintTracking::Configuration { + PamStartToAuthenticateConfig() { this = "PAM auth bypass (Start to Authenticate)" } + + override predicate isSource(DataFlow::Node source) { + exists(PamStartFunc p | p.getACall().getResult(0) = source) + } + + override predicate isSink(DataFlow::Node sink) { + exists(PamAuthenticate p | p.getACall().getReceiver() = sink) + } +} + +from + PamStartToAcctMgmtConfig acctMgmtConfig, PamStartToAuthenticateConfig authConfig, + DataFlow::Node source, DataFlow::Node sink +where + not isInTestFile(source.asExpr()) and + (authConfig.hasFlow(source, sink) and not acctMgmtConfig.hasFlow(source, _)) +select source, "This Pam transaction may not be secure." diff --git a/go/ql/src/experimental/CWE-285/PamAuthGood.go b/go/ql/src/experimental/CWE-285/PamAuthGood.go new file mode 100644 index 000000000000..08924b18525c --- /dev/null +++ b/go/ql/src/experimental/CWE-285/PamAuthGood.go @@ -0,0 +1,19 @@ +func good() error { + t, err := pam.StartFunc("", "username", func(s pam.Style, msg string) (string, error) { + switch s { + case pam.PromptEchoOff: + return string(pass), nil + } + return "", fmt.Errorf("unsupported message style") + }) + if err != nil { + return nil, err + } + + if err := t.Authenticate(0); err != nil { + return nil, fmt.Errorf("Authenticate: %w", err) + } + if err := t.AcctMgmt(0); err != nil { + return nil, fmt.Errorf("AcctMgmt: %w", err) + } +} diff --git a/go/ql/test/experimental/CWE-285/PamAuthBypass.expected b/go/ql/test/experimental/CWE-285/PamAuthBypass.expected new file mode 100644 index 000000000000..23441b3361e5 --- /dev/null +++ b/go/ql/test/experimental/CWE-285/PamAuthBypass.expected @@ -0,0 +1 @@ +| main.go:10:2:12:3 | ... := ...[0] | This Pam transaction may not be secure. | \ No newline at end of file diff --git a/go/ql/test/experimental/CWE-285/PamAuthBypass.qlref b/go/ql/test/experimental/CWE-285/PamAuthBypass.qlref new file mode 100644 index 000000000000..8a1d5b259e0b --- /dev/null +++ b/go/ql/test/experimental/CWE-285/PamAuthBypass.qlref @@ -0,0 +1 @@ +experimental/CWE-285/PamAuthBypass.ql \ No newline at end of file diff --git a/go/ql/test/experimental/CWE-285/go.mod b/go/ql/test/experimental/CWE-285/go.mod new file mode 100644 index 000000000000..36e39ab93fb1 --- /dev/null +++ b/go/ql/test/experimental/CWE-285/go.mod @@ -0,0 +1,5 @@ +module main + +go 1.18 + +require github.com/msteinert/pam v1.0.0 \ No newline at end of file diff --git a/go/ql/test/experimental/CWE-285/main.go b/go/ql/test/experimental/CWE-285/main.go new file mode 100644 index 000000000000..b0607a74a410 --- /dev/null +++ b/go/ql/test/experimental/CWE-285/main.go @@ -0,0 +1,28 @@ +package main + +//go:generate depstubber -vendor github.com/msteinert/pam Style,Transaction StartFunc + +import ( + "github.com/msteinert/pam" +) + +func bad() error { + t, _ := pam.StartFunc("", "", func(s pam.Style, msg string) (string, error) { + return "", nil + }) + return t.Authenticate(0) + +} + +func good() error { + t, err := pam.StartFunc("", "", func(s pam.Style, msg string) (string, error) { + return "", nil + }) + err = t.Authenticate(0) + if err != nil { + return err + } + return t.AcctMgmt(0) +} + +func main() {} diff --git a/go/ql/test/experimental/CWE-285/vendor/github.com/msteinert/pam/stub.go b/go/ql/test/experimental/CWE-285/vendor/github.com/msteinert/pam/stub.go new file mode 100644 index 000000000000..8451ad5dcc7c --- /dev/null +++ b/go/ql/test/experimental/CWE-285/vendor/github.com/msteinert/pam/stub.go @@ -0,0 +1,68 @@ +// Code generated by depstubber. DO NOT EDIT. +// This is a simple stub for github.com/msteinert/pam, strictly for use in testing. + +// See the LICENSE file for information about the licensing of the original library. +// Source: github.com/msteinert/pam (exports: Style,Transaction; functions: StartFunc) + +// Package pam is a stub of github.com/msteinert/pam, generated by depstubber. +package pam + +type Flags int + +type Item int + +func StartFunc(_ string, _ string, _ func(Style, string) (string, error)) (*Transaction, error) { + return nil, nil +} + +type Style int + +type Transaction struct{} + +func (_ *Transaction) AcctMgmt(_ Flags) error { + return nil +} + +func (_ *Transaction) Authenticate(_ Flags) error { + return nil +} + +func (_ *Transaction) ChangeAuthTok(_ Flags) error { + return nil +} + +func (_ *Transaction) CloseSession(_ Flags) error { + return nil +} + +func (_ *Transaction) Error() string { + return "" +} + +func (_ *Transaction) GetEnv(_ string) string { + return "" +} + +func (_ *Transaction) GetEnvList() (map[string]string, error) { + return nil, nil +} + +func (_ *Transaction) GetItem(_ Item) (string, error) { + return "", nil +} + +func (_ *Transaction) OpenSession(_ Flags) error { + return nil +} + +func (_ *Transaction) PutEnv(_ string) error { + return nil +} + +func (_ *Transaction) SetCred(_ Flags) error { + return nil +} + +func (_ *Transaction) SetItem(_ Item, _ string) error { + return nil +} diff --git a/go/ql/test/experimental/CWE-285/vendor/modules.txt b/go/ql/test/experimental/CWE-285/vendor/modules.txt new file mode 100644 index 000000000000..5ad327e8bb5b --- /dev/null +++ b/go/ql/test/experimental/CWE-285/vendor/modules.txt @@ -0,0 +1,3 @@ +# github.com/msteinert/pam v1.0.0 +## explicit +github.com/msteinert/pam \ No newline at end of file