Skip to content
This repository was archived by the owner on Jan 5, 2023. It is now read-only.

Golang : Add Query To Detect PAM Authorization Bugs #709

Closed
ghost wants to merge 1 commit into from
Closed

Golang : Add Query To Detect PAM Authorization Bugs #709

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
68 changes: 68 additions & 0 deletions ql/src/experimental/CWE-285/PamAuthBypass.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
/**
* @name PAM Authentication Bypass
* @description Depending on
* @kind problem
* @problem.severity warning
* @id go/unreachable-statement
* @tags maintainability
* correctness
* external/cwe/cwe-561
* @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."
49 changes: 49 additions & 0 deletions ql/src/experimental/CWE-285/PamAuthBypassLocal.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
/**
* @name PAM Authentication Bypass
Copy link
Copy Markdown
Contributor

@smowton smowton May 30, 2022

Choose a reason for hiding this comment

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

What is the local version for? It should be strictly a subset of the global one, right?

Copy link
Copy Markdown
Author

@ghost ghost May 30, 2022

Choose a reason for hiding this comment

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

The global one has a longer runtime but is more accurate. The local one has a shorter runtime but would miss some edge cases; Not that I have found any in the wild.

* @description Depending on
* @kind problem
* @problem.severity warning
* @id go/unreachable-statement
* @tags maintainability
* correctness
* external/cwe/cwe-561
* @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."
1 change: 1 addition & 0 deletions ql/test/experimental/CWE-285/PamAuthBypass.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
| main.go:10:2:12:3 | ... := ...[0] | This Pam transaction may not be secure. |
1 change: 1 addition & 0 deletions ql/test/experimental/CWE-285/PamAuthBypass.qlref
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
experimental/CWE-285/PamAuthBypass.ql
5 changes: 5 additions & 0 deletions ql/test/experimental/CWE-285/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
module main

go 1.18

require github.com/msteinert/pam v1.0.0
28 changes: 28 additions & 0 deletions ql/test/experimental/CWE-285/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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() {}
68 changes: 68 additions & 0 deletions ql/test/experimental/CWE-285/vendor/github.com/msteinert/pam/stub.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions ql/test/experimental/CWE-285/vendor/modules.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# github.com/msteinert/pam v1.0.0
## explicit
github.com/msteinert/pam