Support disabling password login via -l flag to passwd and fix exit code error in passwd #8

schuelermine · 2024-09-24T21:12:58Z

Note: I haven’t yet been able to build this, but it seems like a simple change that should work.

This is two commits:

Fix a bug where passwd exits with zero exit code indicating success even on error. The variable ret was set up which would enable returning the correct exit code on error, but this wasn’t used. The commit returns ret as was probably originally intended.
Enable passwd to receive a -l argument that causes it to remove the .termux_authinfo file, in analogy to shadow’s passwd’s -l option. This is accomplished by splitting main into two functions, main_set_password and main_remove_password, and adding the function termux_remove_passwd, analogous to termux_set_passwd, which main_remove_password calls. main itself is replaced by a simple switch to check the arguments.

This is technically a breaking change, though it breaks behaviors that I would expect weren’t and shouldn’t’ve been relied upon. Specifically:

The exit code on failure changes.
Arguments to passwd other than no arguments and a single -l are rejected, previously they were silently ignored.
Behavior of passwd when passed -l changes.

schuelermine · 2024-09-24T21:15:11Z

Sorry, force push because I forgot to make the new functions static.

Grimler91

Thanks!

Maybe I am just not well-versed in the unix world, but why -l for removing? Is there a particular passwd implementation that has that command? On my system I have:

$ passwd --help
[ ... ]
  -d, --delete                  delete the password for the named account
  -e, --expire                  force expire the password for the named account
  -h, --help                    display this help message and exit
  -k, --keep-tokens             change password only if expired
  -i, --inactive INACTIVE       set password inactive after expiration
                                to INACTIVE
  -l, --lock                    lock the password of the named account
[ ... ]

passwd.c

Grimler91 · 2024-09-25T08:29:04Z

passwd.c

