agetpass() to replace getpass(3) #573

alejandro-colomar · 2022-09-27T19:55:42Z

There are several issues with getpass(3).

Many implementations of it share the same issues that the infamous
gets(3).  In glibc it's not so terrible, since it's a wrapper
around getline(3).  But it still has an important bug:

If the password is long enough, getline(3) will realloc(3) memory,
and prefixes of the password will be laying around in some
deallocated memory.

See the getpass(3) manual page for more details, and especially
the commit that marked it as deprecated, which links to a long
discussion in the linux-man@ mailing list.

So, readpassphrase(3bsd) is preferrable, which is provided by
libbsd on GNU systems.  However, using readpassphrase(3) directly
is a bit verbose, so we can write our own wrapper with a simpler
interface similar to that of getpass(3).

One of the benefits of writing our own interface around
readpassphrase(3) is that we can hide there any checks that should
be done always and which would be error-prone to repeat every
time.  For example, check that there was no truncation in the
password.

Also, use malloc(3) to get the buffer, instead of using a global
buffer.  We're not using a multithreaded program (and it wouldn't
make sense to do so), but it's nice to know that the visibility of
our passwords is as limited as possible.

erase_pass() is a clean-up function that handles all clean-up
correctly, including zeroing the entire buffer, and then
free(3)ing the memory.  By using [[gnu::malloc(erase_pass)]], we
make sure that we don't leak the buffers in any case, since the
compiler will be able to enforce clean up.

Link: <https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/commit?id=7ca189099d73bde954eed2d7fc21732bcc8ddc6b>
Reported-by: Christian Göttsche <cgzones@googlemail.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>

I still haven't added autoconf code to add the libbsd dependency, since I don't have much experience in using autoconf. If someone wants to help with that, please do :) Otherwise, gimme some time to do that.

I also reopened an old thread in the libc-alpha@ mailing list to discuss the possible addition of readpassphrase(3) to glibc (and also discuss the agetpass(3) interface, in case they want to add it too).

hallyn · 2022-09-27T21:06:00Z

You'll need to add libbsd-dev to build deps right?

alejandro-colomar · 2022-09-27T21:30:13Z

You'll need to add libbsd-dev to build deps right?

Yep. And in RPM-based distros I guess they call it libbsd-devel.

libmisc/agetpass.c

alejandro-colomar · 2022-09-27T21:44:22Z

Changes: readpassphrase(3): Add RPP_REQUIRE_TTY to flags field.

alejandro-colomar · 2022-09-27T21:45:21Z

Rebased to master.

src/sulogin.c

alejandro-colomar · 2022-09-27T22:07:09Z

Changes:

Split a commit that was conceptually completely different from the others.
wsfix

alejandro-colomar · 2022-09-27T22:14:44Z

You'll need to add libbsd-dev to build deps right?

In which file(s) do you define the build deps? And also the runtime deps, if at all?

libmisc/agetpass.c

src/sulogin.c

ikerexxe

I added some comments.

Yep. And in RPM-based distros I guess they call it libbsd-devel.

You are right.

@hallyn do we have any policy regarding indentation (tabs vs spaces)? And regarding if and mandatory block sequences?

alejandro-colomar · 2022-09-28T16:02:22Z

Changes: Rework indentation in erase_pass(), by returning early. That way, it's more explicit the intention that passing NULL is a no-op. It also removes indentation from a function call, which, to me, is nicer to read.

alejandro-colomar · 2022-09-28T17:21:44Z

Changes:

Add documentation for agetpass() and erase_pass().
Set errno to ENOBUFS from agetpass() (this is irrelevant to shadow utils, but as a library function, it makes sense to be consistent with libc functions). It also makes the code more self-documenting than just a goto label, so now I'm less averse to droping the label.

cgzones

The build system needs to be updated.

Draft:

diff --git a/configure.ac b/configure.ac
index 61f2e588..399bb0cb 100644
--- a/configure.ac
+++ b/configure.ac
@@ -393,6 +393,11 @@ if test "$enable_subids" != "no"; then
 fi
 AM_CONDITIONAL(ENABLE_SUBIDS, test "x$enable_subids" != "xno")
 
+AC_SEARCH_LIBS([readpassphrase], [bsd],,[AC_MSG_ERROR([readpassphrase() function not found])])
+AC_CHECK_HEADERS([readpassphrase.h], [], [
+       AC_CHECK_HEADERS([bsd/readpassphrase.h], [], [AC_MSG_ERROR([readpassphrase.h header file not found])])
+])
+
 AC_SUBST(LIBCRYPT)
 AC_CHECK_LIB(crypt, crypt, [LIBCRYPT=-lcrypt],
        [AC_MSG_ERROR([crypt() not found])])
