Created attachment 238198 [details] send a non-null-terminated appl_version to kadmind kadmind's handle_v5() calls krb_recvauth_match_version(), which contains: n = krb5_net_read (context, p_fd, &len, 4); ...; len = ntohl(len); her_appl_version = malloc (len); if (krb5_net_read (context, p_fd, her_appl_version, len) != len || !(*match_appl_version)(match_data, her_appl_version)) { repl = 2; krb5_net_write (context, p_fd, &repl, 1); krb5_set_error_message(context, KRB5_SENDAUTH_BADAPPLVERS, N_("wrong sendauth version (%s)", ""), her_appl_version); The code does not check that the incoming message in her_appl_version is null terminated, which can cause trouble for match_appl_version()'s call to sscanf, and krb5_set_error_message's use of her_apply_version. This is with CURRENT source from today (Nov 20 2022). I've attached a demo. Since there's often a null somewhere soon after the end of the allocated buffer, the problem is only reliably visible with something like valgrind: # /usr/libexec/kadmind --version kadmind (Heimdal 1.5.2) Copyright 1995-2011 Kungliga Tekniska Högskolan Send bug-reports to heimdal-bugs@h5l.org # valgrind /usr/libexec/kadmind --debug & # cc kadmind3a.c # ./a.out ==67648== Memcheck, a memory error detector ==67648== Copyright (C) 2002-2022, and GNU GPL'd, by Julian Seward et al. ==67648== Using Valgrind-3.19.0 and LibVEX; rerun with -h for copyright info ==67648== Command: /usr/libexec/kadmind --debug ==67648== ==67648== Invalid read of size 1 ==67648== at 0x4852EE9: strlen (in /usr/local/libexec/valgrind/vgpreload_memcheck-amd64-freebsd.so) ==67648== by 0x4A596BC: vsscanf (in /lib/libc.so.7) ==67648== by 0x4A4C72C: sscanf (in /lib/libc.so.7) ==67648== by 0x112677: ??? (in /usr/libexec/kadmind) ==67648== by 0x4907BE6: krb5_recvauth_match_version (in /usr/lib/libkrb5.so.11) ==67648== by 0x1114CD: ??? (in /usr/libexec/kadmind) ==67648== by 0x112978: ??? (in /usr/libexec/kadmind) ==67648== by 0x10D16C: ??? (in /usr/libexec/kadmind) ==67648== by 0x4823007: ??? ==67648== Address 0x5b463b6 is 0 bytes after a block of size 150 alloc'd ==67648== at 0x484C8A4: malloc (in /usr/local/libexec/valgrind/vgpreload_memcheck-amd64-freebsd.so) ==67648== by 0x4907BA9: krb5_recvauth_match_version (in /usr/lib/libkrb5.so.11) ==67648== by 0x1114CD: ??? (in /usr/libexec/kadmind) ==67648== by 0x112978: ??? (in /usr/libexec/kadmind) ==67648== by 0x10D16C: ??? (in /usr/libexec/kadmind) ==67648== by 0x4823007: ???
It appears that upstream Heimdal 7.8.0 suffers the same bug. The simplest solution would be to strlcpy() the string to a temporary bufer in match_appl_version(), do the sscanf(), and discard the temporary buffer. I think this should also be reported to our friends at Heimdal.
Created attachment 238215 [details] Ensure terminating NUL is in the string Making sure that only sscanf() receives a terminating NUL fails to consider that other consumers of her_appl_version (kf) will also encounter the same problem. This patch ensures that there is always a terminating NUL. Additionally, if bytes read is zero the version string is not processed. (Though it could be argued that this could be a second commit.) It appears that upstream Heimdal 7.8.0 has the same problem.
Adding hps@, MAINTAINER of security/heimdal port.
Created attachment 238233 [details] Same patch for both Heimdal ports The same patch applied to both heimdal ports.
Created attachment 238240 [details] Ensure terminating NUL is in the string This patch fixes a -Wparentheses error in the previous version.
Created attachment 238241 [details] Same patch for both Heimdal ports. This patch fixes a -Wparentheses error in the previous version.
Patch results: bob# valgrind /usr/libexec/kadmind --debug ==87610== Memcheck, a memory error detector ==87610== Copyright (C) 2002-2022, and GNU GPL'd, by Julian Seward et al. ==87610== Using Valgrind-3.20.0 and LibVEX; rerun with -h for copyright info ==87610== Command: /usr/libexec/kadmind --debug ==87610== ==87610== ==87610== HEAP SUMMARY: ==87610== in use at exit: 11,344 bytes in 159 blocks ==87610== total heap usage: 1,052 allocs, 893 frees, 273,855 bytes allocated ==87610== ==87610== LEAK SUMMARY: ==87610== definitely lost: 0 bytes in 0 blocks ==87610== indirectly lost: 0 bytes in 0 blocks ==87610== possibly lost: 437 bytes in 6 blocks ==87610== still reachable: 10,907 bytes in 153 blocks ==87610== suppressed: 0 bytes in 0 blocks ==87610== Rerun with --leak-check=full to see details of leaked memory ==87610== ==87610== For lists of detected and suppressed errors, rerun with: -s ==87610== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 6 from 1) bob#
Review posted to https://reviews.freebsd.org/D37471.
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=d7e8666ffb9967a92709a2d2ded4d31568ab1473 commit d7e8666ffb9967a92709a2d2ded4d31568ab1473 Author: Cy Schubert <cy@FreeBSD.org> AuthorDate: 2022-11-21 15:33:08 +0000 Commit: Cy Schubert <cy@FreeBSD.org> CommitDate: 2022-11-24 17:21:13 +0000 heimdal: The version string must always contain a terminating NUL Should the sender send a string without a terminating NUL, ensure that the NUL terminates the string regardless. And while at it only process the version string when bytes are returned. PR: 267884 Reported by: Robert Morris <rtm@lcs.mit.edu> MFC after: 3 days Differential Revision: https://reviews.freebsd.org/D37471 crypto/heimdal/lib/krb5/recvauth.c | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-)
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/ports/commit/?id=d831a2fe480fe02126bd5b9aba5569c5e69f1034 commit d831a2fe480fe02126bd5b9aba5569c5e69f1034 Author: Cy Schubert <cy@FreeBSD.org> AuthorDate: 2022-11-21 22:41:13 +0000 Commit: Cy Schubert <cy@FreeBSD.org> CommitDate: 2022-11-24 17:22:01 +0000 security/heimdal*: The version string must always contain a terminating NUL Should the sender send a string without a terminating NUL, ensure that the NUL terminates the string regardless. And while at it only process the version string when bytes are returned. PR: 267884 Reported by: Robert Morris <rtm@lcs.mit.edu> MFH: 2022Q4 security/heimdal-devel/Makefile | 2 +- .../files/patch-lib_krb5_recvauth.c (new) | 42 ++++++++++++++++++++++ security/heimdal/Makefile | 2 +- .../heimdal/files/patch-lib_krb5_recvauth.c (new) | 42 ++++++++++++++++++++++ 4 files changed, 86 insertions(+), 2 deletions(-)
A commit in branch stable/13 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=e7dcb18d51088654948840a66ad3864a66f1cb96 commit e7dcb18d51088654948840a66ad3864a66f1cb96 Author: Cy Schubert <cy@FreeBSD.org> AuthorDate: 2022-11-21 15:33:08 +0000 Commit: Cy Schubert <cy@FreeBSD.org> CommitDate: 2022-12-01 14:25:10 +0000 heimdal: The version string must always contain a terminating NUL Should the sender send a string without a terminating NUL, ensure that the NUL terminates the string regardless. And while at it only process the version string when bytes are returned. PR: 267884 Reported by: Robert Morris <rtm@lcs.mit.edu> Differential Revision: https://reviews.freebsd.org/D37471 (cherry picked from commit d7e8666ffb9967a92709a2d2ded4d31568ab1473) crypto/heimdal/lib/krb5/recvauth.c | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-)
A commit in branch stable/12 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=8d3c6e82cdcac5fa31836df6d3d067efd2e0e3bc commit 8d3c6e82cdcac5fa31836df6d3d067efd2e0e3bc Author: Cy Schubert <cy@FreeBSD.org> AuthorDate: 2022-11-21 15:33:08 +0000 Commit: Cy Schubert <cy@FreeBSD.org> CommitDate: 2022-12-01 14:25:52 +0000 heimdal: The version string must always contain a terminating NUL Should the sender send a string without a terminating NUL, ensure that the NUL terminates the string regardless. And while at it only process the version string when bytes are returned. PR: 267884 Reported by: Robert Morris <rtm@lcs.mit.edu> Differential Revision: https://reviews.freebsd.org/D37471 (cherry picked from commit d7e8666ffb9967a92709a2d2ded4d31568ab1473) crypto/heimdal/lib/krb5/recvauth.c | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-)
fixed
A commit in branch 2022Q4 references this bug: URL: https://cgit.FreeBSD.org/ports/commit/?id=a8c5dca736cdaed68738c39c52a3e0e158951b28 commit a8c5dca736cdaed68738c39c52a3e0e158951b28 Author: Cy Schubert <cy@FreeBSD.org> AuthorDate: 2022-11-21 22:41:13 +0000 Commit: Cy Schubert <cy@FreeBSD.org> CommitDate: 2022-12-05 20:06:14 +0000 security/heimdal*: The version string must always contain a terminating NUL Should the sender send a string without a terminating NUL, ensure that the NUL terminates the string regardless. And while at it only process the version string when bytes are returned. PR: 267884 Reported by: Robert Morris <rtm@lcs.mit.edu> (cherry picked from commit d831a2fe480fe02126bd5b9aba5569c5e69f1034) security/heimdal-devel/Makefile | 2 +- .../files/patch-lib_krb5_recvauth.c (new) | 42 ++++++++++++++++++++++ security/heimdal/Makefile | 2 +- .../heimdal/files/patch-lib_krb5_recvauth.c (new) | 42 ++++++++++++++++++++++ 4 files changed, 86 insertions(+), 2 deletions(-)