Page MenuHomeFreeBSD

pam_exec: fix segfault when existing auth token is null on `pam_sm_set_cred`
AcceptedPublic

Authored by nyan_myuji.xyz on Tue, May 10, 6:30 PM.

Details

Reviewers
des
Summary

According to pam_exec(8), the expose_authtok option should be ignored when
the service function is pam_sm_setcred. Currently pam_exec only prevent
prompt for anth token when expose_authtok is set on pam_sm_setcred. This
subsequently led to segfault when there isn't an existing auth token available.

Bug reported on this: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=263893

After reading https://reviews.freebsd.org/rS349556 I am not sure if the default
behaviour supposed to be simply not prompt for authentication token, or is it
to ignore the option entirely as stated in the man page.

This patch is therefore only adding an additional NULL check on the item
pam_get_item provide, and exit with PAM_SYSTEM_ERR when such item is NULL.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 45593
Build 42481: arc lint + arc unit

Event Timeline

also check rc

check value of rc to only catch the cases when item is null but rc is success

Is it ever expected there *is* an authtok available when setcred is called? If not, wouldn't this still fail (and just avoid the crash)?

Is it ever expected there *is* an authtok available when setcred is called?

Yes, if an earlier module (e.g. passwdqc) already prompted for a new password. And the old one will be in PAM_OLD_AUTHTOK if memory serves.

So, does this patch look fine?

Cause if so, I'd apply it locally, so I can move forward writing/testing a "unix selfauth" helper for use with pam_exec :)

des requested changes to this revision.Mon, May 16, 1:43 PM
des added inline comments.
lib/libpam/modules/pam_exec/pam_exec.c
266
268–270

There's no harm in setting authtok to NULL, so you can just leave the assignment outside the if.

This revision now requires changes to proceed.Mon, May 16, 1:43 PM
  • Rephrase error message and formatting

Hang on, I think the change I requested may have been incorrect...

des accepted this revision.EditedMon, May 16, 4:07 PM

Ah, sorry, I forgot that OUT() is a macro that wraps return, so everything is fine.

lib/libpam/modules/pam_exec/pam_exec.c
263

extraneous blank line

This revision is now accepted and ready to land.Mon, May 16, 4:07 PM
  • Rephrase error message and formatting
  • Removed blank line
This revision now requires review to proceed.Mon, May 16, 5:09 PM
This revision is now accepted and ready to land.Mon, May 16, 5:10 PM