diff --git a/libmisc/Makefile.am b/libmisc/Makefile.am
index 3d319cd8..636cc439 100644
--- a/libmisc/Makefile.am
+++ b/libmisc/Makefile.am
@@ -8,6 +8,7 @@ noinst_LTLIBRARIES = libmisc.la
 libmisc_la_SOURCES = \
        addgrps.c \
        age.c \
+       agetpass.c \
        audit_help.c \
        basename.c \
        chkname.c \
diff --git a/libmisc/agetpass.c b/libmisc/agetpass.c
index f6eca4be..edee4e19 100644
--- a/libmisc/agetpass.c
+++ b/libmisc/agetpass.c
@@ -26,8 +26,14 @@
  * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
+#include <config.h>
+
 #include <limits.h>
+#ifdef HAVE_READPASSPHRASE_H
 #include <readpassphrase.h>
+#elif defined HAVE_BSD_READPASSPHRASE_H
+#include <bsd/readpassphrase.h>
+#endif
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>

lib/prototypes.h

libmisc/agetpass.c

alejandro-colomar · 2022-09-28T20:44:01Z

Changes:

Reduce goto labels to one instead of two. Now that we set errno to ENOBUFS, that's self-documenting enough to remove the labels. This also has the side effect that we don't need to care if readpassphrase(3) can fail with a partially-filled buffer anymore. Now we always clear the buffer before free(3)ing it. [@cgzones, @ikerexxe]

alejandro-colomar · 2022-09-28T20:46:45Z

The build system needs to be updated.

Draft:

[...]

diff --git a/libmisc/agetpass.c b/libmisc/agetpass.c
index f6eca4be..edee4e19 100644
--- a/libmisc/agetpass.c
+++ b/libmisc/agetpass.c
@@ -26,8 +26,14 @@

OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*/

+#include <config.h>
+
#include <limits.h>
+#ifdef HAVE_READPASSPHRASE_H
#include <readpassphrase.h>
+#elif defined HAVE_BSD_READPASSPHRASE_H
+#include <bsd/readpassphrase.h>
+#endif
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

You should always use the <readpassphrase.h> form, and you need to adapt the build system for that. See libbsd(7):

     The library can be used in an overlay mode, which is the
     preferred way, so that the code is portable and requires no
     modification to the original BSD code.  This can be done
     easily with the pkg‐config(1) library named libbsd‐overlay.
     Or by adding the system‐specific include directory with the
     bsd/ suffix to the list of system include paths.  With gcc
     this could be ‐isystem ${includedir}/bsd.  In addition the
     LIBBSD_OVERLAY pre‐processor variable needs to be defined.
     The includes in this case should be the usual system ones,
     such as <unistd.h>.

     The other way to use the library is to use the namespaced
     headers, this is less portable as it makes using libbsd
     mandatory and it will not work on BSD‐based systems, and
     requires modifying original BSD code.  This can be done
     with the pkg‐config(1) library named libbsd.  The includes
     in this case should be namespaced with bsd/, such as
     <bsd/unistd.h>.

See for example what we do in the Linux man-pages project:
https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/tree/lib/build-src.mk#n17

alejandro-colomar · 2022-09-28T20:59:45Z

Changes: Add agetpass.c to the Makefile. [@cgzones]

alejandro-colomar · 2022-09-28T21:12:11Z

Changes: Hide [[gnu::malloc(x)]] in a macro to workaround the issues in clang. [@cgzones]

libmisc/agetpass.c

ikerexxe

One minor comment left.

Apart from that you should add libbsd-dev or libbsd-devel to the package dependencies in the .build folder. An example of such a file is .builds/ubuntu-focal.yml.

alejandro-colomar · 2022-09-29T09:50:29Z

Changes: Add libbsd-dev[el] to <.build/> files. [@ikerexxe]

alejandro-colomar · 2022-11-22T12:47:12Z

Changes: Added SYNOPSIS to the documentation.

ikerexxe · 2022-11-22T14:16:23Z

Locally it seems to build fine, thanks to @guillemj 's patch. There seems to be some missing piece in the CI. Could you please have a look at it?

Fixed in #596

alejandro-colomar · 2022-11-22T18:24:20Z

Changes: cosmetic fix in function documentation

alejandro-colomar · 2022-11-22T19:16:09Z

Changes:

Better name for erase_pass()'s parameter in the prototype.
Use libmisc: prefix in a commit message.

alejandro-colomar · 2022-11-23T10:32:51Z

