From 3eb63ea48c0e408bce381e94f47e10ed578a8b2c Mon Sep 17 00:00:00 2001 From: "Dmitry V. Levin" Date: Fri, 24 Apr 2020 03:27:12 +0000 Subject: pam_issue: fix potential read out of bounds Reported by gcc-10 -Warray-bounds: In file included from /usr/include/string.h:494, from modules/pam_issue/pam_issue.c:19: In function 'strncat', inlined from 'read_issue_quoted' at modules/pam_issue/pam_issue.c:197:3: /usr/include/x86_64-linux-gnu/bits/string_fortified.h:136:10: error: '__builtin___strncat_chk' offset [260, 389] from the object at 'uts' is out of the bounds of referenced subobject 'version' with type 'char[65]' at offset 195 [-Werror=array-bounds] 136 | return __builtin___strncat_chk (__dest, __src, __len, __bos (__dest)); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ In file included from modules/pam_issue/pam_issue.c:26: modules/pam_issue/pam_issue.c: In function 'read_issue_quoted': /usr/include/x86_64-linux-gnu/sys/utsname.h:59:10: note: subobject 'version' declared here 59 | char version[_UTSNAME_VERSION_LENGTH]; | ^~~~~~~ In file included from /usr/include/string.h:494, from modules/pam_issue/pam_issue.c:19: In function 'strncat', inlined from 'read_issue_quoted' at modules/pam_issue/pam_issue.c:188:3: /usr/include/x86_64-linux-gnu/bits/string_fortified.h:136:10: error: '__builtin___strncat_chk' offset [65, 389] from the object at 'uts' is out of the bounds of referenced subobject 'sysname' with type 'char[65]' at offset 0 [-Werror=array-bounds] 136 | return __builtin___strncat_chk (__dest, __src, __len, __bos (__dest)); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ In file included from modules/pam_issue/pam_issue.c:26: modules/pam_issue/pam_issue.c: In function 'read_issue_quoted': /usr/include/x86_64-linux-gnu/sys/utsname.h:51:10: note: subobject 'sysname' declared here 51 | char sysname[_UTSNAME_SYSNAME_LENGTH]; | ^~~~~~~ In file included from /usr/include/string.h:494, from modules/pam_issue/pam_issue.c:19: In function 'strncat', inlined from 'read_issue_quoted' at modules/pam_issue/pam_issue.c:194:3: /usr/include/x86_64-linux-gnu/bits/string_fortified.h:136:10: error: '__builtin___strncat_chk' offset [195, 389] from the object at 'uts' is out of the bounds of referenced subobject 'release' with type 'char[65]' at offset 130 [-Werror=array-bounds] 136 | return __builtin___strncat_chk (__dest, __src, __len, __bos (__dest)); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ In file included from modules/pam_issue/pam_issue.c:26: modules/pam_issue/pam_issue.c: In function 'read_issue_quoted': /usr/include/x86_64-linux-gnu/sys/utsname.h:57:10: note: subobject 'release' declared here 57 | char release[_UTSNAME_RELEASE_LENGTH]; | ^~~~~~~ In file included from /usr/include/string.h:494, from modules/pam_issue/pam_issue.c:19: In function 'strncat', inlined from 'read_issue_quoted' at modules/pam_issue/pam_issue.c:191:3: /usr/include/x86_64-linux-gnu/bits/string_fortified.h:136:10: error: '__builtin___strncat_chk' offset [130, 389] from the object at 'uts' is out of the bounds of referenced subobject 'nodename' with type 'char[65]' at offset 65 [-Werror=array-bounds] 136 | return __builtin___strncat_chk (__dest, __src, __len, __bos (__dest)); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ In file included from modules/pam_issue/pam_issue.c:26: modules/pam_issue/pam_issue.c: In function 'read_issue_quoted': /usr/include/x86_64-linux-gnu/sys/utsname.h:54:10: note: subobject 'nodename' declared here 54 | char nodename[_UTSNAME_NODENAME_LENGTH]; | ^~~~~~~~ In file included from /usr/include/string.h:494, from modules/pam_issue/pam_issue.c:19: In function 'strncat', inlined from 'read_issue_quoted' at modules/pam_issue/pam_issue.c:200:3: /usr/include/x86_64-linux-gnu/bits/string_fortified.h:136:10: error: '__builtin___strncat_chk' offset [325, 389] from the object at 'uts' is out of the bounds of referenced subobject 'machine' with type 'char[65]' at offset 260 [-Werror=array-bounds] 136 | return __builtin___strncat_chk (__dest, __src, __len, __bos (__dest)); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ In file included from modules/pam_issue/pam_issue.c:26: modules/pam_issue/pam_issue.c: In function 'read_issue_quoted': /usr/include/x86_64-linux-gnu/sys/utsname.h:62:10: note: subobject 'machine' declared here 62 | char machine[_UTSNAME_MACHINE_LENGTH]; | ^~~~~~~ * modules/pam_issue/pam_issue.c (read_issue_quoted): Rewrite to avoid strncat from potentially not null-terminated string buffer fields of struct utsname. --- modules/pam_issue/pam_issue.c | 50 +++++++++++++++++++++++++------------------ 1 file changed, 29 insertions(+), 21 deletions(-) (limited to 'modules/pam_issue') diff --git a/modules/pam_issue/pam_issue.c b/modules/pam_issue/pam_issue.c index 49cbad4c..8a74ce03 100644 --- a/modules/pam_issue/pam_issue.c +++ b/modules/pam_issue/pam_issue.c @@ -163,6 +163,7 @@ read_issue_quoted(pam_handle_t *pamh, FILE *fp, char **prompt) { int c; size_t size = 1024; + size_t issue_len = 0; char *issue; struct utsname uts; @@ -173,42 +174,41 @@ read_issue_quoted(pam_handle_t *pamh, FILE *fp, char **prompt) return PAM_BUF_ERR; } - issue[0] = '\0'; (void) uname(&uts); while ((c = getc(fp)) != EOF) { - char buf[1024]; + const char *src = NULL; + size_t len = 0; + char buf[1024] = ""; - buf[0] = '\0'; if (c == '\\') { if ((c = getc(fp)) == EOF) break; switch (c) { case 's': - strncat(buf, uts.sysname, sizeof(buf) - 1); + src = uts.sysname; + len = strnlen(uts.sysname, sizeof(uts.sysname)); break; case 'n': - strncat(buf, uts.nodename, sizeof(buf) - 1); + src = uts.nodename; + len = strnlen(uts.nodename, sizeof(uts.nodename)); break; case 'r': - strncat(buf, uts.release, sizeof(buf) - 1); + src = uts.release; + len = strnlen(uts.release, sizeof(uts.release)); break; case 'v': - strncat(buf, uts.version, sizeof(buf) - 1); + src = uts.version; + len = strnlen(uts.version, sizeof(uts.version)); break; case 'm': - strncat(buf, uts.machine, sizeof(buf) - 1); + src = uts.machine; + len = strnlen(uts.machine, sizeof(uts.machine)); break; case 'o': #ifdef HAVE_GETDOMAINNAME - { - char domainname[256]; - - if (getdomainname(domainname, sizeof(domainname)) >= 0) { - domainname[sizeof(domainname)-1] = '\0'; - strncat(buf, domainname, sizeof(buf) - 1); - } - } + if (getdomainname(buf, sizeof(buf)) >= 0) + buf[sizeof(buf) - 1] = '\0'; #endif break; case 'd': @@ -243,7 +243,8 @@ read_issue_quoted(pam_handle_t *pamh, FILE *fp, char **prompt) const char *str = pam_str_skip_prefix(ttyn, "/dev/"); if (str != NULL) ttyn = str; - strncat(buf, ttyn, sizeof(buf) - 1); + src = ttyn; + len = strlen(ttyn); } } break; @@ -272,20 +273,27 @@ read_issue_quoted(pam_handle_t *pamh, FILE *fp, char **prompt) buf[0] = c; buf[1] = '\0'; } - if ((strlen(issue) + strlen(buf)) + 1 > size) { + if (src == NULL) { + src = buf; + len = strlen(buf); + } + if (issue_len + len + 1 > size) { char *new_issue; - size += strlen(buf) + 1; - new_issue = (char *) realloc (issue, size); + size += len + 1; + new_issue = realloc (issue, size); if (new_issue == NULL) { _pam_drop(issue); return PAM_BUF_ERR; } issue = new_issue; } - strcat(issue, buf); + memcpy(issue + issue_len, src, len); + issue_len += len; } + issue[issue_len] = '\0'; + if (ferror(fp)) { pam_syslog(pamh, LOG_ERR, "read error: %m"); _pam_drop(issue); -- cgit v1.2.3