Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use fewer buffers and reduce string-copying #7

Merged
merged 12 commits into from
Oct 28, 2019

Conversation

UnitedMarsupials-zz
Copy link
Contributor

@UnitedMarsupials-zz UnitedMarsupials-zz commented Feb 8, 2017

Also:

  • call mkstemp(3) instead of a static name for the action-script
  • Use syslog(3) to log errors instead of printing them to stderr
  • Use default prompt-strings with pam_get_authtok and pam_get_user
  • Use more accurate return values in cases of different errors, instead of always replying with PAM_AUTH_ERR

they should ever be communicated to the user, but, if any should be,
pam_error() should be used for the purpose instead of writing to
stderr directly.
any is necessary. This saves a modicum of space but also, more
importantly, makes usage of our module harder to detect for any
(potentially hostile) observer.
@rafket
Copy link
Owner

rafket commented Feb 10, 2017

I disagree with the syslog usage. These errors are very serious and the user should be aware of them upon usage.

Also, the casting is necessary, as those variables are of different type. It doesn't matter if the compiler understands this and converts them either way. They need to be cast for clarity and readability of the code.

There is no need to allow larger usernames and then truncate them.

I agree with the rest of the changes.

…ne --

/usr/share is read-only on some systems. Use the path /var/db/multipassword
on BSD-systems.

Catch and properly report (with syslog(3)) more potential failures.
@UnitedMarsupials-zz
Copy link
Contributor Author

These errors are very serious and the user should be aware of them upon usage.

Reporting these errors will reveal usage of pam_duress to a potentially hostile party watching from behind the user's shoulder... If they are in a safe environment, they can login as themselves (or root) and find all of the error-messages in the logfile...

At the very least, PAM_SILENT-flag should be obeyed -- it controls, whether or not a PAM-module is allowed to produce output (other than through syslog(3)) at all. If the flag is not set, then pam_error(3) (and/or pam_info(3)) should be used to communicate information to the user, not writing directly to stderr -- for all we know, the user may be logging in through GUI (such as xdm)...

But best, in my opinion, is to just be silent. :-)

Also, the casting is necessary, as those variables are of different type.

Some of the casting was not quite right, because it dropped the const-qualifier. Instead of fixing it all, I simply removed it altogether -- I normally compile with -Wno-pointer-sign, which is a FreeBSD default. I'll add the warning explicitly and fix the calls...

There is no need to allow larger usernames and then truncate them.

They are only truncated, if they are too long even for what is allowed. In cases of crashes, an admin may have easier time sorting through abandoned /tmp/action.*.foo, may he not?

@Lqp1
Copy link
Contributor

Lqp1 commented Oct 18, 2019

Any chance to see this PR merged - at least partially into master ?

@rafket rafket merged commit a0cfac8 into rafket:master Oct 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants