[Ar-maan05]

Fixes #319.

pwd_init() sets SIGINT to SIG_IGN, and passwd calls it before do_pam_passwd(), so Ctrl-C at the pam_chauthtok() prompt does nothing. This is the only path with the bug: the non-PAM path prompts before pwd_init(), and chfn, chsh and gpasswd collect their input before calling it, so I left pwd_init() alone.

The change saves the inherited SIGINT disposition before pwd_init() and restores it just before do_pam_passwd() in the interactive PAM path. SIGINT is left as inherited, not forced to SIG_DFL. SIGQUIT and the other signals stay ignored.

Verification

Verified in a Fedora 44 container with --with-libpam. configure reports PAM support: yes and passwd.c builds without warnings. The new test fails on master with a timeout ("passwd ignored SIGINT at the password prompt") and passes with this change.

Disclosure

The code change made after an LLM-based review is documented in the commit message.

[alejandro-colomar]

Disclosure

The pam_unix and dash/echo findings above come from an AI-assisted review of this change.

To be honest, I can't trust it having come from an AI-assisted review. Those are wrong (and very subtly so, so that it's hard to tell whether they're right) more than 50% of the time.

Can you please clarify what part of this is not AI-poisoned? I'd like to read only what was done and known by you before interacting with an AI.

[alejandro-colomar]

To be clear, here's our AI guidelines:

	It is expressly forbidden to contribute to this project any
	content that has been created with non-trivial assistance of
	AI tools.

	The use of AI tools should be minimal, limited to reviewing
	a contribution once it's almost finished.  Use of AI tools in
	the process of a contribution should be disclosed explicitly and
	clearly.  If such use of AI tools resulted in changing the
	contribution in any way --for example, if an AI-based static
	analyzer detected a bug, which was fixed before sending the
	contribution--, this should be disclosed explicitly with more
	detail.

The description of your contribution is AI-derived, and thus expressly forbidden.

You could use an AI at the end, to verify you didn't forget anything --once the contribution is finished--, but you should not use it to change the contribution in any way. The contribution should only be changed after an AI review if it catches something really grave, like a buffer overflow.

So, what part of this contribution is not AI-poisoned?

[Ar-maan05]

I apologize for the confusion. After writing the patch, I had a locally running LLM review the patch, it raised a few points, only one changed the actual patch.

• The pam_unix analysis (temp file + rename under lckpwdf), used to argue that interrupting is safe. I checked passverify.c myself to verify.
• The dash/echo observation. Because of which, I removed a check from the test that terminal echo was restored after ctrl-C, since it failed under dash.
• The note about su was mostly thanks to the AI-assisted review. I can remove it from the description if you'd prefer that.

The description was written by me, but i did use the AI-assisted review as a reference for some parts of it (namely the pam_unix analysis part, the su note, and the "Test" part).

[alejandro-colomar]

I apologize for the confusion. After writing the patch, I had a locally running LLM review the patch, it raised a few points, only one changed the actual patch.

* The pam_unix analysis (temp file + rename under lckpwdf), used to argue that interrupting is safe. I checked passverify.c myself to verify.

* The dash/echo observation. Because of which, I removed a check from the test that terminal echo was restored after ctrl-C, since it failed under dash.

* The note about su was mostly thanks to the AI-assisted review. I can remove it from the description if you'd prefer that.

Yes, please.

The issues you've changed in code after working with the LLM, let's keep them but document them in the commit message as having been fixed after LLM-based review. Please be clear in the commit message about exactly what code has changed after LLMs.

From the description and commit message, please remove entirely anything poisoned by an LLM. I prefer reviewing the code without any description than with an LLM-poisoned description.

The description was written by me, but i did use the AI-assisted review as a reference for some parts of it (namely the pam_unix analysis part, the su note, and the "Test" part).

[Ar-maan05]

I have documented the code change made after the AI-assisted review in the commit message and removed all "LLM-poisoned" parts from the description.

[alejandro-colomar]

Thanks!

Cc: @archenemies, @edneville

[hallyn]

Hi, any reason not to also re-enable ctrl-c for the call to agetpass_internal()? But really, for the pam case, is this safe? pam_chauthtok doesn't only prompt for the password, it effects the change. So you could be ctrl-c'ing during that work. I think you have to create a wrapped conversation function, which enables sigint at start and disables again when done. This will complicate the SHADOW_PAM_CONVERSATION currently set up in configure.ac, but then again, maybe in the process you can make it more palatable :)

[alejandro-colomar]

Hi, any reason not to also re-enable ctrl-c for the call to agetpass_internal()? But really, for the pam case, is this safe? pam_chauthtok doesn't only prompt for the password, it effects the change. So you could be ctrl-c'ing during that work. I think you have to create a wrapped conversation function, which enables sigint at start and disables again when done. This will complicate the SHADOW_PAM_CONVERSATION currently set up in configure.ac, but then again, maybe in the process you can make it more palatable :)

TBH, I'm of the opinion that I prefer a simpler and safer implementation.

Having to wait 1 s when I make a mistake can be tiring, but I'd pay that price for having a simple code that is obviously safe.

[Ar-maan05]

Hi,

any reason not to also re-enable ctrl-c for the call to
agetpass_internal()?

@hallyn for agetpass, I don't think it needs anything. In the non-PAM build, new_password() runs before pwd_init(), so SIGINT is still whatever is inherited, and readpassphrase() already restores the tty and re-raises on ctrl-c.

But really, for the pam case, is this safe? pam_chauthtok doesn't
only prompt for the password, it effects the change. So you could
be ctrl-c'ing during that work.

fair point on PAM though. pam_unix itself writes /etc/shadow atomically, but a stack with pwhistory, gnome-keyring, sss, etc could end up half-applied if we die in the middle, so I can't really claim the current version is safe in general.

I see two possible ways to go:

1. Wrap the conversation function so SIGINT is only live while we're sitting at a prompt. I'd maybe go one step further and use a handler without SA_RESTART, so ctrl-C makes the conversation return PAM_CONV_ERR and the stack fails normally, instead of the process getting killed inside a module.

2. Drop the PAM change and leave things as they are.

TBH, I'm of the opinion that I prefer a simpler and safer implementation.

@alejandro-colomar by "simpler and safer", do you mean you'd rather have 2, or a minimal version of 1? i'd be happy to do either, just don't want to write the wrapper if it's not wanted :)

[alejandro-colomar]

Hi,

any reason not to also re-enable ctrl-c for the call to
agetpass_internal()?

@hallyn for agetpass, I don't think it needs anything. In the non-PAM build, new_password() runs before pwd_init(), so SIGINT is still whatever is inherited, and readpassphrase() already restores the tty and re-raises on ctrl-c.

But really, for the pam case, is this safe? pam_chauthtok doesn't
only prompt for the password, it effects the change. So you could
be ctrl-c'ing during that work.

fair point on PAM though. pam_unix itself writes /etc/shadow atomically, but a stack with pwhistory, gnome-keyring, sss, etc could end up half-applied if we die in the middle, so I can't really claim the current version is safe in general.

I see two possible ways to go:

1. Wrap the conversation function so SIGINT is only live while we're sitting at a prompt. I'd maybe go one step further and use a handler without SA_RESTART, so ctrl-C makes the conversation return PAM_CONV_ERR and the stack fails normally, instead of the process getting killed inside a module.

2. Drop the PAM change and leave things as they are.

TBH, I'm of the opinion that I prefer a simpler and safer implementation.

@alejandro-colomar by "simpler and safer", do you mean you'd rather have 2, or a minimal version of 1? i'd be happy to do either, just don't want to write the wrapper if it's not wanted :)

I don't want to impose my view. If @hallyn feels safe with some of the changes you propose, and they don't mean a lot of complexity, that could be okay.

Also, until you show actual code, I don't know how I'll feel about it.

I could certainly live with 2. A minimal version of 1 might be acceptable. And a non-minimal version of 1 might be acceptable, if it's somehow more robust than a minimal version of 1, and @hallyn thinks it's okay.