Re: [PATCH v3 1/7] ima-evm-utils: Fix hash buffer overflow in verify_evm and hmac_evm

[Mimi Zohar <zohar@linux.ibm.com>]

On Mon, 2018-12-03 at 06:35 +0300, Vitaly Chikunov wrote:
> Commit ae1319eeabd6 ("Remove hardcoding of SHA1 in EVM signatures")
> introduces overflow of 20 byte buffer on the stack while calculating
> hash. Also, invalid hash length is passed to the underlying verification
> function in verify_evm. This prevents any non-SHA1 hashes from being
> properly validated using evmctl.
> 
> Fixes: ae1319eeabd6 ("Remove hardcoding of SHA1 in EVM signatures")
> Signed-off-by: Vitaly Chikunov <vt@altlinux.org>

Thank you!  This patch set has now been applied and is in master.

Mimi

> ---
> Changes since v1:
> - Fix similar issue in hmac_evm
> Changes since v2:
> - No changes.
> 
>  src/evmctl.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/src/evmctl.c b/src/evmctl.c
> index 1b46d58..f8035da 100644
> --- a/src/evmctl.c
> +++ b/src/evmctl.c
> @@ -55,6 +55,7 @@
>  #include <keyutils.h>
>  #include <ctype.h>
>  #include <termios.h>
> +#include <assert.h>
> 
>  #include <openssl/sha.h>
>  #include <openssl/pem.h>
> @@ -760,13 +761,15 @@ static int cmd_sign_evm(struct command *cmd)
> 
>  static int verify_evm(const char *file)
>  {
> -	unsigned char hash[20];
> +	unsigned char hash[64];
>  	unsigned char sig[1024];
> +	int mdlen;
>  	int len;
> 
> -	len = calc_evm_hash(file, hash);
> -	if (len <= 1)
> -		return len;
> +	mdlen = calc_evm_hash(file, hash);
> +	assert(mdlen <= sizeof(hash));
> +	if (mdlen <= 1)
> +		return mdlen;
> 
>  	len = lgetxattr(file, "security.evm", sig, sizeof(sig));
>  	if (len < 0) {
> @@ -779,7 +782,7 @@ static int verify_evm(const char *file)
>  		return -1;
>  	}
> 
> -	return verify_hash(file, hash, sizeof(hash), sig + 1, len - 1);
> +	return verify_hash(file, hash, mdlen, sig + 1, len - 1);
>  }
> 
>  static int cmd_verify_evm(struct command *cmd)
> @@ -1135,11 +1138,12 @@ out:
> 
>  static int hmac_evm(const char *file, const char *key)
>  {
> -	unsigned char hash[20];
> +	unsigned char hash[64];
>  	unsigned char sig[1024];
>  	int len, err;
> 
>  	len = calc_evm_hmac(file, key, hash);
> +	assert(len <= sizeof(hash));
>  	if (len <= 1)
>  		return len;
>