Re: [PATCH 3/3][RFC] tools: create power/crypto utility

[Eric Biggers]

Hi Yu,

On Fri, Jun 22, 2018 at 10:39:13AM +0800, Yu Chen wrote:
> Hi Eric,
> On Wed, Jun 20, 2018 at 10:41:42AM -0700, Eric Biggers wrote:
> > Hi Chen,
> > 
> > On Wed, Jun 20, 2018 at 05:40:51PM +0800, Chen Yu wrote:
> > > crypto_hibernate is a user-space utility to generate
> > > 512bits AES key and pass it to the kernel via ioctl
> > > for hibernation encryption.(We can also add the key
> > > into kernel via keyctl if necessary, but currently
> > > using ioctl seems to be more straightforward as we
> > > need both the key and salt transit).
> > > 
> > > The key derivation is based on a simplified implementation
> > > of PBKDF2[1] in e2fsprogs - both the key length and the hash
> > > bytes are the same - 512bits. crypto_hibernate will firstly
> > > probe the user for passphrase and get salt from kernel, then
> > > uses them to generate a 512bits AES key based on PBKDF2.
> Thanks for reviewing this.
> > 
> > What is a "512bits AES key"?  Do you mean AES-256-XTS (which takes a 512-bit
> > key, which the XTS mode internally splits into two keys)? 
> Yes, it is AES-256-XTS.
> > Do you allow for
> > other algorithms, or is it hardcoded to AES-256-XTS? 
> Currently it is hardcoded to AES-256-XTS. It is copied from implementation
> of PBKDF2 in e2fsprogs, which is hardcoded to useAES-256-XTS for ext4 encryption
> in the kernel(pbkdf2_sha512() in e2fsprogs)
>
> > What if someone wants to
> > use a different algorithm?
> > 
> If user wants to use a different algorithm, then I think we need to
> port the code from openssl, which is the full implementation of PBKDF2
> for:
> https://www.ietf.org/rfc/rfc2898.txt 

You seem to be confusing the key derivation function (KDF) with the encryption
algorithm the derived key is used for.  The purpose of a KDF is to derive key
material.  The resulting key material can be used for any encryption algorithm,
provided that you derive the needed amount of key material.  Note that different
encryption algorithms can use the same length key, e.g. SERPENT-256-XTS and
AES-256-XTS are both examples of algorithms that use a 64 byte (512-bit) key.
So your design probably should provide a way for an *algorithm* to be selected,
not just a key length.

> > BTW, it's difficult to review this with only patch 3/3 Cc'ed to me, as there is
> > no context about the problem you are trying to solve and what your actual
> > proposed kernel changes are.  I suggest Cc'ing linux-crypto on all 3 patches.
> > 
> Ok, I'll send a refreshed version.
> > A few more comments below, from a very brief reading of the code:
> > 
> > [...]
> > > +
> > > +#define PBKDF2_ITERATIONS          0xFFFF
> > > +#define SHA512_BLOCKSIZE 128
> > > +#define SHA512_LENGTH 64
> > > +#define SALT_BYTES	16
> > > +#define SYM_KEY_BYTES SHA512_LENGTH
> > > +#define TOTAL_USER_INFO_LEN	(SALT_BYTES+SYM_KEY_BYTES)
> > > +#define MAX_PASSPHRASE_SIZE	1024
> > > +
> > > +struct hibernation_crypto_keys {
> > > +	char derived_key[SYM_KEY_BYTES];
> > > +	char salt[SALT_BYTES];
> > > +	bool valid;
> > > +};
> > > +
> > > +struct hibernation_crypto_keys hib_keys;
> > > +
> > > +static char *get_key_ptr(void)
> > > +{
> > > +	return hib_keys.derived_key;
> > > +}
> > > +
> > > +static char *get_salt_ptr(void)
> > > +{
> > > +	return hib_keys.salt;
> > > +}
> > [...]
> > > +
> > > +
> > > +#define HIBERNATE_SALT_READ      _IOW('C', 3, struct hibernation_crypto_keys)
> > > +#define HIBERNATE_KEY_WRITE     _IOW('C', 4, struct hibernation_crypto_keys)
> > 
> > Why are the ioctl numbers defined based on the size of 'struct
> > hibernation_crypto_keys'?  It's not a UAPI structure, right?
> > 
> It's not a UAPI structure, and it is defined both in user space tool
> and in kernel. Do you mean, I should put the defination of this
> structure under include/uapi ?

No, I'm saying that you shouldn't define ioctl numbers (permanent ABI) based on
the size of some random structure that is subject to change.

> > > +
> > > +static void get_passphrase(char *passphrase, int len)
> > > +{
> > > +	char *p;
> > > +	struct termios current_settings;
> > > +
> > > +	assert(len > 0);
> > > +	disable_echo(&current_settings);
> > > +	p = fgets(passphrase, len, stdin);
> > > +	tcsetattr(0, TCSANOW, &current_settings);
> > > +	printf("\n");
> > > +	if (!p) {
> > > +		printf("Aborting.\n");
> > > +		exit(1);
> > > +	}
> > > +	p = strrchr(passphrase, '\n');
> > > +	if (!p)
> > > +		p = passphrase + len - 1;
> > > +	*p = '\0';
> > > +}
> > > +
> > > +#define CRYPTO_FILE	"/dev/crypto_hibernate"
> > > +
> > > +static int write_keys(void)
> > > +{
> > > +	int fd;
> > > +
> > > +	fd = open(CRYPTO_FILE, O_RDWR);
> > > +	if (fd < 0) {
> > > +		printf("Cannot open device file...\n");
> > > +		return -EINVAL;
> > > +	}
> > > +	ioctl(fd, HIBERNATE_KEY_WRITE, get_key_ptr());
> > > +	return 0;
> > 
> > No error checking on the ioctl?
> > 
> Ok, will add error checking for it.
> > Also, how is the kernel supposed to know how long the key is, and which
> > algorithm it's supposed to be used for?  I assume it's hardcoded to AES-256-XTS?
> > What if someone wants to use a different algorithm?
> > 
> Yes, the length in both user space and kernel are hardcoded to AES-256-XTS.
> I can add the support for other algorithm, but might need to port from
> openssl first.

The kernel crypto API already supports many other ciphers.  It's the kernel that
actually does the encryption, right?

Again, you should Cc the whole patchset to linux-crypto so that people can
review it context, including your new kernel code that actually does the
encryption, and see what problem you are actually trying to solve.

Thanks!

- Eric