Fix misuse of PAM_PROMPT_ECHO_OFF in systemd-homed

Previously pam_systemd_home.so was relying on `PAM_PROMPT_ECHO_OFF` to display error messages to the user and also display the next prompt. `PAM_PROMPT_ECHO_OFF` was never meant as a way to convey information to the user, and following the example set in pam_unix.so you can see that it's meant to _only_ display the prompt. Details about why the authentication failed should be done in a `PAM_ERROR_MSG` before displaying a short prompt as per usual using `PAM_PROMPT_ECHO_OFF`.
Merge pull request #15762 from keszybz/gcc-10-build
2020-05-12 00:27:08 +02:00 · 2020-05-11 19:43:40 +02:00 · 2020-05-11 13:57:11 +02:00 · 2020-05-09 09:10:25 +02:00
4 changed files with 38 additions and 16 deletions
--- a/meson.build
+++ b/meson.build
@ -411,6 +411,9 @@ add_project_arguments(cc.get_supported_arguments(basic_disabled_warnings), langu
 add_project_arguments(cc.get_supported_arguments(possible_cc_flags), language : 'c')
 add_project_link_arguments(cc.get_supported_link_arguments(possible_link_flags), language : 'c')

+have = cc.has_argument('-Wzero-length-bounds')
+conf.set10('HAVE_ZERO_LENGTH_BOUNDS', have)
+
 if cc.compiles('''
   #include <time.h>
   #include <inttypes.h>
--- a/src/core/path.c
+++ b/src/core/path.c
@ -223,11 +223,10 @@ static void path_spec_mkdir(PathSpec *s, mode_t mode) {
 }

 static void path_spec_dump(PathSpec *s, FILE *f, const char *prefix) {
-        fprintf(f,
-                "%s%s: %s\n",
-                prefix,
-                path_type_to_string(s->type),
-                s->path);
+        const char *type;
+
+        assert_se(type = path_type_to_string(s->type));
+        fprintf(f, "%s%s: %s\n", prefix, type, s->path);
 }

 void path_spec_done(PathSpec *s) {
@ -607,14 +606,16 @@ static int path_serialize(Unit *u, FILE *f, FDSet *fds) {
        (void) serialize_item(f, "result", path_result_to_string(p->result));

        LIST_FOREACH(spec, s, p->specs) {
+                const char *type;
                _cleanup_free_ char *escaped = NULL;

                escaped = cescape(s->path);
                if (!escaped)
                        return log_oom();

+                assert_se(type = path_type_to_string(s->type));
                (void) serialize_item_format(f, "path-spec", "%s %i %s",
-                                             path_type_to_string(s->type),
+                                             type,
                                             s->previous_exists,
                                             s->path);
        }
--- a/src/home/pam_systemd_home.c
+++ b/src/home/pam_systemd_home.c
@ -250,8 +250,10 @@ static int handle_generic_user_record_error(

                if (strv_isempty(secret->password))
                        r = pam_prompt(handle, PAM_PROMPT_ECHO_OFF, &newp, "Password: ");
-                else
-                        r = pam_prompt(handle, PAM_PROMPT_ECHO_OFF, &newp, "Password incorrect or not sufficient for authentication of user %s, please try again: ", user_name);
+                else {
+                        (void) pam_prompt(handle, PAM_ERROR_MSG, NULL, "Password incorrect or not sufficient for authentication of user %s.", user_name);
+                        r = pam_prompt(handle, PAM_PROMPT_ECHO_OFF, &newp, "Sorry, try again: ");
+                }
                if (r != PAM_SUCCESS)
                        return PAM_CONV_ERR; /* no logging here */

@ -269,10 +271,13 @@ static int handle_generic_user_record_error(
        } else if (sd_bus_error_has_name(error, BUS_ERROR_BAD_PASSWORD_AND_NO_TOKEN)) {
                _cleanup_(erase_and_freep) char *newp = NULL;

-                if (strv_isempty(secret->password))
-                        r = pam_prompt(handle, PAM_PROMPT_ECHO_OFF, &newp, "Security token of user %s not inserted, please enter password: ", user_name);
-                else
-                        r = pam_prompt(handle, PAM_PROMPT_ECHO_OFF, &newp, "Password incorrect or not sufficient, and configured security token of user %s not inserted, please enter password: ", user_name);
+                if (strv_isempty(secret->password)) {
+                        (void) pam_prompt(handle, PAM_ERROR_MSG, NULL, "Security token of user %s not inserted.", user_name);
+                        r = pam_prompt(handle, PAM_PROMPT_ECHO_OFF, &newp, "Try again with password: ");
+                } else {
+                        (void) pam_prompt(handle, PAM_ERROR_MSG, NULL, "Password incorrect or not sufficient, and configured security token of user %s not inserted.", user_name);
+                        r = pam_prompt(handle, PAM_PROMPT_ECHO_OFF, &newp, "Try again with password: ");
+                }
                if (r != PAM_SUCCESS)
                        return PAM_CONV_ERR; /* no logging here */

@ -290,7 +295,7 @@ static int handle_generic_user_record_error(
        } else if (sd_bus_error_has_name(error, BUS_ERROR_TOKEN_PIN_NEEDED)) {
                _cleanup_(erase_and_freep) char *newp = NULL;

-                r = pam_prompt(handle, PAM_PROMPT_ECHO_OFF, &newp, "Please enter security token PIN: ");
+                r = pam_prompt(handle, PAM_PROMPT_ECHO_OFF, &newp, "Security token PIN: ");
                if (r != PAM_SUCCESS)
                        return PAM_CONV_ERR; /* no logging here */

@ -318,7 +323,8 @@ static int handle_generic_user_record_error(
        } else if (sd_bus_error_has_name(error, BUS_ERROR_TOKEN_BAD_PIN)) {
                _cleanup_(erase_and_freep) char *newp = NULL;

-                r = pam_prompt(handle, PAM_PROMPT_ECHO_OFF, &newp, "Security token PIN incorrect, please enter PIN for security token of user %s again: ", user_name);
+                (void) pam_prompt(handle, PAM_ERROR_MSG, NULL, "Security token PIN incorrect for user %s.", user_name);
+                r = pam_prompt(handle, PAM_PROMPT_ECHO_OFF, &newp, "Sorry, retry security token PIN: ");
                if (r != PAM_SUCCESS)
                        return PAM_CONV_ERR; /* no logging here */

@ -336,7 +342,8 @@ static int handle_generic_user_record_error(
        } else if (sd_bus_error_has_name(error, BUS_ERROR_TOKEN_BAD_PIN_FEW_TRIES_LEFT)) {
                _cleanup_(erase_and_freep) char *newp = NULL;

-                r = pam_prompt(handle, PAM_PROMPT_ECHO_OFF, &newp, "Security token PIN incorrect (only a few tries left!), please enter PIN for security token of user %s again: ", user_name);
+                (void) pam_prompt(handle, PAM_ERROR_MSG, NULL, "Security token PIN of user %s incorrect (only a few tries left!)", user_name);
+                r = pam_prompt(handle, PAM_PROMPT_ECHO_OFF, &newp, "Sorry, retry security token PIN: ");
                if (r != PAM_SUCCESS)
                        return PAM_CONV_ERR; /* no logging here */

@ -354,7 +361,8 @@ static int handle_generic_user_record_error(
        } else if (sd_bus_error_has_name(error, BUS_ERROR_TOKEN_BAD_PIN_ONE_TRY_LEFT)) {
                _cleanup_(erase_and_freep) char *newp = NULL;

-                r = pam_prompt(handle, PAM_PROMPT_ECHO_OFF, &newp, "Security token PIN incorrect (only one try left!), please enter PIN for security token of user %s again: ", user_name);
+                (void) pam_prompt(handle, PAM_ERROR_MSG, NULL, "Security token PIN of user %s incorrect (only one try left!)", user_name);
+                r = pam_prompt(handle, PAM_PROMPT_ECHO_OFF, &newp, "Sorry, retry security token PIN: ");
                if (r != PAM_SUCCESS)
                        return PAM_CONV_ERR; /* no logging here */

--- a/src/shared/ethtool-util.c
+++ b/src/shared/ethtool-util.c
@ -256,8 +256,13 @@ int ethtool_get_permanent_macaddr(int *ethtool_fd, const char *ifname, struct et
        if (epaddr.addr.size != 6)
                return -EOPNOTSUPP;

+#pragma GCC diagnostic push
+#if HAVE_ZERO_LENGTH_BOUNDS
+#  pragma GCC diagnostic ignored "-Wzero-length-bounds"
+#endif
        for (size_t i = 0; i < epaddr.addr.size; i++)
                ret->ether_addr_octet[i] = epaddr.addr.data[i];
+#pragma GCC diagnostic pop

        return 0;
 }
@ -485,7 +490,12 @@ static int get_stringset(int ethtool_fd, struct ifreq *ifr, int stringset_id, st
        if (!buffer.info.sset_mask)
                return -EINVAL;

+#pragma GCC diagnostic push
+#if HAVE_ZERO_LENGTH_BOUNDS
+#  pragma GCC diagnostic ignored "-Wzero-length-bounds"
+#endif
        len = buffer.info.data[0];
+#pragma GCC diagnostic pop

        strings = malloc0(sizeof(struct ethtool_gstrings) + len * ETH_GSTRING_LEN);
        if (!strings)
Author	SHA1	Message	Date
Joel Shapiro	d423294394	Fix misuse of PAM_PROMPT_ECHO_OFF in systemd-homed Previously pam_systemd_home.so was relying on `PAM_PROMPT_ECHO_OFF` to display error messages to the user and also display the next prompt. `PAM_PROMPT_ECHO_OFF` was never meant as a way to convey information to the user, and following the example set in pam_unix.so you can see that it's meant to _only_ display the prompt. Details about why the authentication failed should be done in a `PAM_ERROR_MSG` before displaying a short prompt as per usual using `PAM_PROMPT_ECHO_OFF`.	2020-05-12 00:27:08 +02:00
Zbigniew Jędrzejewski-Szmek	9b107000ab	Merge pull request #15762 from keszybz/gcc-10-build Fix build with -O3 with gcc 10	2020-05-11 19:43:40 +02:00
Zbigniew Jędrzejewski-Szmek	94c0c5b7ea	shared/ethtool-util: hush gcc warnings about array bounds [127/1355] Compiling C object 'src/shared/5afaae1@@systemd-shared-245@sta/ethtool-util.c.o' ../src/shared/ethtool-util.c: In function ‘ethtool_get_permanent_macaddr’: ../src/shared/ethtool-util.c:260:60: warning: array subscript 5 is outside the bounds of an interior zero-length array ‘__u8[0]’ {aka ‘unsigned char[]’} [-Wzero-length-bounds] 260 \| ret->ether_addr_octet[i] = epaddr.addr.data[i]; \| ~~~~~~~~~~~~~~~~^~~ In file included from ../src/shared/ethtool-util.c:5: ../src/shared/linux/ethtool.h:704:7: note: while referencing ‘data’ 704 \| __u8 data[0]; \| ^~~~ ../src/shared/ethtool-util.c: In function ‘ethtool_set_features’: ../src/shared/ethtool-util.c:488:31: warning: array subscript 0 is outside the bounds of an interior zero-length array ‘__u32[0]’ {aka ‘unsigned int[]’} [-Wzero-length-bounds] 488 \| len = buffer.info.data[0]; \| ~~~~~~~~~~~~~~~~^~~ In file included from ../src/shared/ethtool-util.c:5: ../src/shared/linux/ethtool.h:631:8: note: while referencing ‘data’ 631 \| __u32 data[0]; \| ^~~~ The kernel should not define the length of the array, but it does. We can't fix that, so let's use a cast to avoid the warning. For https://github.com/systemd/systemd/issues/6119#issuecomment-626073743. v2: - use #pragma instead of a cast. It seems the cast only works in some cases, and gcc is "smart" enough to see beyond the cast. Unfortunately clang does not support this warning, so we need to do a config check whether to try to suppress.	2020-05-11 13:57:11 +02:00
Zbigniew Jędrzejewski-Szmek	23450c897d	core: fix compilation with gcc -O3 ../src/core/path.c: In function ‘path_serialize’: ../src/core/path.c:616:24: error: ‘%s’ directive argument is null [-Werror=format-overflow=] 616 \| (void) serialize_item_format(f, "path-spec", "%s %%i %%s", \| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 617 \| path_type_to_string(s->type) //, \| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 618 \| // s->previous_exists, \| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 619 \| // s->path \| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 620 \| ); \| ~ In function ‘path_spec_dump’, inlined from ‘path_dump’ at ../src/core/path.c:392:17: ../src/core/path.c:226:9: error: ‘%s’ directive argument is null [-Werror=format-overflow=] 226 \| fprintf(f, \| ^~~~~~~~~~ 227 \| "%s%s: %s\n", \| ~~~~~~~~~~~~~ 228 \| prefix, \| ~~~~~~~ 229 \| path_type_to_string(s->type), \| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 230 \| s->path); \| ~~~~~~~~ s->type should be valid here, so let's just add an assert. For https://github.com/systemd/systemd/issues/6119#issuecomment-626073743.	2020-05-09 09:10:25 +02:00