rafket / pam_duress

A pam module written in C for duress codes in linux authentication
GNU General Public License v2.0
146 stars 11 forks source link

Use fewer buffers and reduce string-copying #7

Closed UnitedMarsupials-zz closed 5 years ago

UnitedMarsupials-zz commented 7 years ago

Also:

rafket commented 7 years ago

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.

UnitedMarsupials-zz commented 7 years ago

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 commented 5 years ago

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