Added pam misconfig detector by mzfr · Pull Request #1705 · google/osv-scalibr

mzfr · 2026-01-22T06:08:19Z

Also please let me know if I should split either the tests or the main detector logic in multiple files

alessandro-Doyensec · 2026-02-09T10:39:04Z

detector/misc/pammisconfig/pammisconfig.go

+}
+
+// PAM control flags that can cause authentication bypass when misconfigured.
+// Reference: http://www.linux-pam.org/Linux-PAM-html/sag-configuration-file.html


I’m getting a 404 when I click the link.

alessandro-Doyensec · 2026-02-09T10:40:59Z

detector/misc/pammisconfig/pammisconfig.go

+type Detector struct{}
+
+// New returns a new PAM misconfiguration detector.
+func New() detector.Detector {


The constructor is never called throughout the project. Every plugin should be registered in a list.go file, see: https://github.com/google/osv-scalibr/blob/main/detector/list/list.go

alessandro-Doyensec · 2026-02-09T11:00:04Z

detector/misc/pammisconfig/pammisconfig.go

+			authEntries = append(authEntries, *entry)
+		}
+
+		if lineIssues := analyzePAMLine(filePath, lineNum, line, isLegacyFormat); len(lineIssues) > 0 {


Is there a reason why the entry isn't directly passed to the analyzePAMLine function?

alessandro-Doyensec · 2026-02-09T11:01:34Z

detector/misc/pammisconfig/pammisconfig.go

+}
+
+// analyzePAMLine analyzes a single PAM configuration line for security issues.
+func analyzePAMLine(filePath string, lineNum int, line string, isLegacyFormat bool) []string {


If it's possible to directly pass the PAM entry to this function as an argument (ref #1705 (comment)), rename this analyzePAMEntry

alessandro-Doyensec · 2026-02-09T11:02:17Z

detector/misc/pammisconfig/pammisconfig.go

+	control    string   // required, sufficient, optional, etc.
+	modulePath string   // e.g., pam_unix.so, pam_permit.so
+	args       []string // module arguments
+}


Move parsePAMLine right below the pamEntry definition.

alessandro-Doyensec · 2026-02-09T11:04:27Z

detector/misc/pammisconfig/pammisconfig.go

+	return false
+}
+
+type controlEffect struct {


Please, document what a controlEffect is (providing an example)

alessandro-Doyensec · 2026-02-09T11:09:00Z

detector/misc/pammisconfig/pammisconfig.go

+	isOptionalOnly   bool
+}
+
+func parseControlEffect(control string) controlEffect {


Can the parseControlEffect logic be moved inside the parsePAMLine function?

I would expect the pamEntry struct to be something like:

type pamEntry struct { moduleType string // auth, account, password, session control controlEffect // required, sufficient, optional, etc. modulePath string // e.g., pam_unix.so, pam_permit.so args []string // module arguments }

This way the parsing and checking step could be completely separated.

alessandro-Doyensec · 2026-02-09T11:12:33Z

Please, Add the detector to the docs/supported_inventory_types.md.

alessandro-Doyensec · 2026-02-09T11:12:53Z

Hi @mzfr

Thanks for the pull request!

I’ve reviewed the code and left a few comments for you to address.

Add misc/pammisconfig to the Linux detector list and supported plugins doc. Refactor PAM parsing to compute control effects once per entry.

alessandro-Doyensec · 2026-02-11T16:04:56Z

detector/list/list.go

+var Misc = concat(
+	InitMap{
+		cronjobprivesc.Name: {noCFG(cronjobprivesc.New)},
+		dockersocket.Name:   {noCFG(dockersocket.New)},


Directly add the plugins registration here (remove list/misc_linux.go and list/misc_other.go):

pammisconfig.Name: {noCFG(pammisconfig.New)},

As a result TestDetectorsFromName/Find_misc_detectors will fail, to fix it just add: ""misc/pammisconfig" into the wantDets.

alessandro-Doyensec · 2026-02-11T16:08:50Z

detector/misc/pammisconfig/pammisconfig.go

+	return "password authentication"
+}
+
+func checkOptionalOnlyAuth(filePath string, entries []pamEntry) []string {


Please, document the checkOptionalOnlyAuth function

alessandro-Doyensec · 2026-02-11T16:09:43Z

detector/misc/pammisconfig/pammisconfig.go

+type pamEntry struct {
+	moduleType string // auth, account, password, session
+	control    string // required, sufficient, optional, etc.
+	controlEff controlEffect


Is the controlEff field always populated? If not make it a pointer to controlEffect (using *controlEffect as type).

alessandro-Doyensec · 2026-02-11T16:10:08Z

Hi @mzfr

Thanks for the changes.

I left a couple more comments for you to address

Register misc/pammisconfig in the main misc list, update list tests, and document/nil-guard control effects.

alessandro-Doyensec · 2026-02-12T17:17:23Z

detector/misc/pammisconfig/pammisconfig.go

+	// and when combined with 'sufficient', those users bypass further auth checks.
+	// Reference: https://unix.stackexchange.com/a/767197
+	if isModule(entry.modulePath, "pam_succeed_if.so") {
+		control := parsedControlEffect(entry.controlEff)


Can we add a nil-check for .controlEff and skip the if corpus if it's missing?

I’m also fine with rolling the type back to controlEff (removing the pointer) as long as the default values are coherent

alessandro-Doyensec · 2026-02-12T17:19:26Z

Hi @mzfr

Thanks for the change, I left a comment for you to check.

I see in the code that you are using the .controlEffect as if the pointer was not there.

Ideally you could check if entry.controlEff is nil and return early since neither Check 1 and Check 2 make sense in that case.

alessandro-Doyensec · 2026-02-13T10:00:29Z

detector/misc/pammisconfig/pammisconfig.go

+	isSufficientLike bool
+	skipNext         int
+	isOptionalOnly   bool
+}


Please add the following helper methods to the controllEffect type:

func (c *controlEffect) ShortCircuits() bool { if c == nil { return false } return c.isSufficientLike || c.skipNext > 0 } func (c *controlEffect) IsOptionalOnly() bool { if c == nil { return false } return c.isOptionalOnly }

Feel free to rename the ShortCircuits as you deem corrrect

alessandro-Doyensec · 2026-02-13T10:01:09Z

detector/misc/pammisconfig/pammisconfig.go

+	// pam_permit.so ALWAYS returns success, so using it with bypass controls
+	// means any user can authenticate without a valid password.
+	// Reference: https://serverfault.com/questions/890012/pam-accepting-any-password-for-valid-users
+	if hasControlEffect && isModule(entry.modulePath, "pam_permit.so") {


Use

if entry.controlEff.ShortCircuits() && isModule(entry.modulePath, "pam_permit.so")

here

alessandro-Doyensec · 2026-02-13T10:01:35Z

detector/misc/pammisconfig/pammisconfig.go

+	// pam_succeed_if.so can match broad user groups (e.g., uid >= 1000),
+	// and when combined with 'sufficient', those users bypass further auth checks.
+	// Reference: https://unix.stackexchange.com/a/767197
+	if hasControlEffect && isModule(entry.modulePath, "pam_succeed_if.so") {


Use

if entry.controlEff.ShortCircuits() && isModule(entry.modulePath, "pam_succeed_if.so")

here

alessandro-Doyensec · 2026-02-13T10:01:54Z

detector/misc/pammisconfig/pammisconfig.go

+	return false
+}
+
+func parsedControlEffect(effect *controlEffect) controlEffect {


Please, remove this function

alessandro-Doyensec · 2026-02-13T10:05:27Z

detector/misc/pammisconfig/pammisconfig.go

+
+	var hasEffectiveNonOptional bool
+	var hasPermitOptional bool
+	for _, entry := range entries {


Please, adjust to use:

entry.controlEff.IsOptionalOnly()

alessandro-Doyensec · 2026-02-13T10:06:24Z

Hello @mzfr

Thanks for the changes, I left a couple of comments regarding the addition of helper function which should simplify the logic and remove some nesting from the code.

Thanks again!

Use nil-safe controlEffect helpers and remove duplicate parsing logic

alessandro-Doyensec

Hello @mzfr

Thanks for the changes! Looks good to merge now.

google/osv-scalibr#1705

Added pam misconfig detector

9afd0d2

alessandro-Doyensec self-requested a review February 9, 2026 09:52

alessandro-Doyensec reviewed Feb 9, 2026

View reviewed changes

Register PAM misconfig detector

e9e0ad6

Add misc/pammisconfig to the Linux detector list and supported plugins doc. Refactor PAM parsing to compute control effects once per entry.

alessandro-Doyensec reviewed Feb 11, 2026

View reviewed changes

Refine PAM detector registration and parsing

65fc7f8

Register misc/pammisconfig in the main misc list, update list tests, and document/nil-guard control effects.

alessandro-Doyensec reviewed Feb 12, 2026

View reviewed changes

Add nil guard for PAM control effects

af42aa7

alessandro-Doyensec reviewed Feb 13, 2026

View reviewed changes

refactor(pammisconfig): simplify control effect checks

af837ed

Use nil-safe controlEffect helpers and remove duplicate parsing logic

alessandro-Doyensec approved these changes Feb 17, 2026

View reviewed changes

alessandro-Doyensec added the lgtm The PR has been reviewed and approved ("Looks Good To Me") by vendors helping with code reviews. label Feb 17, 2026

mzfr added a commit to mzfr/security-testbeds that referenced this pull request Feb 17, 2026

Added security testbed for osv scalibr pammisconfig

d52ab1d

google/osv-scalibr#1705

mzfr mentioned this pull request Feb 17, 2026

Added security testbed for osv scalibr pammisconfig google/security-testbeds#191

Open

Conversation

mzfr commented Jan 22, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alessandro-Doyensec Feb 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alessandro-Doyensec commented Feb 9, 2026

Uh oh!

alessandro-Doyensec commented Feb 9, 2026

Uh oh!

alessandro-Doyensec Feb 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alessandro-Doyensec commented Feb 11, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alessandro-Doyensec commented Feb 12, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alessandro-Doyensec Feb 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alessandro-Doyensec commented Feb 13, 2026

Uh oh!

alessandro-Doyensec left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

alessandro-Doyensec Feb 9, 2026 •

edited

Loading

alessandro-Doyensec Feb 11, 2026 •

edited

Loading

alessandro-Doyensec Feb 13, 2026 •

edited

Loading