linux-kernel - Re: [patch] Remove notion of key schemes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1331901784.3730.72.camel@sauron.fi.intel.com>
Date:	Fri, 16 Mar 2012 14:43:04 +0200
From:	Artem Bityutskiy <dedekind1@...il.com>
To:	Joel Reardon <joel@...mbassador.com>
Cc:	linux-mtd@...ts.infradead.org, linux-fsdevel@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [patch] Remove notion of key schemes

On Thu, 2012-03-15 at 15:48 +0100, Joel Reardon wrote:
> @@ -112,8 +109,7 @@ static inline void ino_key_init_flash(const struct ubifs_info *c, void *k,
>  	union ubifs_key *key = k;
> 
>  	key->j32[0] = cpu_to_le32(inum);
> -	key->j32[1] = cpu_to_le32(UBIFS_INO_KEY << UBIFS_S_KEY_BLOCK_BITS);
> -	memset(k + 8, 0, UBIFS_MAX_KEY_LEN - 8);
> +	key->j32[1] = cpu_to_le32(UBIFS_INO_KEY << UBIFS_KEY_BLOCK_BITS);
>  }

So current UBIFS driver will always zero out unused parts of the key.
Looks like a flaw in UBIFS, but it is too late to do anything about
this. Could you please also think about the situation when a
security-enabled image is mounted in an older kernel which will start
zeroing unused bytes. What will happen when it is mounted by newer UBIFS
with the security stuff? Would be great to make sure this is handled
nicely.

>  /**
> - * key_max_inode_size - get maximum file size allowed by current key format.
> + * key_max_inode_size - get maximum file size allowed.
>   * @c: UBIFS file-system description object
>   */
>  static inline unsigned long long key_max_inode_size(const struct ubifs_info *c)
>  {
> -	switch (c->key_fmt) {
> -	case UBIFS_SIMPLE_KEY_FMT:
> -		return (1ULL << UBIFS_S_KEY_BLOCK_BITS) * UBIFS_BLOCK_SIZE;
> -	default:
> -		return 0;
> -	}
> +	return (1ULL << UBIFS_KEY_BLOCK_BITS) * UBIFS_BLOCK_SIZE;
>  }

I think this function should also be removed and turned into a macro. 
>  struct ubifs_dent_node {
>  	struct ubifs_ch ch;
> -	__u8 key[UBIFS_MAX_KEY_LEN];
> +	__u8 key[UBIFS_KEY_LEN];
> +	__u8 padding0[8]; /* Watch 'zero_dent_node_unused()' if changing! */
>  	__le64 inum;
> -	__u8 padding1;
> +	__u8 padding1; /* Watch 'zero_dent_node_unused()' if changing! */
>  	__u8 type;
>  	__le16 nlen;
>  	__u8 padding2[4]; /* Watch 'zero_dent_node_unused()' if changing! */
> @@ -552,7 +547,8 @@ struct ubifs_dent_node {
>   */
>  struct ubifs_data_node {
>  	struct ubifs_ch ch;
> -	__u8 key[UBIFS_MAX_KEY_LEN];
> +	__u8 key[UBIFS_KEY_LEN];
> +	__le64 crypto_lookup;

Err, no, this patch should be _pure_ key schemes removal. All the crypto
stuff should be separate.

Otherwise looks good!

-- 
Best Regards,
Artem Bityutskiy

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)