+    case 1:
+        return main_set_password();
+    case 2:
+        if (strcmp(argv[1], "-l") == 0) {


Please change to use strncmp(argv[1], "-l", 2) just to follow good practice

I used strcmp since the rest of the code also uses it. Isn't the suggested use of strncmp exactly equivalent in this case? And aren't argv elements guaranteed to be nul-terminated?
But I guess it's good in case something changes.

Yeah, no changes for risk of out-of-bounds reads (I think). IMHO a good idea to always use safe functions nevertheless.
Only change would be that any arg -l* is then interpreted as -l also.

~~we also have to remember that github copilot monitors everything we do here, so with strcmp we risk microsoft suing us after copilot introduces strcmp vulnerabilities in other projects~~

I don't think accepting anything starting with -l is a good idea though

Maybe a comment like this?

if (strcmp(argv[1], "-l") == 0) { // safe since argv[n] and literals are nul-terminated

passwd.c

termux-auth.c

schuelermine · 2024-09-25T11:41:49Z

I choose -l because that's effectively what -l in shadow's passwd does. It no longer lets the user login by password.

TomJo2000 · 2024-09-25T21:54:36Z

I choose -l because that's effectively what -l in shadow's passwd does. It no longer lets the user login by password.

To quote from the man 1 passwd man page.

-l, --lock
    Lock the password of the named account. This option disables a
    password by changing it to a value which matches no possible
    encrypted value (it adds a ´!´ at the beginning of the password).
                                                                        
    Note that this does not disable the account. The user may still be
    able to login using another authentication token (e.g. an SSH key).
    To disable the account, administrators should use usermod
    --expiredate 1 (this set the account's expire date to Jan 2, 1970).
                                                                        
    Users with a locked password are not allowed to change their
    password.

Shadow-Utils passwd -l explicitly does not remove the users password.
The more appropriate flag in that case would be -d, quoting again:

-d, --delete
    Delete a user's password (make it empty). This is a quick way to
    disable a password for an account. It will set the named account
    passwordless.

As the paragraph from the man page would indicate, this does not disable the account.
In the context of Termux it would restore the default state of having no password set,
which would only serve to disallow password based remote authentication over for example SSH.

schuelermine · 2024-09-25T22:01:02Z

I see.
I hadn’t considered that the behavior over SSH would be different from the local behavior and expected -d to make login automatic with no verification on SSH, too (as it does locally), and since this wasn’t the case I concluded that -l was necessary to disable password login via SSH.

schuelermine · 2024-09-25T22:09:31Z

@TomJo2000 fixed

sylirre · 2024-09-26T00:43:04Z

Deleting ~/.termux_authinfo locks the account. It doesn't disable password authentication but will make that no one of supplied passwords will succeed. This is not equivalent of having the empty password.

termux-auth doesn't really support empty passwords.

-l is preferred. Possibly it can even be an alias to -d.

TomJo2000 · 2024-09-26T00:56:16Z

Deleting ~/.termux_authinfo locks the account. It doesn't disable password authentication but will make that no one of supplied passwords will succeed. This is not equivalent of having the empty password.

termux-auth doesn't really support empty passwords.

-l is preferred. Possibly it can even be an alias to -d.

It is my understanding that since Termux is a regular Android app
we don't have access to /etc/shadow (or the Android equivalent),
thus the encrypted password is stored in ~/.termux_authinfo.

Would removing that file not "delete" the set password?
The concept of "locking" an account does not make sense to me in a Termux context,
since Termux does not have the ability to create new users, and is thus an effective single user system.

schuelermine · 2024-09-26T01:06:57Z

An alternative is to accept both

schuelermine · 2024-09-26T01:07:24Z

Since, if we consider launching Termux as a way to log in outside of the usual shadow/login mechanisms, the behavior is identical in either case

sylirre · 2024-09-26T08:19:57Z

Would removing that file not "delete" the set password?

The concept of "locking" an account does not make sense to me in a Termux context,
since Termux does not have the ability to create new users, and is thus an effective single user system.

@TomJo2000 termux-auth was never designed to be compatible with PAM or shadow-utils.

Its exclusive purpose is to provide way of password-based authentication for packages openssh, dropbear and shellinabox.

So as not to expose users to danger when using remote access utilities, termux-auth was designed to be totally locked down when password is not set.

Whenever ~/.termux_authinfo exists, the termux-auth will always ask for password and compare pbkdf result with one of that file. If ~/.termux_authinfo is empty or pbkdf material was somehow generated from empty string, packages using termux-auth will always ask for password.

This behavior differs from what you can observe on normal Linux systems where empty password in /etc/shadow permits login without authentication (no password prompt appears).

TomJo2000 · 2024-09-26T09:26:26Z

Its exclusive purpose is to provide way of password-based authentication for packages openssh, dropbear and shellinabox.

So as not to expose users to danger when using remote access utilities, termux-auth was designed to be totally locked down when password is not set.

I did not realize that was something we implemented specifically.
I recall SSH not allowing you to login with a user that has no password set on regular Linux systems as well, but I did not go out of my way to check that.

sylirre · 2024-09-26T10:00:31Z

I recall SSH not allowing you to login with a user that has no password set on regular Linux systems as well

This is because of PermitEmptyPasswords set to "no" by default. Change it to yes, and ssh will let you to log in without prompt into accounts with removed password.

Not all remote access tools have the same mechanism. Besides OpenSSH, termux-auth is being used by Dropbear and Shellinabox packages.

sylirre

This pull request introduces compilation issues:

Co-authored-by: Henrik Grimler <grimler@termux.dev>

schuelermine · 2024-09-26T10:27:15Z

@sylirre fixed!

schuelermine · 2024-09-26T10:28:19Z

I think accepting both -l and -d is the best solution

sylirre · 2024-09-26T10:34:10Z

Yes, better to provide -l as alias to -d option.

schuelermine · 2024-09-26T10:35:12Z

Should the help message mention both or should one be the canonical form?

sylirre · 2024-09-26T10:58:25Z

Mentioning only one is enough.

schuelermine · 2024-09-26T11:00:46Z

This maybe isn’t the most efficient way of comparing these but who cares, I would rather not write a state machine for this

passwd: now returns failure exit code on failure

8fb7957

schuelermine force-pushed the support-lock branch from 2f858a6 to 98c6638 Compare September 24, 2024 21:14

Grimler91 requested changes Sep 25, 2024

View reviewed changes

sylirre self-requested a review September 25, 2024 11:10

termux deleted a comment from schuelermine Sep 25, 2024

passwd: support -d to remove password login

325cf60

schuelermine force-pushed the support-lock branch from b82e8d2 to cb2c0d9 Compare September 25, 2024 22:09

sylirre suggested changes Sep 26, 2024

View reviewed changes

schuelermine and others added 2 commits September 26, 2024 12:25

passwd: improve error handling for -d

b1424ed

passwd: improve argv error message

934b06a

Co-authored-by: Henrik Grimler <grimler@termux.dev>

schuelermine force-pushed the support-lock branch from cb2c0d9 to 934b06a Compare September 26, 2024 10:26

passwd: permit -l as alias for -d

2e83e1c

update passwd help message

fe7552d

sylirre approved these changes Sep 26, 2024

View reviewed changes

sylirre merged commit 68004b4 into termux:master Sep 26, 2024

Support disabling password login via -l flag to passwd and fix exit code error in passwd #8

Support disabling password login via -l flag to passwd and fix exit code error in passwd #8

Uh oh!

Conversation

schuelermine commented Sep 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

schuelermine commented Sep 24, 2024

Uh oh!

Grimler91 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Grimler91 Sep 25, 2024

Choose a reason for hiding this comment

Uh oh!

schuelermine Sep 25, 2024

Choose a reason for hiding this comment

Uh oh!

Grimler91 Sep 25, 2024

Choose a reason for hiding this comment

Uh oh!

Grimler91 Sep 25, 2024

Choose a reason for hiding this comment

Uh oh!

schuelermine Sep 25, 2024

Choose a reason for hiding this comment

Uh oh!

schuelermine Sep 25, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

schuelermine commented Sep 25, 2024

Uh oh!

TomJo2000 commented Sep 25, 2024

Uh oh!

schuelermine commented Sep 25, 2024

Uh oh!

schuelermine commented Sep 25, 2024

Uh oh!

sylirre commented Sep 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TomJo2000 commented Sep 26, 2024

Uh oh!

schuelermine commented Sep 26, 2024

Uh oh!

schuelermine commented Sep 26, 2024

Uh oh!

sylirre commented Sep 26, 2024

Uh oh!

TomJo2000 commented Sep 26, 2024

Uh oh!

sylirre commented Sep 26, 2024

Uh oh!

sylirre left a comment

Choose a reason for hiding this comment

Uh oh!

schuelermine commented Sep 26, 2024

Uh oh!

schuelermine commented Sep 26, 2024

Uh oh!

sylirre commented Sep 26, 2024

Uh oh!

schuelermine commented Sep 26, 2024

Uh oh!

sylirre commented Sep 26, 2024

Uh oh!

schuelermine commented Sep 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

schuelermine commented Sep 24, 2024 •

edited

Loading

sylirre commented Sep 26, 2024 •

edited

Loading

schuelermine commented Sep 26, 2024 •

edited

Loading