Bug 267972

Summary: kadmind can use uninitialized ent.tl_data...tl_data_contents and tl_data_length
Product: Base System Reporter: Robert Morris <rtm>
Component: binAssignee: Cy Schubert <cy>
Status: Closed FIXED    
Severity: Affects Some People CC: cy, emaste, philip
Priority: ---    
Version: CURRENT   
Hardware: Any   
OS: Any   
Bug Depends on: 267944    
Bug Blocks:    
Attachments:
Description Flags
crash kadmind with a short kadm_modify message none

Description Robert Morris 2022-11-24 18:29:58 UTC
Created attachment 238312 [details]
crash kadmind with a short kadm_modify message

If a client kadm_modify message ends unexpectedly early during
KADM5_TL_DATA, krb5_ret_data() can return (due to error) before
setting data->size of data->length. But the call from
kadm5_ret_tl_data() doesn't check for an error, and the surrounding
call from ret_principal_ent() for KADM5_TL_DATA doesn't check for an
error either. So list elements in ent.tl_data (princ->tl_data) may
contain uninitialized junk.

I've attached a demo. It expects to be run with tickets. Maybe some
previous bugs have to be fixed in order for kadmind to get as far as
this one.

# cc kadmind16a.c -lkrb5
# ./a.out

A backtrace from kadmin:

#0  memset (xdst=0x17e4ffb480, c=0, len=18446744073709541600)
#1  0x00000017d6b6bff2 in kadm5_free_principal_ent (
    server_handle=<optimized out>, princ=0x17d5c2f420)
    at /usr/rtm/symbsd/src/crypto/heimdal/lib/kadm5/free.c:73
#2  0x0000001754e3c340 in kadmind_dispatch (kadm_handlep=0x17e4fd44c0, 
    initial=0, in=<optimized out>, out=0x17d5c2f5f8)
    at /usr/rtm/symbsd/src/crypto/heimdal/kadmin/server.c:161
#3  0x0000001754e3bf9e in v5_loop (contextp=<optimized out>, 
    ac=<optimized out>, initial=<optimized out>, kadm_handlep=<optimized out>, 
    fd=<optimized out>)
    at /usr/rtm/symbsd/src/crypto/heimdal/kadmin/server.c:459
#4  0x0000001754e3be72 in handle_v5 (contextp=0x17e4f8ee10, 
    keytab=<optimized out>, fd=<optimized out>)
    at /usr/rtm/symbsd/src/crypto/heimdal/kadmin/server.c:551
#5  0x0000001754e3bd7a in kadmind_loop (contextp=0x17e4f8ee10, 
    keytab=0x17e4fad330, sock=<optimized out>)
    at /usr/rtm/symbsd/src/crypto/heimdal/kadmin/server.c:579
#6  0x0000001754e3ccb2 in main (argc=<optimized out>, argv=<optimized out>)
    at /usr/rtm/symbsd/src/crypto/heimdal/kadmin/kadmind.c:202
Comment 1 Cy Schubert freebsd_committer freebsd_triage 2022-11-25 23:13:35 UTC
I'll take this.
Comment 2 Cy Schubert freebsd_committer freebsd_triage 2022-11-26 00:06:05 UTC
This will need more testing because I get the following in the sandbox jail used to test these kadmind problems.

bob# /usr/libexec/kadmind --debug
2022-11-25T15:39:16 MODIFY: Invalid argument
bob# 

One of the previous patches may have solved this. I'll try reverting them (in a branch of course) to see if I can reproduce this problem.
Comment 3 Cy Schubert freebsd_committer freebsd_triage 2022-11-26 15:40:35 UTC
PR/267944 also fixes this PR.
Comment 4 commit-hook freebsd_committer freebsd_triage 2022-11-27 02:44:26 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=e13150e28c93d9e74f419dcd17d2e2bad41715ad

commit e13150e28c93d9e74f419dcd17d2e2bad41715ad
Author:     Cy Schubert <cy@FreeBSD.org>
AuthorDate: 2022-11-25 23:29:14 +0000
Commit:     Cy Schubert <cy@FreeBSD.org>
CommitDate: 2022-11-27 02:41:51 +0000

    heimdal: Fix uninitialized pointer dereference

    krb5_ret_preincipal() returns a non-zero return code when
    a garbage principal is passed to it. Unfortunately ret_principal_ent()
    does not check the return code, with garbage pointing to what would
    have been the principal. This results in a segfault when free() is
    called.

    PR:             267944, 267972
    Reported by:    Robert Morris <rtm@lcs.mit.edu>
    MFC after:      3 days

 crypto/heimdal/lib/kadm5/marshall.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)
Comment 5 commit-hook freebsd_committer freebsd_triage 2022-11-27 02:44:30 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=780f663df3a0fc30da5f0680c128087b1a05ea40

commit 780f663df3a0fc30da5f0680c128087b1a05ea40
Author:     Cy Schubert <cy@FreeBSD.org>
AuthorDate: 2022-11-26 16:48:51 +0000
Commit:     Cy Schubert <cy@FreeBSD.org>
CommitDate: 2022-11-27 02:41:51 +0000

    heimdal: Add missing kadm5 error checks

    Generally obtained from upstream 655c057769f56bd8cdb7d16e93f1e7a7cb260342.

    PR:             267944, 267972
    Obtained from:  Heimdal commit 655c057769f56bd8cdb7d16e93f1e7a7cb260342
    MFC after:      3 days

 crypto/heimdal/lib/kadm5/marshall.c | 185 ++++++++++++++++++++----------------
 1 file changed, 105 insertions(+), 80 deletions(-)
