From ae2cc378e5ccc0980fcb722ca16078722845e7b5 Mon Sep 17 00:00:00 2001 From: Porcupiney Hairs Date: Tue, 31 May 2022 01:28:55 +0530 Subject: [PATCH 1/5] Golang : Add Query To Detect PAM Authorization Bugs --- go/ql/src/experimental/CWE-285/PamAuthBad.go | 16 +++++ .../experimental/CWE-285/PamAuthBypass.qhelp | 53 ++++++++++++++ .../src/experimental/CWE-285/PamAuthBypass.ql | 69 +++++++++++++++++++ .../CWE-285/PamAuthBypassLocal.ql | 50 ++++++++++++++ go/ql/src/experimental/CWE-285/PamAuthGood.go | 19 +++++ .../CWE-285/PamAuthBypass.expected | 1 + .../experimental/CWE-285/PamAuthBypass.qlref | 1 + go/ql/test/experimental/CWE-285/go.mod | 5 ++ go/ql/test/experimental/CWE-285/main.go | 28 ++++++++ .../vendor/github.com/msteinert/pam/stub.go | 68 ++++++++++++++++++ .../experimental/CWE-285/vendor/modules.txt | 3 + 11 files changed, 313 insertions(+) create mode 100644 go/ql/src/experimental/CWE-285/PamAuthBad.go create mode 100644 go/ql/src/experimental/CWE-285/PamAuthBypass.qhelp create mode 100644 go/ql/src/experimental/CWE-285/PamAuthBypass.ql create mode 100644 go/ql/src/experimental/CWE-285/PamAuthBypassLocal.ql create mode 100644 go/ql/src/experimental/CWE-285/PamAuthGood.go create mode 100644 go/ql/test/experimental/CWE-285/PamAuthBypass.expected create mode 100644 go/ql/test/experimental/CWE-285/PamAuthBypass.qlref create mode 100644 go/ql/test/experimental/CWE-285/go.mod create mode 100644 go/ql/test/experimental/CWE-285/main.go create mode 100644 go/ql/test/experimental/CWE-285/vendor/github.com/msteinert/pam/stub.go create mode 100644 go/ql/test/experimental/CWE-285/vendor/modules.txt 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..921cabae1e42 --- /dev/null +++ b/go/ql/src/experimental/CWE-285/PamAuthBypass.qhelp @@ -0,0 +1,53 @@ + + + +

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

+