Changes: Remove CI bits (already handled in #596 )

alejandro-colomar · 2022-11-23T10:36:16Z

Changes: Add @ikerexxe changes to CI to test them.

src/gpasswd.c

@@ -887,24 +887,24 @@
 	printf (_("Changing the password for group %s\n"), group);

 	for (retries = 0; retries < RETRIES; retries++) {
-		cp = getpass (_("New Password: "));
+		cp = agetpass (_("New Password: "));


src/newgrp.c

@@ -158,7 +158,7 @@
 		 * get the password from her, and set the salt for
 		 * the decryption from the group file.
 		 */
-		cp = getpass (_("Password: "));
+		cp = agetpass (_("Password: "));


src/passwd.c

@@ -204,15 +204,15 @@
 	 */

 	if (!amroot && ('\0' != crypt_passwd[0])) {
-		clear = getpass (_("Old password: "));
+		clear = agetpass (_("Old password: "));


src/passwd.c

@@ -286,7 +286,7 @@

 	warned = false;
 	for (i = getdef_num ("PASS_CHANGE_TRIES", 5); i > 0; i--) {
-		cp = getpass (_("New password: "));
+		cp = agetpass (_("New password: "));


src/sulogin.c

@@ -182,7 +182,7 @@
 		 */

 		/* get a password for root */
-		cp = getpass (_(
+		cp = agetpass (_(


Signed-off-by: Alejandro Colomar <alx@kernel.org>

…s safely There are several issues with getpass(3). Many implementations of it share the same issues that the infamous gets(3). In glibc it's not so terrible, since it's a wrapper around getline(3). But it still has an important bug: If the password is long enough, getline(3) will realloc(3) memory, and prefixes of the password will be laying around in some deallocated memory. See the getpass(3) manual page for more details, and especially the commit that marked it as deprecated, which links to a long discussion in the linux-man@ mailing list. So, readpassphrase(3bsd) is preferrable, which is provided by libbsd on GNU systems. However, using readpassphrase(3) directly is a bit verbose, so we can write our own wrapper with a simpler interface similar to that of getpass(3). One of the benefits of writing our own interface around readpassphrase(3) is that we can hide there any checks that should be done always and which would be error-prone to repeat every time. For example, check that there was no truncation in the password. Also, use malloc(3) to get the buffer, instead of using a global buffer. We're not using a multithreaded program (and it wouldn't make sense to do so), but it's nice to know that the visibility of our passwords is as limited as possible. erase_pass() is a clean-up function that handles all clean-up correctly, including zeroing the entire buffer, and then free(3)ing the memory. By using [[gnu::malloc(erase_pass)]], we make sure that we don't leak the buffers in any case, since the compiler will be able to enforce clean up. Link: <https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/commit?id=7ca189099d73bde954eed2d7fc21732bcc8ddc6b> Reported-by: Christian Göttsche <cgzones@googlemail.com> Signed-off-by: Alejandro Colomar <alx@kernel.org>

getpass(3) is broken in all implementations; in some, more than others, but somewhat broken in all of them. Check the immediate previous commit, which added the functions, for more details. Check also the Linux man-pages commit that marked it as deprecated, for more details: 7ca189099d73bde954eed2d7fc21732bcc8ddc6b. Link: <https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/commit?id=7ca189099d73bde954eed2d7fc21732bcc8ddc6b> Reported-by: Christian Göttsche <cgzones@googlemail.com> Signed-off-by: Alejandro Colomar <alx@kernel.org>

Clang doesn't implement this attribute and reports an error. Work around it by hiding it in a macro that will be empty in clang. Reported-by: Christian Göttsche <cgzones@googlemail.com> Signed-off-by: Alejandro Colomar <alx@kernel.org>

Signed-off-by: Guillem Jover <guillem@hadrons.org> Signed-off-by: Alejandro Colomar <alx@kernel.org>

alejandro-colomar · 2022-11-28T17:42:13Z

Rebase to master

alejandro-colomar · 2022-11-28T17:43:50Z

Changes: reword caveats.

calling erase_pass() twice is actually much worse than calling free(3) twice. It's calling memset(3) on freed memory. That's critical undefined behavior.

alejandro-colomar · 2022-11-28T17:44:24Z

I'm happy with the patch set now. If it passes the autotests, please review again.

ikerexxe · 2022-11-29T11:36:41Z

LGTM. Thanks for the patches!

@hallyn any opinion before I merge it?

alejandro-colomar · 2022-12-02T13:33:27Z

libmisc/agetpass.c

+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#include <config.h>


@ikerexxe @cgzones @hallyn Is this include necessary?

I'm not any of the highlighted, but on autotools-based projects that's required yes, so that any of the checks performed by configure apply here, including extensions such as LFS support and similar.

I'm not any of the highlighted,

I forgot to CC you. Good that you were notified anyway :)

but on autotools-based projects that's required yes, so that any of the checks performed by configure apply here, including extensions such as LFS support and similar.

Thanks!

It was a bit unintuitive to me that I'd need to include a header if I wan't using any of its definitions (since the build system usually finds the location of libbsd headers in projects I use, and then it's transparent to me; you can guess I don't use autotools :p).

ikerexxe · 2022-12-05T09:47:11Z

Moving this forward to stop blocking you from other work you are doing.

Thanks for your hard work!

alejandro-colomar · 2022-12-05T10:33:36Z

Moving this forward to stop blocking you from other work you are doing.

Thanks for your hard work!

Thanks! I'll rebase the currently-open PRs.

Cheers,

Alex

hallyn reviewed Sep 27, 2022

View reviewed changes

libmisc/agetpass.c Outdated Show resolved Hide resolved

alejandro-colomar force-pushed the agetpass branch from b0e0ca4 to cc08c6e Compare September 27, 2022 21:43

alejandro-colomar force-pushed the agetpass branch from cc08c6e to 831bbeb Compare September 27, 2022 21:45

alejandro-colomar commented Sep 27, 2022

View reviewed changes

src/sulogin.c Show resolved Hide resolved

alejandro-colomar force-pushed the agetpass branch from 831bbeb to 2e5adec Compare September 27, 2022 22:05

ikerexxe reviewed Sep 28, 2022

View reviewed changes

libmisc/agetpass.c Outdated Show resolved Hide resolved

ikerexxe reviewed Sep 28, 2022

View reviewed changes

src/sulogin.c Show resolved Hide resolved

ikerexxe reviewed Sep 28, 2022

View reviewed changes

alejandro-colomar force-pushed the agetpass branch from 2e5adec to 8186c67 Compare September 28, 2022 16:01

alejandro-colomar force-pushed the agetpass branch from 8186c67 to a720f16 Compare September 28, 2022 17:19

cgzones suggested changes Sep 28, 2022

View reviewed changes

lib/prototypes.h Outdated Show resolved Hide resolved

libmisc/agetpass.c Outdated Show resolved Hide resolved

alejandro-colomar force-pushed the agetpass branch 2 times, most recently from ca1efa1 to 316a15f Compare September 28, 2022 20:43

alejandro-colomar force-pushed the agetpass branch from 316a15f to b7c02bd Compare September 28, 2022 20:59

ikerexxe reviewed Sep 29, 2022

View reviewed changes

libmisc/agetpass.c Outdated Show resolved Hide resolved

libmisc/agetpass.c Outdated Show resolved Hide resolved

ikerexxe reviewed Sep 29, 2022

View reviewed changes

alejandro-colomar force-pushed the agetpass branch from 82c6cc5 to 56b629b Compare September 29, 2022 09:49

alejandro-colomar force-pushed the agetpass branch from 4c1da4e to 97ad87f Compare November 22, 2022 12:44

ikerexxe mentioned this pull request Nov 22, 2022

CI: add libbsd and pkg-config dependencies #596

Merged

alejandro-colomar force-pushed the agetpass branch from 97ad87f to a298bf0 Compare November 22, 2022 18:23

alejandro-colomar force-pushed the agetpass branch 2 times, most recently from 1435d79 to f32ab22 Compare November 22, 2022 19:14

alejandro-colomar force-pushed the agetpass branch from f32ab22 to 1b3be1b Compare November 23, 2022 10:32

github-advanced-security bot found potential problems Nov 23, 2022

View reviewed changes

alejandro-colomar force-pushed the agetpass branch from 4dd00fd to 85aa820 Compare November 23, 2022 12:24

Don't 'else' after a 'noreturn' call

0be6bdf

Signed-off-by: Alejandro Colomar <alx@kernel.org>

alejandro-colomar force-pushed the agetpass branch from 85aa820 to 1abcebf Compare November 28, 2022 17:27

alejandro-colomar and others added 4 commits November 28, 2022 18:28

Hide [[gnu::malloc(deallocator)]] in a macro

2e9f18a

Clang doesn't implement this attribute and reports an error. Work around it by hiding it in a macro that will be empty in clang. Reported-by: Christian Göttsche <cgzones@googlemail.com> Signed-off-by: Alejandro Colomar <alx@kernel.org>

agetpass: Hook into build-system

a04f230

Signed-off-by: Guillem Jover <guillem@hadrons.org> Signed-off-by: Alejandro Colomar <alx@kernel.org>

alejandro-colomar force-pushed the agetpass branch from 1abcebf to a04f230 Compare November 28, 2022 17:42

alejandro-colomar commented Dec 2, 2022

View reviewed changes

ikerexxe merged commit 2a5b881 into shadow-maint:master Dec 5, 2022

alejandro-colomar deleted the agetpass branch December 5, 2022 10:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

agetpass() to replace getpass(3) #573

agetpass() to replace getpass(3) #573

alejandro-colomar commented Sep 27, 2022

hallyn commented Sep 27, 2022

alejandro-colomar commented Sep 27, 2022

alejandro-colomar commented Sep 27, 2022 •

edited

alejandro-colomar commented Sep 27, 2022

alejandro-colomar commented Sep 27, 2022

alejandro-colomar commented Sep 27, 2022 •

edited

ikerexxe left a comment

alejandro-colomar commented Sep 28, 2022 •

edited

alejandro-colomar commented Sep 28, 2022

cgzones left a comment

alejandro-colomar commented Sep 28, 2022 •

edited

alejandro-colomar commented Sep 28, 2022 •

edited

alejandro-colomar commented Sep 28, 2022

alejandro-colomar commented Sep 28, 2022 •

edited

ikerexxe left a comment

alejandro-colomar commented Sep 29, 2022 •

edited

alejandro-colomar commented Nov 22, 2022

ikerexxe commented Nov 22, 2022

alejandro-colomar commented Nov 22, 2022

alejandro-colomar commented Nov 22, 2022

alejandro-colomar commented Nov 23, 2022

alejandro-colomar commented Nov 23, 2022 •

edited

alejandro-colomar commented Nov 28, 2022

alejandro-colomar commented Nov 28, 2022

alejandro-colomar commented Nov 28, 2022

ikerexxe commented Nov 29, 2022

alejandro-colomar Dec 2, 2022

guillemj Dec 2, 2022

alejandro-colomar Dec 2, 2022 •

edited

ikerexxe commented Dec 5, 2022

alejandro-colomar commented Dec 5, 2022

agetpass() to replace getpass(3) #573

agetpass() to replace getpass(3) #573

Conversation

alejandro-colomar commented Sep 27, 2022

hallyn commented Sep 27, 2022

alejandro-colomar commented Sep 27, 2022

alejandro-colomar commented Sep 27, 2022 • edited

alejandro-colomar commented Sep 27, 2022

alejandro-colomar commented Sep 27, 2022

alejandro-colomar commented Sep 27, 2022 • edited

ikerexxe left a comment

Choose a reason for hiding this comment

alejandro-colomar commented Sep 28, 2022 • edited

alejandro-colomar commented Sep 28, 2022

cgzones left a comment

Choose a reason for hiding this comment

alejandro-colomar commented Sep 28, 2022 • edited

alejandro-colomar commented Sep 28, 2022 • edited

alejandro-colomar commented Sep 28, 2022

alejandro-colomar commented Sep 28, 2022 • edited

ikerexxe left a comment

Choose a reason for hiding this comment

alejandro-colomar commented Sep 29, 2022 • edited

alejandro-colomar commented Nov 22, 2022

ikerexxe commented Nov 22, 2022

alejandro-colomar commented Nov 22, 2022

alejandro-colomar commented Nov 22, 2022

alejandro-colomar commented Nov 23, 2022

alejandro-colomar commented Nov 23, 2022 • edited

alejandro-colomar commented Nov 28, 2022

alejandro-colomar commented Nov 28, 2022

alejandro-colomar commented Nov 28, 2022

ikerexxe commented Nov 29, 2022

alejandro-colomar Dec 2, 2022

Choose a reason for hiding this comment

guillemj Dec 2, 2022

Choose a reason for hiding this comment

alejandro-colomar Dec 2, 2022 • edited

Choose a reason for hiding this comment

ikerexxe commented Dec 5, 2022

alejandro-colomar commented Dec 5, 2022

alejandro-colomar commented Sep 27, 2022 •

edited

alejandro-colomar commented Sep 27, 2022 •

edited

alejandro-colomar commented Sep 28, 2022 •

edited

alejandro-colomar commented Sep 28, 2022 •

edited

alejandro-colomar commented Sep 28, 2022 •

edited

alejandro-colomar commented Sep 28, 2022 •

edited

alejandro-colomar commented Sep 29, 2022 •

edited

alejandro-colomar commented Nov 23, 2022 •

edited

alejandro-colomar Dec 2, 2022 •

edited