Comment 6 commit-hook freebsd_committer freebsd_triage 2022-12-01 14:25:42 UTC
A commit in branch stable/13 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=0ae2fac599455cfef3348a52483d1d0c01ae1908

commit 0ae2fac599455cfef3348a52483d1d0c01ae1908
Author:     Cy Schubert <cy@FreeBSD.org>
AuthorDate: 2022-11-26 16:48:51 +0000
Commit:     Cy Schubert <cy@FreeBSD.org>
CommitDate: 2022-12-01 14:25:10 +0000

    heimdal: Add missing kadm5 error checks

    Generally obtained from upstream 655c057769f56bd8cdb7d16e93f1e7a7cb260342.

    PR:             267944, 267972
    Obtained from:  Heimdal commit 655c057769f56bd8cdb7d16e93f1e7a7cb260342

    (cherry picked from commit 780f663df3a0fc30da5f0680c128087b1a05ea40)

 crypto/heimdal/lib/kadm5/marshall.c | 185 ++++++++++++++++++++----------------
 1 file changed, 105 insertions(+), 80 deletions(-)
Comment 7 commit-hook freebsd_committer freebsd_triage 2022-12-01 14:25:48 UTC
A commit in branch stable/13 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=fed526b0af2bd709f8d187f0c7529d710d3a1bb0

commit fed526b0af2bd709f8d187f0c7529d710d3a1bb0
Author:     Cy Schubert <cy@FreeBSD.org>
AuthorDate: 2022-11-25 23:29:14 +0000
Commit:     Cy Schubert <cy@FreeBSD.org>
CommitDate: 2022-12-01 14:25:10 +0000

    heimdal: Fix uninitialized pointer dereference

    krb5_ret_preincipal() returns a non-zero return code when
    a garbage principal is passed to it. Unfortunately ret_principal_ent()
    does not check the return code, with garbage pointing to what would
    have been the principal. This results in a segfault when free() is
    called.

    PR:             267944, 267972
    Reported by:    Robert Morris <rtm@lcs.mit.edu>

    (cherry picked from commit e13150e28c93d9e74f419dcd17d2e2bad41715ad)

 crypto/heimdal/lib/kadm5/marshall.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)
Comment 8 commit-hook freebsd_committer freebsd_triage 2022-12-01 14:26:50 UTC
A commit in branch stable/12 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=92586930433d215a606a9a50bfaa5358e4540acc

commit 92586930433d215a606a9a50bfaa5358e4540acc
Author:     Cy Schubert <cy@FreeBSD.org>
AuthorDate: 2022-11-25 23:29:14 +0000
Commit:     Cy Schubert <cy@FreeBSD.org>
CommitDate: 2022-12-01 14:25:52 +0000

    heimdal: Fix uninitialized pointer dereference

    krb5_ret_preincipal() returns a non-zero return code when
    a garbage principal is passed to it. Unfortunately ret_principal_ent()
    does not check the return code, with garbage pointing to what would
    have been the principal. This results in a segfault when free() is
    called.

    PR:             267944, 267972
    Reported by:    Robert Morris <rtm@lcs.mit.edu>

    (cherry picked from commit e13150e28c93d9e74f419dcd17d2e2bad41715ad)

 crypto/heimdal/lib/kadm5/marshall.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)
Comment 9 commit-hook freebsd_committer freebsd_triage 2022-12-01 14:26:57 UTC
A commit in branch stable/12 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=f3dc873719d4bdf7341c00097643843562e3114c

commit f3dc873719d4bdf7341c00097643843562e3114c
Author:     Cy Schubert <cy@FreeBSD.org>
AuthorDate: 2022-11-26 16:48:51 +0000
Commit:     Cy Schubert <cy@FreeBSD.org>
CommitDate: 2022-12-01 14:25:53 +0000

    heimdal: Add missing kadm5 error checks

    Generally obtained from upstream 655c057769f56bd8cdb7d16e93f1e7a7cb260342.

    PR:             267944, 267972
    Obtained from:  Heimdal commit 655c057769f56bd8cdb7d16e93f1e7a7cb260342

    (cherry picked from commit 780f663df3a0fc30da5f0680c128087b1a05ea40)

 crypto/heimdal/lib/kadm5/marshall.c | 185 ++++++++++++++++++++----------------
 1 file changed, 105 insertions(+), 80 deletions(-)
Comment 10 Cy Schubert freebsd_committer freebsd_triage 2022-12-01 14:35:45 UTC
fixed
Comment 11 commit-hook freebsd_committer freebsd_triage 2024-03-14 22:13:41 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/ports/commit/?id=f8c4316342857a4fa4a05c1cb6ab16992faddb69

commit f8c4316342857a4fa4a05c1cb6ab16992faddb69
Author:     Cy Schubert <cy@FreeBSD.org>
AuthorDate: 2022-11-26 16:27:08 +0000
Commit:     Cy Schubert <cy@FreeBSD.org>
CommitDate: 2024-03-14 22:12:36 +0000

    security/heimdal: Fix uninitialized pointer dereference

    krb5_ret_preincipal() returns a non-zero return code when
    a garbage principal is passed to it. Unfortunately ret_principal_ent()
    does not check the return code, with garbage pointing to what would
    have been the principal. This results in a segfault when free() is
    called.

    PR:             267944, 267972
    Reported by:    Robert Morris <rtm@lcs.mit.edu>
    MFH:            2024Q1

 security/heimdal/Makefile                         |  2 +-
 security/heimdal/files/patch-lib_kadm5_marshall.c | 31 +++++++++++++++++++++--
 2 files changed, 30 insertions(+), 3 deletions(-)