Ticket History #6770: kg_unseal leads to overlap of source and desitination in memcpy...

# Fri Sep 03 00:27:16 2010 epeisach (Ezra Peisach) - Ticket created

Date:	Fri, 03 Sep 2010 00:27:03 -0400
From:	Ezra Peisach <epeisach@MIT.EDU>
To:	krb5-bugs@MIT.EDU
Subject:	kg_unseal leads to overlap of source and desitination in memcpy...

Download (untitled) / with headers
text/plain 1KiB

This happens in kadmin...

According to the memcpy man page memmove should be used if the memory
overlaps.

The k5_unseal calls kg_encrypt with the same source and destination....

==3917== Command:
/home/epeisach/krb5/trunk/build/tests/dejagnu/../../kadmin/cli
/kadmin -p krbtest/admin@KRBTEST.COM -q ank\ -randkey\
sample/chem-dhcp-143.bu.e
du@KRBTEST.COM
==3917== Parent PID: 27515
==3917==
==3917== Source and destination overlap in memcpy(0x41ca160, 0x41ca160, 16)
==3917== at 0x4007535: memcpy (mc_replace_strmem.c:497)
==3917== by 0x412BF9B: krb5_k_encrypt (encrypt.c:65)
==3917== by 0x405AAC8: kg_encrypt (util_crypt.c:239)
==3917== by 0x4053A1C: kg_unseal (k5unseal.c:301)
==3917== by 0x405B398: krb5_gss_verify_mic (verify.c:43)
==3917== by 0x4045B44: gss_verify_mic (g_verify.c:72)
==3917== by 0x402098F: authgss_refresh (auth_gss.c:492)
==3917== by 0x4020BC5: gssrpc_authgss_create (auth_gss.c:210)
==3917== by 0x40171C8: init_any (client_init.c:756)
==3917== by 0x804D751: kadmin_startup (kadmin.c:515)
==3917== by 0x804DB63: main (ss_wrapper.c:48)

# Mon Sep 13 20:31:38 2010 ghudson@mit.edu (Greg Hudson) - Correspondence added

Download (untitled) / with headers
text/plain 933B

Mulling this one over still.

It's very unusual for callers to call krb5_[ck]_encrypt like this, since
normally the output needs a larger buffer than the input. In this use
case, the enctype is raw-mode DES, so in-place encryption is sensical.
There are four kg_encrypt call sites like this in the krb5 mech.

Possible solutions are:

1. Declare the callers at fault and change them. This would be fairly
easy since the encryptions are of fixed-length (16-byte) buffers.

2. Use memmove instead of memcpy, or check if input->data == output-

Show quoted text

>ciphertext.data + header_len and elide the memcpy in that case. This

would allow aliasing of input and output in krb5_k_encrypt only for raw-
mode enctypes.

3. Allow aliasing for all enctypes somehow. (I'm not sure if it worked
for non-raw enctypes prior to 1.8.)

I am currently tempted to go with #1 since the other solutions have a
performance impact on well-behaved callers.

# Tue Oct 26 13:18:22 2010 ghudson@mit.edu (Greg Hudson) - Given to ghudson@mit.edu (Greg Hudson)

# Tue Oct 26 13:18:22 2010 ghudson@mit.edu (Greg Hudson) - Status changed from 'new' to 'review'

# Tue Oct 26 13:18:22 2010 ghudson@mit.edu (Greg Hudson) - Correspondence added

From:	ghudson@mit.edu
Subject:	SVN Commit

Download (untitled) / with headers
text/plain 608B

Add a kg_encrypt_inplace() utility function to the krb5 GSS mech, and
use it where we do in-place encryption of checksums in the non-CFX
seal tokens with raw DES enctypes. Avoids a harmless but incorrect
in-place memcpy().

https://github.com/krb5/krb5/commit/c079477480c839bd983afd082ac7a7ec51e906ca
Commit By: ghudson
Revision: 24485
Changed Files:
U trunk/src/lib/gssapi/krb5/gssapiP_krb5.h
U trunk/src/lib/gssapi/krb5/k5seal.c
U trunk/src/lib/gssapi/krb5/k5sealiov.c
U trunk/src/lib/gssapi/krb5/k5unseal.c
U trunk/src/lib/gssapi/krb5/k5unsealiov.c
U trunk/src/lib/gssapi/krb5/util_crypt.c

# Mon Oct 17 19:01:23 2011 tlyu (Taylor Yu) - Version_Fixed 1.10 added

# Sun Sep 24 20:39:56 2017 ghudson@mit.edu (Greg Hudson) - Status changed from 'review' to 'resolved'