From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mat Martineau Subject: Re: [PATCH v2] KEYS: add SP800-56A KDF support for DH Date: Thu, 4 Aug 2016 13:41:58 -0700 (PDT) Message-ID: References: <2239809.KsND40bFeW@positron.chronox.de> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Cc: David Howells , keyrings@vger.kernel.org, linux-crypto@vger.kernel.org To: Stephan Mueller Return-path: Received: from mga09.intel.com ([134.134.136.24]:65004 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934199AbcHDUmA (ORCPT ); Thu, 4 Aug 2016 16:42:00 -0400 In-Reply-To: <2239809.KsND40bFeW@positron.chronox.de> Sender: linux-crypto-owner@vger.kernel.org List-ID: Stephan, On Thu, 4 Aug 2016, Stephan Mueller wrote: > Hi Mat, > > the following code addresses all your comments as discussed. > > In addition, this code is now complete: it contains an extension > to the documentation as well as the compat code. The compat > code was tested successfully with compiling the user space tool > with -m32 after applying the patch from David ensuring the > invocation of the keyctl compat syscall. > > Ciao > Stephan > > ---8<---- > > SP800-56A defines the use of DH with key derivation function based on a > counter. The input to the KDF is defined as (DH shared secret || other > information). The value for the "other information" is to be provided by > the caller. > > The KDF is provided by the kernel crypto API. The SP800-56A KDF is equal > to the SP800-108 counter KDF. However, the caller is allowed to specify > the KDF type that he wants to use to derive the key material allowing > the use of the other KDFs provided with the kernel crypto API. > > As the KDF implements the proper truncation of the DH shared secret to > the requested size, this patch fills the caller buffer up to its size. > > The patch is tested with a new test added to the keyutils user space > code which uses a CAVS test vector testing the compliance with > SP800-56A. > > Signed-off-by: Stephan Mueller > --- > Documentation/security/keys.txt | 35 +++++++++--- > include/linux/compat.h | 7 +++ > include/uapi/linux/keyctl.h | 7 +++ > security/keys/Kconfig | 1 + > security/keys/compat.c | 34 +++++++++++- > security/keys/dh.c | 119 ++++++++++++++++++++++++++++++++++++---- > security/keys/internal.h | 9 ++- > security/keys/keyctl.c | 2 +- > 8 files changed, 189 insertions(+), 25 deletions(-) > > diff --git a/Documentation/security/keys.txt b/Documentation/security/keys.txt > index 3849814..29fc036 100644 > --- a/Documentation/security/keys.txt > +++ b/Documentation/security/keys.txt > @@ -827,7 +827,7 @@ The keyctl syscall functions are: > > long keyctl(KEYCTL_DH_COMPUTE, struct keyctl_dh_params *params, > char *buffer, size_t buflen, > - void *reserved); > + struct keyctl_kdf_params *kdf); > > The params struct contains serial numbers for three keys: > > @@ -844,18 +844,37 @@ The keyctl syscall functions are: > public key. If the base is the remote public key, the result is > the shared secret. > > - The reserved argument must be set to NULL. > + If the parameter kdf is NULL, the following applies: > > - The buffer length must be at least the length of the prime, or zero. > + - The buffer length must be at least the length of the prime, or zero. > > - If the buffer length is nonzero, the length of the result is > - returned when it is successfully calculated and copied in to the > - buffer. When the buffer length is zero, the minimum required > - buffer length is returned. > + - If the buffer length is nonzero, the length of the result is > + returned when it is successfully calculated and copied in to the > + buffer. When the buffer length is zero, the minimum required > + buffer length is returned. > + > + The kdf parameter allows the caller to apply a key derivation function > + (KDF) on the Diffie-Hellman computation where only the result > + of the KDF is returned to the caller. The KDF is characterized with > + struct keyctl_kdf_params as follows: > + > + - char *kdfname specifies the NULL terminated string identifying Change "NULL" to "NUL". NULL refers to a zero pointer, NUL refers to the '\0' character. > + the KDF function used from the kernel crypto API. As of now, > + only non-keyed KDFs are supported, such as kdf_ctr(sha256), > + kdf_fb(sha1) or kdf_dpi(sha512). The use of kdf_ctr() complies > + with SP800-56A. > + > + - char *otherinfo specifies the OtherInfo string as documented in I suggest calling it "OtherInfo data" rather than a "string", which implies a NUL-terminated C string. > + SP800-56A section 5.8.1.2. The length of the buffer is given with > + otherinfolen. The format of OtherInfo is defined by the caller. > + The otherinfo pointer may be NULL if no OtherInfo shall be used. > > This function will return error EOPNOTSUPP if the key type is not > supported, error ENOKEY if the key could not be found, or error > - EACCES if the key is not readable by the caller. > + EACCES if the key is not readable by the caller. In addition, the > + function will return EMSGSIZE when the parameter kdf is non-NULL > + and either the buffer length or the OtherInfo length exceeds the > + allowed length. > > =============== > KERNEL SERVICES > diff --git a/include/linux/compat.h b/include/linux/compat.h > index f964ef7..00f348f 100644 > --- a/include/linux/compat.h > +++ b/include/linux/compat.h > @@ -295,6 +295,13 @@ struct compat_old_sigaction { > }; > #endif > > +struct compat_keyctl_kdf_params { > + compat_uptr_t kdfname; > + compat_uptr_t otherinfo; > + __u32 otherinfolen; > + __u32 __spare[8]; > +}; > + > struct compat_statfs; > struct compat_statfs64; > struct compat_old_linux_dirent; > diff --git a/include/uapi/linux/keyctl.h b/include/uapi/linux/keyctl.h > index 86eddd6..0abe048 100644 > --- a/include/uapi/linux/keyctl.h > +++ b/include/uapi/linux/keyctl.h > @@ -68,4 +68,11 @@ struct keyctl_dh_params { > __s32 base; > }; > > +struct keyctl_kdf_params { > + char *kdfname; > + char *otherinfo; > + __u32 otherinfolen; > + __u32 __spare[8]; > +}; > + > #endif /* _LINUX_KEYCTL_H */ > diff --git a/security/keys/Kconfig b/security/keys/Kconfig > index f826e87..56491fe 100644 > --- a/security/keys/Kconfig > +++ b/security/keys/Kconfig > @@ -90,6 +90,7 @@ config KEY_DH_OPERATIONS > bool "Diffie-Hellman operations on retained keys" > depends on KEYS > select MPILIB > + select CRYPTO_KDF > help > This option provides support for calculating Diffie-Hellman > public keys and shared secrets using values stored as keys > diff --git a/security/keys/compat.c b/security/keys/compat.c > index 36c80bf..7a7d48a 100644 > --- a/security/keys/compat.c > +++ b/security/keys/compat.c > @@ -48,6 +48,35 @@ static long compat_keyctl_instantiate_key_iov( > return ret; > } > > +#ifdef CONFIG_KEY_DH_OPERATIONS See chapter 20 of https://www.kernel.org/doc/Documentation/CodingStyle, preprocessor conditionals are advised against in .c files. You can put the function prototype and stub implementation in internal.h, and move the function to compat_dh.c. Then the Makefile should only build compat_dh.c if both relevant config options are set (look for "compat-obj" in other Makefiles). > +static long compat_keyctl_dh_compute(struct keyctl_dh_params __user *params, > + char __user *buffer, size_t buflen, > + struct compat_keyctl_kdf_params __user *kdf) > +{ > + struct keyctl_kdf_params kdfcopy; > + struct compat_keyctl_kdf_params compat_kdfcopy; > + > + if (!kdf) > + return __keyctl_dh_compute(params, buffer, buflen, NULL); > + > + if (copy_from_user(&compat_kdfcopy, kdf, sizeof(compat_kdfcopy)) != 0) > + return -EFAULT; > + > + kdfcopy.kdfname = compat_ptr(compat_kdfcopy.kdfname); > + kdfcopy.otherinfo = compat_ptr(compat_kdfcopy.otherinfo); > + kdfcopy.otherinfolen = compat_kdfcopy.otherinfolen; > + > + return __keyctl_dh_compute(params, buffer, buflen, &kdfcopy); > +} > +#else > +static long compat_keyctl_dh_compute(struct keyctl_dh_params __user *params, > + char __user *buffer, size_t buflen, > + struct keyctl_kdf_params __user *kdf) > +{ > + return -EOPNOTSUPP > +} > +#endif > + > /* > * The key control system call, 32-bit compatibility version for 64-bit archs > * > @@ -133,8 +162,9 @@ COMPAT_SYSCALL_DEFINE5(keyctl, u32, option, > return keyctl_get_persistent(arg2, arg3); > > case KEYCTL_DH_COMPUTE: > - return keyctl_dh_compute(compat_ptr(arg2), compat_ptr(arg3), > - arg4, compat_ptr(arg5)); > + return compat_keyctl_dh_compute(compat_ptr(arg2), > + compat_ptr(arg3), > + arg4, compat_ptr(arg5)); > > default: > return -EOPNOTSUPP; > diff --git a/security/keys/dh.c b/security/keys/dh.c > index 531ed2e..6a3dea9 100644 > --- a/security/keys/dh.c > +++ b/security/keys/dh.c > @@ -77,9 +77,44 @@ error: > return ret; > } > > -long keyctl_dh_compute(struct keyctl_dh_params __user *params, > - char __user *buffer, size_t buflen, > - void __user *reserved) > +static int keyctl_dh_compute_kdf(struct crypto_rng *tfm, > + char __user *buffer, size_t buflen, > + uint8_t *kbuf, size_t kbuflen) > +{ > + uint8_t *outbuf = NULL; > + int ret; > + > +#if 0W Need to remove these conditional lines. > + /* we do not support HMAC currently */ > + ret = crypto_rng_reset(tfm, xx, xxlen); > + if (ret) { > + crypto_free_rng(tfm); > + goto error5; > + } > +#endif > + > + outbuf = kmalloc(buflen, GFP_KERNEL); > + if (!outbuf) { > + ret = -ENOMEM; > + goto err; > + } > + > + ret = crypto_rng_generate(tfm, kbuf, kbuflen, outbuf, buflen); > + if (ret) > + goto err; > + > + ret = buflen; > + if (copy_to_user(buffer, outbuf, buflen) != 0) > + ret = -EFAULT; > + > +err: > + kzfree(outbuf); > + return ret; > +} > + > +long __keyctl_dh_compute(struct keyctl_dh_params __user *params, > + char __user *buffer, size_t buflen, > + struct keyctl_kdf_params *kdfcopy) > { > long ret; > MPI base, private, prime, result; > @@ -88,6 +123,7 @@ long keyctl_dh_compute(struct keyctl_dh_params __user *params, > uint8_t *kbuf; > ssize_t keylen; > size_t resultlen; > + struct crypto_rng *tfm = NULL; > > if (!params || (!buffer && buflen)) { > ret = -EINVAL; > @@ -98,12 +134,36 @@ long keyctl_dh_compute(struct keyctl_dh_params __user *params, > goto out; > } > > - if (reserved) { > - ret = -EINVAL; > - goto out; > + if (kdfcopy) { > + char *kdfname; > + > + if (buflen > KEYCTL_KDF_MAX_OUTPUTLEN || > + kdfcopy->otherinfolen > KEYCTL_KDF_MAX_STRING_LEN) { > + ret = -EMSGSIZE; > + goto out; > + } > + > + /* get KDF name string */ > + kdfname = strndup_user(kdfcopy->kdfname, CRYPTO_MAX_ALG_NAME); > + if (IS_ERR(kdfname)) { > + ret = PTR_ERR(kdfname); > + goto out; > + } > + > + /* allocate KDF from the kernel crypto API */ > + tfm = crypto_alloc_rng(kdfname, 0, 0); > + kfree(kdfname); > + if (IS_ERR(tfm)) { > + ret = PTR_ERR(tfm); > + goto out; > + } > } > > - keylen = mpi_from_key(pcopy.prime, buflen, &prime); > + /* > + * If the caller requests postprocessing with a KDF, allow an > + * arbitrary output buffer size since the KDF ensures proper truncation. > + */ > + keylen = mpi_from_key(pcopy.prime, kdfcopy ? SIZE_MAX : buflen, &prime); > if (keylen < 0 || !prime) { > /* buflen == 0 may be used to query the required buffer size, > * which is the prime key length. > @@ -133,12 +193,25 @@ long keyctl_dh_compute(struct keyctl_dh_params __user *params, > goto error3; > } > > - kbuf = kmalloc(resultlen, GFP_KERNEL); > + /* allocate space for DH shared secret and SP800-56A otherinfo */ > + kbuf = kmalloc(kdfcopy ? (resultlen + kdfcopy->otherinfolen) : resultlen, > + GFP_KERNEL); > if (!kbuf) { > ret = -ENOMEM; > goto error4; > } > > + /* > + * Concatenate SP800-56A otherinfo past DH shared secret -- the > + * input to the KDF is (DH shared secret || otherinfo) > + */ > + if (kdfcopy && kdfcopy->otherinfo && > + copy_from_user(kbuf + resultlen, kdfcopy->otherinfo, > + kdfcopy->otherinfolen) != 0) { > + ret = -EFAULT; > + goto error5; > + } > + > ret = do_dh(result, base, private, prime); > if (ret) > goto error5; > @@ -147,12 +220,17 @@ long keyctl_dh_compute(struct keyctl_dh_params __user *params, > if (ret != 0) > goto error5; > > - ret = nbytes; > - if (copy_to_user(buffer, kbuf, nbytes) != 0) > - ret = -EFAULT; > + if (kdfcopy) { > + ret = keyctl_dh_compute_kdf(tfm, buffer, buflen, kbuf, > + resultlen + kdfcopy->otherinfolen); > + } else { > + ret = nbytes; > + if (copy_to_user(buffer, kbuf, nbytes) != 0) > + ret = -EFAULT; > + } > > error5: > - kfree(kbuf); > + kzfree(kbuf); > error4: > mpi_free(result); > error3: > @@ -162,5 +240,22 @@ error2: > error1: > mpi_free(prime); > out: > + if (tfm) > + crypto_free_rng(tfm); > return ret; > } > + > +long keyctl_dh_compute(struct keyctl_dh_params __user *params, > + char __user *buffer, size_t buflen, > + struct keyctl_kdf_params __user *kdf) > +{ > + struct keyctl_kdf_params kdfcopy; > + > + if (!kdf) > + return __keyctl_dh_compute(params, buffer, buflen, NULL); > + > + if (copy_from_user(&kdfcopy, kdf, sizeof(kdfcopy)) != 0) > + return -EFAULT; > + > + return __keyctl_dh_compute(params, buffer, buflen, &kdfcopy); I'd find this more readable if there was one call to __keyctl_dh_compute. > +} > diff --git a/security/keys/internal.h b/security/keys/internal.h > index a705a7d..9014af3 100644 > --- a/security/keys/internal.h > +++ b/security/keys/internal.h > @@ -259,12 +259,17 @@ static inline long keyctl_get_persistent(uid_t uid, key_serial_t destring) > #endif > > #ifdef CONFIG_KEY_DH_OPERATIONS > +#include > extern long keyctl_dh_compute(struct keyctl_dh_params __user *, char __user *, > - size_t, void __user *); > + size_t, struct keyctl_kdf_params __user *); > +extern long __keyctl_dh_compute(struct keyctl_dh_params __user *, char __user *, > + size_t, struct keyctl_kdf_params *); > +#define KEYCTL_KDF_MAX_OUTPUTLEN 1024 /* max length of KDF output */ KEYCTL_KDF_MAX_OUTPUT_LEN ^ for consistency? > +#define KEYCTL_KDF_MAX_STRING_LEN 64 /* maximum length of strings */ Since this applies only to otherinfo (binary bytes, not a C string), avoid "STRING"/"strings". > #else > static inline long keyctl_dh_compute(struct keyctl_dh_params __user *params, > char __user *buffer, size_t buflen, > - void __user *reserved) > + struct keyctl_kdf_params __user *kdf) > { > return -EOPNOTSUPP; > } > diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c > index d580ad0..b106898 100644 > --- a/security/keys/keyctl.c > +++ b/security/keys/keyctl.c > @@ -1689,7 +1689,7 @@ SYSCALL_DEFINE5(keyctl, int, option, unsigned long, arg2, unsigned long, arg3, > case KEYCTL_DH_COMPUTE: > return keyctl_dh_compute((struct keyctl_dh_params __user *) arg2, > (char __user *) arg3, (size_t) arg4, > - (void __user *) arg5); > + (struct keyctl_kdf_params __user *) arg5); > > default: > return -EOPNOTSUPP; Regards, -- Mat Martineau Intel OTC