+ A + pam.Authenticate + 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..c99bc16faa24 --- /dev/null +++ b/go/ql/src/experimental/CWE-285/PamAuthBypass.ql @@ -0,0 +1,69 @@ +/** + * @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 PamTransaction extends StructType { + PamTransaction() { this.hasQualifiedName("github.com/msteinert/pam", "Transaction") } +} + +class PamStartFunc extends Function { + PamStartFunc() { this.hasQualifiedName("github.com/msteinert/pam", ["StartFunc", "Start"]) } +} + +class PamAuthBypassConfiguration extends TaintTracking::Configuration { + PamAuthBypassConfiguration() { this = "PAM auth bypass" } + + 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 PamAuthBypassConfig extends TaintTracking::Configuration { + PamAuthBypassConfig() { this = "PAM auth bypass2" } + + 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 + PamAuthBypassConfiguration config, PamAuthBypassConfig config2, DataFlow::Node source, + DataFlow::Node sink +where + not isInTestFile(source.asExpr()) and + (config2.hasFlow(source, sink) and not config.hasFlow(source, _)) +select source, "This Pam transaction may not be secure." diff --git a/go/ql/src/experimental/CWE-285/PamAuthBypassLocal.ql b/go/ql/src/experimental/CWE-285/PamAuthBypassLocal.ql new file mode 100644 index 000000000000..d75e47981fe2 --- /dev/null +++ b/go/ql/src/experimental/CWE-285/PamAuthBypassLocal.ql @@ -0,0 +1,50 @@ +/** + * @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 PamTransaction extends StructType { + PamTransaction() { this.hasQualifiedName("github.com/msteinert/pam", "Transaction") } +} + +class PamStartFunc extends Function { + PamStartFunc() { this.hasQualifiedName("github.com/msteinert/pam", ["StartFunc", "Start"]) } +} + +from + DataFlow::Node source, DataFlow::Node sink, PamAuthenticate auth, PamStartFunc start, + PamAcctMgmt actmgt +where + not isInTestFile(source.asExpr()) and + ( + start.getACall().getResult(0) = source and + sink = auth.getACall().getReceiver() and + DataFlow::localFlow(source, sink) and + not DataFlow::localFlow(source, actmgt.getACall().getReceiver()) + ) +select source, "This Pam transaction may not check authorization correctly." 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 From 5c5e978d30adf0f3a3fd81696eab6fe69c348b8a Mon Sep 17 00:00:00 2001 From: Porcupiney Hairs Date: Tue, 31 May 2022 03:53:02 +0530 Subject: [PATCH 2/5] Remove local data flow query --- .../CWE-285/PamAuthBypassLocal.ql | 50 ------------------- 1 file changed, 50 deletions(-) delete mode 100644 go/ql/src/experimental/CWE-285/PamAuthBypassLocal.ql diff --git a/go/ql/src/experimental/CWE-285/PamAuthBypassLocal.ql b/go/ql/src/experimental/CWE-285/PamAuthBypassLocal.ql deleted file mode 100644 index d75e47981fe2..000000000000 --- a/go/ql/src/experimental/CWE-285/PamAuthBypassLocal.ql +++ /dev/null @@ -1,50 +0,0 @@ -/** - * @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 PamTransaction extends StructType { - PamTransaction() { this.hasQualifiedName("github.com/msteinert/pam", "Transaction") } -} - -class PamStartFunc extends Function { - PamStartFunc() { this.hasQualifiedName("github.com/msteinert/pam", ["StartFunc", "Start"]) } -} - -from - DataFlow::Node source, DataFlow::Node sink, PamAuthenticate auth, PamStartFunc start, - PamAcctMgmt actmgt -where - not isInTestFile(source.asExpr()) and - ( - start.getACall().getResult(0) = source and - sink = auth.getACall().getReceiver() and - DataFlow::localFlow(source, sink) and - not DataFlow::localFlow(source, actmgt.getACall().getReceiver()) - ) -select source, "This Pam transaction may not check authorization correctly." From 8b32eaf05c737768189789584187eeca9eb1f504 Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Tue, 31 May 2022 11:05:40 +0100 Subject: [PATCH 3/5] Copyedits --- go/ql/src/experimental/CWE-285/PamAuthBypass.qhelp | 3 +-- go/ql/src/experimental/CWE-285/PamAuthBypass.ql | 12 ++++++------ 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/go/ql/src/experimental/CWE-285/PamAuthBypass.qhelp b/go/ql/src/experimental/CWE-285/PamAuthBypass.qhelp index 921cabae1e42..7bbe34c407a8 100644 --- a/go/ql/src/experimental/CWE-285/PamAuthBypass.qhelp +++ b/go/ql/src/experimental/CWE-285/PamAuthBypass.qhelp @@ -7,8 +7,7 @@ to check the validity of a login can lead to authorization bypass vulnerabilities.

    - A - pam.Authenticate + 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. diff --git a/go/ql/src/experimental/CWE-285/PamAuthBypass.ql b/go/ql/src/experimental/CWE-285/PamAuthBypass.ql index c99bc16faa24..4775141b86fa 100644 --- a/go/ql/src/experimental/CWE-285/PamAuthBypass.ql +++ b/go/ql/src/experimental/CWE-285/PamAuthBypass.ql @@ -36,8 +36,8 @@ class PamStartFunc extends Function { PamStartFunc() { this.hasQualifiedName("github.com/msteinert/pam", ["StartFunc", "Start"]) } } -class PamAuthBypassConfiguration extends TaintTracking::Configuration { - PamAuthBypassConfiguration() { this = "PAM auth bypass" } +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) @@ -48,8 +48,8 @@ class PamAuthBypassConfiguration extends TaintTracking::Configuration { } } -class PamAuthBypassConfig extends TaintTracking::Configuration { - PamAuthBypassConfig() { this = "PAM auth bypass2" } +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) @@ -61,9 +61,9 @@ class PamAuthBypassConfig extends TaintTracking::Configuration { } from - PamAuthBypassConfiguration config, PamAuthBypassConfig config2, DataFlow::Node source, + PamStartToAcctMgmtConfig acctMgmtConfig, PamStartToAuthenticateConfig authConfig, DataFlow::Node source, DataFlow::Node sink where not isInTestFile(source.asExpr()) and - (config2.hasFlow(source, sink) and not config.hasFlow(source, _)) + (authConfig.hasFlow(source, sink) and not acctMgmtConfig.hasFlow(source, _)) select source, "This Pam transaction may not be secure." From cea909f03ecb10222c8a52fafe50eaa5e5edbe8a Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Tue, 31 May 2022 11:14:00 +0100 Subject: [PATCH 4/5] Autoformat --- go/ql/src/experimental/CWE-285/PamAuthBypass.ql | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/go/ql/src/experimental/CWE-285/PamAuthBypass.ql b/go/ql/src/experimental/CWE-285/PamAuthBypass.ql index 4775141b86fa..34e4e21091ee 100644 --- a/go/ql/src/experimental/CWE-285/PamAuthBypass.ql +++ b/go/ql/src/experimental/CWE-285/PamAuthBypass.ql @@ -61,8 +61,8 @@ class PamStartToAuthenticateConfig extends TaintTracking::Configuration { } from - PamStartToAcctMgmtConfig acctMgmtConfig, PamStartToAuthenticateConfig authConfig, DataFlow::Node source, - DataFlow::Node sink + 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, _)) From d4f9c75315913edb61014a5d88285b13be30981a Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Tue, 31 May 2022 11:14:36 +0100 Subject: [PATCH 5/5] Remove dead code --- go/ql/src/experimental/CWE-285/PamAuthBypass.ql | 4 ---- 1 file changed, 4 deletions(-) diff --git a/go/ql/src/experimental/CWE-285/PamAuthBypass.ql b/go/ql/src/experimental/CWE-285/PamAuthBypass.ql index 34e4e21091ee..06f2904599ed 100644 --- a/go/ql/src/experimental/CWE-285/PamAuthBypass.ql +++ b/go/ql/src/experimental/CWE-285/PamAuthBypass.ql @@ -28,10 +28,6 @@ class PamAcctMgmt extends Method { PamAcctMgmt() { this.hasQualifiedName("github.com/msteinert/pam", "Transaction", "AcctMgmt") } } -class PamTransaction extends StructType { - PamTransaction() { this.hasQualifiedName("github.com/msteinert/pam", "Transaction") } -} - class PamStartFunc extends Function { PamStartFunc() { this.hasQualifiedName("github.com/msteinert/pam", ["StartFunc", "Start"]) } }