[v2] Remove the superfluous salt parameter
diff mbox series

Message ID 20190620201452.28712-1-laszlo@ashin.hu
State Changes Requested
Headers show
Series
  • [v2] Remove the superfluous salt parameter
Related show

Commit Message

'Pierre-Jean Texier' via swupdate June 20, 2019, 8:14 p.m. UTC
I started working on adding mbedTLS as an alternative implementation and while running
the unittest test_crypt I noticed an interesting thing. If I null out the salt argument
like below then the test remains successful:

> --- a/corelib/test/test_crypt.c
> +++ b/corelib/test/test_crypt.c
> @@ -95,6 +95,7 @@ static void test_crypt_salt(void **state)
>         hex2bin((crypt.key = calloc(1, strlen((const char *)KEY))), KEY);
>         hex2bin((crypt.iv = calloc(1, strlen((const char *)IV))), IV);
>         hex2bin((crypt.salt = calloc(1, strlen((const char *)SALT))), SALT);
> +       crypt.salt = NULL;
>         hex2bin((crypt.crypttext = calloc(1, strlen((const char *)CRYPTTEXT))), CRYPTTEXT);
>
>         do_crypt(&crypt, &CRYPTTEXT[0], &PLAINTEXT[0]);

Does this mean that the crypto feature doesn't even use this argument?
After reading the openssl manual it looks like that salt is only an optional input for generating
the key and the IV but it is not used for encryption/decryption. Once you have the key and the IV
derived from the password and this optional salt, the only inputs needed for doing the actual
encryption or decryption is the key and the IV.

But what does the code use this salt value for? The answer can be found in corelib/swupdate_decrypt.c:

```
	if (salt != NULL) {
		unsigned char dummy_key[EVP_MAX_KEY_LENGTH];
		unsigned char dummy_iv[EVP_MAX_IV_LENGTH];
		unsigned char dummy_pwd[5] = "DUMMY";
		if (!EVP_BytesToKey(cipher, EVP_sha1(), salt,
							dummy_pwd, sizeof(dummy_pwd),
							1,
							(unsigned char *)&dummy_key, (unsigned char *)&dummy_iv)) {
			ERROR("Cannot set salt.");
			free(dgst);
			return NULL;
		}
	}
```

If this salt value is not null, it gets passed to the EVP_BytesToKey function. Its manual says:

>      int
>      EVP_BytesToKey(const EVP_CIPHER *type, const EVP_MD *md,
>          const unsigned char *salt, const unsigned char *data, int datal,
>          int count, unsigned char *key, unsigned char *iv);
>
> DESCRIPTION
>      EVP_BytesToKey() derives a key and IV from various parameters.  type is
>      the cipher to derive the key and IV for.  md is the message digest to
>      use.  The salt parameter is used as a salt in the derivation: it should
>      point to an 8-byte buffer or NULL if no salt is used.  data is a buffer
>      containing datal bytes which is used to derive the keying data.  count is
>      the iteration count to use.  The derived key and IV will be written to
>      key and iv, respectively.

So this function produces its output to the buffers pointed to by the last two arguments.
In swupdate these are dummy_key and dummy_iv and those get discarded right after the
call because those are just local buffers allocated on the stack. (Not to mention we are
using the hard wired value of "DUMMY" as password.)

It looks like this piece of code doesn't affect the behaviour of the actual decryption
algorithm, so it should be discarded.

The swupdate documentation needs to be revisited as well.
First, it suggests that we use the -S parameter of `openssl enc` for the encryption
which is unnecessary as the command produces exactly the same result without this option.

```
$ echo abc > myfile
$ openssl enc -aes-256-cbc -k mypassword -P -md sha1
salt=1182D4B32E653C85
key=F3A67C3871FEC7FF2D1772DFE0B770111A098FB53B98AEF6D69E7BFFEC90FDF1
iv =121EE144C6BE4625CA9FBD01BC386053
$ openssl enc -aes-256-cbc -in myfile -out enc1 \
	-K 20FC1F26F93E4BFADB087E45CC3F30FD4749D5D9F00989EB12BFA4DFD534381C \
	-iv 5389F2FCE84BFA80CC77D8C99015FD85 \
	-S 768AA42EC048396D
$ openssl enc -aes-256-cbc -in myfile -out enc2 \
	-K 20FC1F26F93E4BFADB087E45CC3F30FD4749D5D9F00989EB12BFA4DFD534381C \
	-iv 5389F2FCE84BFA80CC77D8C99015FD85
$ sha1sum enc1 enc2
6ca7157f81b056c04e452744ba85a12ab4b457f0  enc1
6ca7157f81b056c04e452744ba85a12ab4b457f0  enc2
```

This salt parameter is only needed for the key generation from a password in the first step.
Then, passing the salt parameter to swupdate in the key file as a third field
is also unnecessary. The parser code of this parameter can be discarded.

As an additional note, we should not reuse the same key/IV pair for encrypting files/images.
See https://crypto.stackexchange.com/a/10511
Best would be to use a randomized IV for each encrypted file and image and include that IV
for each ciphertext for making decryption possible. IV should be used only once but doesn't
need to be kept as a secret. Only the key should be confidential.

As another side note using sha1 for forming the key and IV from the password is not a best
practice either. Newer version of openssl warns to use a password key derivation function
like PBKDF2. Generating key and IV can be done without using passwords at all:

```
key=$(dd if=/dev/random bs=32 count=1 | xxd -ps -cols 64)
iv=$(dd if=/dev/random bs=16 count=1 | xxd -ps -cols 32)
```

My current patch only copes with the superfluous salt parameter.

Signed-off-by: Laszlo Ashin <laszlo@ashin.hu>
---
 core/cpio_utils.c               |  4 +---
 core/util.c                     | 23 ++++-------------------
 corelib/swupdate_decrypt.c      | 16 +---------------
 corelib/test/test_crypt.c       | 19 ++++++-------------
 doc/source/encrypted_images.rst | 17 +++++++----------
 include/sslapi.h                |  4 ++--
 include/util.h                  |  1 -
 7 files changed, 21 insertions(+), 63 deletions(-)

Comments

Stefano Babic June 21, 2019, 9:29 a.m. UTC | #1
Hi Laszlo,

On 20/06/19 22:14, 'Laszlo Ashin' via swupdate wrote:
> I started working on adding mbedTLS as an alternative implementation and while running
> the unittest test_crypt I noticed an interesting thing. If I null out the salt argument
> like below then the test remains successful:
> 
>> --- a/corelib/test/test_crypt.c
>> +++ b/corelib/test/test_crypt.c
>> @@ -95,6 +95,7 @@ static void test_crypt_salt(void **state)
>>         hex2bin((crypt.key = calloc(1, strlen((const char *)KEY))), KEY);
>>         hex2bin((crypt.iv = calloc(1, strlen((const char *)IV))), IV);
>>         hex2bin((crypt.salt = calloc(1, strlen((const char *)SALT))), SALT);
>> +       crypt.salt = NULL;
>>         hex2bin((crypt.crypttext = calloc(1, strlen((const char *)CRYPTTEXT))), CRYPTTEXT);
>>
>>         do_crypt(&crypt, &CRYPTTEXT[0], &PLAINTEXT[0]);
> 
> Does this mean that the crypto feature doesn't even use this argument?
> After reading the openssl manual it looks like that salt is only an optional input for generating
> the key and the IV but it is not used for encryption/decryption. Once you have the key and the IV
> derived from the password and this optional salt, the only inputs needed for doing the actual
> encryption or decryption is the key and the IV.

After reading myself (again..) the openSSL documentation, I am coming to
the same conclusion. salt is used together with password to increase
entropy of the hashed value. But SWUpdate uses key and initialization
vector, and salt is just the output when the key is generated.

openSSL documentation says that salt is then attached to the output
buffer and it is not needed to be set when it is decrypted.

> 
> But what does the code use this salt value for? The answer can be found in corelib/swupdate_decrypt.c:
> 
> ```
> 	if (salt != NULL) {
> 		unsigned char dummy_key[EVP_MAX_KEY_LENGTH];
> 		unsigned char dummy_iv[EVP_MAX_IV_LENGTH];
> 		unsigned char dummy_pwd[5] = "DUMMY";
> 		if (!EVP_BytesToKey(cipher, EVP_sha1(), salt,
> 							dummy_pwd, sizeof(dummy_pwd),
> 							1,
> 							(unsigned char *)&dummy_key, (unsigned char *)&dummy_iv)) {
> 			ERROR("Cannot set salt.");
> 			free(dgst);
> 			return NULL;
> 		}
> 	}
> ```
> 

I think you're right - salt is used when key is generated, and the
output are key and IV. But as SWUpdate require as input both key and IV,
they are discarded. The EVP_BytesToKey() are then no effect, because the
generated key and IV are not used at all. It looks like this call is
mainly due to a misunderstanding of openSSL documentation (well, it must
be sometimes interpreted) and it is not required.


> If this salt value is not null, it gets passed to the EVP_BytesToKey function. Its manual says:
> 
>>      int
>>      EVP_BytesToKey(const EVP_CIPHER *type, const EVP_MD *md,
>>          const unsigned char *salt, const unsigned char *data, int datal,
>>          int count, unsigned char *key, unsigned char *iv);
>>
>> DESCRIPTION
>>      EVP_BytesToKey() derives a key and IV from various parameters.  type is
>>      the cipher to derive the key and IV for.  md is the message digest to
>>      use.  The salt parameter is used as a salt in the derivation: it should
>>      point to an 8-byte buffer or NULL if no salt is used.  data is a buffer
>>      containing datal bytes which is used to derive the keying data.  count is
>>      the iteration count to use.  The derived key and IV will be written to
>>      key and iv, respectively.
> 
> So this function produces its output to the buffers pointed to by the last two arguments.
> In swupdate these are dummy_key and dummy_iv and those get discarded right after the
> call because those are just local buffers allocated on the stack. (Not to mention we are
> using the hard wired value of "DUMMY" as password.)

I think you're right.

> 
> It looks like this piece of code doesn't affect the behaviour of the actual decryption
> algorithm, so it should be discarded.

Again, you're right - thanks for the good analyses.

> 
> The swupdate documentation needs to be revisited as well.
> First, it suggests that we use the -S parameter of `openssl enc` for the encryption
> which is unnecessary as the command produces exactly the same result without this option.

This could be too simplistic. It is also plausible that the encryption
key is generated from a password, and then

> 
> ```
> $ echo abc > myfile
> $ openssl enc -aes-256-cbc -k mypassword -P -md sha1
> salt=1182D4B32E653C85
> key=F3A67C3871FEC7FF2D1772DFE0B770111A098FB53B98AEF6D69E7BFFEC90FDF1
> iv =121EE144C6BE4625CA9FBD01BC386053
> $ openssl enc -aes-256-cbc -in myfile -out enc1 \
> 	-K 20FC1F26F93E4BFADB087E45CC3F30FD4749D5D9F00989EB12BFA4DFD534381C \
> 	-iv 5389F2FCE84BFA80CC77D8C99015FD85 \
> 	-S 768AA42EC048396D
> $ openssl enc -aes-256-cbc -in myfile -out enc2 \
> 	-K 20FC1F26F93E4BFADB087E45CC3F30FD4749D5D9F00989EB12BFA4DFD534381C \
> 	-iv 5389F2FCE84BFA80CC77D8C99015FD85
> $ sha1sum enc1 enc2
> 6ca7157f81b056c04e452744ba85a12ab4b457f0  enc1
> 6ca7157f81b056c04e452744ba85a12ab4b457f0  enc2> ```
> 
> This salt parameter is only needed for the key generation from a password in the first step.
> Then, passing the salt parameter to swupdate in the key file as a third field
> is also unnecessary. The parser code of this parameter can be discarded.

Again, I think you're right. And I do not see that SWUpdate is using
this parameter because it is discarded  after calling EVP_BytesToKey()

> 
> As an additional note, we should not reuse the same key/IV pair for encrypting files/images.
> See https://crypto.stackexchange.com/a/10511

I know this, this is also in my list.

> Best would be to use a randomized IV for each encrypted file and image and include that IV
> for each ciphertext for making decryption possible.

Yes - this must be added to sw-description, too. So if "encryption =
true", iv should be added.

> IV should be used only once but doesn't
> need to be kept as a secret. Only the key should be confidential.

Right - this helps a lot, because iv can be set into sw-description.

> 
> As another side note using sha1 for forming the key and IV from the password is not a best
> practice either. Newer version of openssl warns to use a password key derivation function
> like PBKDF2. Generating key and IV can be done without using passwords at all:
> 
> ```
> key=$(dd if=/dev/random bs=32 count=1 | xxd -ps -cols 64)
> iv=$(dd if=/dev/random bs=16 count=1 | xxd -ps -cols 32)
> ```

Agree, documentation should be fixed.

> 
> My current patch only copes with the superfluous salt parameter.
> 

ok - let's see how we can go on.

Please repost the patch in a suitable way for merging - the whole
discussion here is too much for a commit message. You could add a link
to the thread on ML, instead.
Of course, not to discard everything, but just describe the problem.


> Signed-off-by: Laszlo Ashin <laszlo@ashin.hu>
> ---
>  core/cpio_utils.c               |  4 +---
>  core/util.c                     | 23 ++++-------------------
>  corelib/swupdate_decrypt.c      | 16 +---------------
>  corelib/test/test_crypt.c       | 19 ++++++-------------
>  doc/source/encrypted_images.rst | 17 +++++++----------
>  include/sslapi.h                |  4 ++--
>  include/util.h                  |  1 -
>  7 files changed, 21 insertions(+), 63 deletions(-)
> 
> diff --git a/core/cpio_utils.c b/core/cpio_utils.c
> index a464108..182ff1a 100644
> --- a/core/cpio_utils.c
> +++ b/core/cpio_utils.c
> @@ -299,7 +299,6 @@ int copyfile(int fdin, void *out, unsigned int nbytes, unsigned long *offs, unsi
>  	unsigned int md_len = 0;
>  	unsigned char *aes_key = NULL;
>  	unsigned char *ivt = NULL;
> -	unsigned char *salt;
>  
>  	struct InputState input_state = {
>  		.fdin = fdin,
> @@ -348,8 +347,7 @@ int copyfile(int fdin, void *out, unsigned int nbytes, unsigned long *offs, unsi
>  	if (encrypted) {
>  		aes_key = get_aes_key();
>  		ivt = get_aes_ivt();
> -		salt = get_aes_salt();
> -		decrypt_state.dcrypt = swupdate_DECRYPT_init(aes_key, ivt, salt);
> +		decrypt_state.dcrypt = swupdate_DECRYPT_init(aes_key, ivt);
>  		if (!decrypt_state.dcrypt) {
>  			ERROR("decrypt initialization failure, aborting");
>  			ret = -EFAULT;
> diff --git a/core/util.c b/core/util.c
> index ea4ad8a..3a72b29 100644
> --- a/core/util.c
> +++ b/core/util.c
> @@ -30,12 +30,10 @@
>  /*
>   * key  is 256 bit for aes_256
>   * ivt  is 128 bit
> - * salt is  64 bit
>   */
>  struct decryption_key {
>  	unsigned char key[32];
>  	unsigned char ivt[16];
> -	unsigned char salt[8];
>  };
>  
>  static struct decryption_key *aes_key = NULL;
> @@ -416,26 +414,22 @@ int count_elem_list(struct imglist *list)
>  int load_decryption_key(char *fname)
>  {
>  	FILE *fp;
> -	char *b1 = NULL, *b2 = NULL, *b3 = NULL;
> +	char *b1 = NULL, *b2 = NULL;
>  	int ret;
>  
>  	fp = fopen(fname, "r");
>  	if (!fp)
>  		return -EBADF;
>  
> -	ret = fscanf(fp, "%ms %ms %ms", &b1, &b2, &b3);
> +	ret = fscanf(fp, "%ms %ms", &b1, &b2);
>  	switch (ret) {
>  		case 2:
> -			b3 = NULL;
>  			DEBUG("Read decryption key and initialization vector from file %s.", fname);
>  			break;
> -		case 3:
> -			DEBUG("Read decryption key, initialization vector, and salt from file %s.", fname);
> -			break;
>  		default:
>  			if (b1 != NULL)
>  				free(b1);
> -			fprintf(stderr, "File with decryption key is not in the format <key> <ivt> nor <key> <ivt> <salt>\n");
> +			fprintf(stderr, "File with decryption key is not in the format <key> <ivt>\n");
>  			fclose(fp);
>  			return -EINVAL;
>  	}
> @@ -449,15 +443,12 @@ int load_decryption_key(char *fname)
>  		return -ENOMEM;
>  
>  	ret = ascii_to_bin(aes_key->key,  b1, sizeof(aes_key->key)  * 2) |
> -	      ascii_to_bin(aes_key->ivt,  b2, sizeof(aes_key->ivt)  * 2) |
> -	      ascii_to_bin(aes_key->salt, b3, sizeof(aes_key->salt) * 2);
> +	      ascii_to_bin(aes_key->ivt,  b2, sizeof(aes_key->ivt)  * 2);
>  
>  	if (b1 != NULL)
>  		free(b1);
>  	if (b2 != NULL)
>  		free(b2);
> -	if (b3 != NULL)
> -		free(b3);
>  
>  	if (ret) {
>  		fprintf(stderr, "Keys are invalid\n");
> @@ -479,12 +470,6 @@ unsigned char *get_aes_ivt(void) {
>  	return aes_key->ivt;
>  }
>  
> -unsigned char *get_aes_salt(void) {
> -	if (!aes_key)
> -		return NULL;
> -	return aes_key->salt;
> -}
> -
>  char** string_split(const char* in, const char d)
>  {
>  	char** result    = 0;
> diff --git a/corelib/swupdate_decrypt.c b/corelib/swupdate_decrypt.c
> index 273196e..e1043d8 100644
> --- a/corelib/swupdate_decrypt.c
> +++ b/corelib/swupdate_decrypt.c
> @@ -17,7 +17,7 @@
>  #include "sslapi.h"
>  #include "util.h"
>  
> -struct swupdate_digest *swupdate_DECRYPT_init(unsigned char *key, unsigned char *iv, unsigned char *salt)
> +struct swupdate_digest *swupdate_DECRYPT_init(unsigned char *key, unsigned char *iv)
>  {
>  	struct swupdate_digest *dgst;
>  	int ret;
> @@ -34,20 +34,6 @@ struct swupdate_digest *swupdate_DECRYPT_init(unsigned char *key, unsigned char
>  		return NULL;
>  	}
>  
> -	if (salt != NULL) {
> -		unsigned char dummy_key[EVP_MAX_KEY_LENGTH];
> -		unsigned char dummy_iv[EVP_MAX_IV_LENGTH];
> -		unsigned char dummy_pwd[5] = "DUMMY";
> -		if (!EVP_BytesToKey(cipher, EVP_sha1(), salt,
> -							dummy_pwd, sizeof(dummy_pwd),
> -							1,
> -							(unsigned char *)&dummy_key, (unsigned char *)&dummy_iv)) {
> -			ERROR("Cannot set salt.");
> -			free(dgst);
> -			return NULL;
> -		}
> -	}
> -
>  #if OPENSSL_VERSION_NUMBER < 0x10100000L || defined(LIBRESSL_VERSION_NUMBER)
>  	EVP_CIPHER_CTX_init(&dgst->ctxdec);
>  #else
> diff --git a/corelib/test/test_crypt.c b/corelib/test/test_crypt.c
> index 84ac6ed..f3b39c6 100644
> --- a/corelib/test/test_crypt.c
> +++ b/corelib/test/test_crypt.c
> @@ -28,7 +28,6 @@
>  struct cryptdata {
>  	unsigned char *key;
>  	unsigned char *iv;
> -	unsigned char *salt;
>  	unsigned char *crypttext;
>  };
>  
> @@ -44,7 +43,7 @@ static void hex2bin(unsigned char *dest, const unsigned char *source)
>  static void do_crypt(struct cryptdata *crypt, unsigned char *CRYPTTEXT, unsigned char *PLAINTEXT)
>  {
>  	int len;
> -	void *dcrypt = swupdate_DECRYPT_init(crypt->key, crypt->iv, crypt->salt);
> +	void *dcrypt = swupdate_DECRYPT_init(crypt->key, crypt->iv);
>  	assert_non_null(dcrypt);
>  
>  	unsigned char *buffer = calloc(1, strlen((const char *)CRYPTTEXT) + EVP_MAX_BLOCK_LENGTH);
> @@ -59,7 +58,7 @@ static void do_crypt(struct cryptdata *crypt, unsigned char *CRYPTTEXT, unsigned
>  	free(buffer);
>  }
>  
> -static void test_crypt_nosalt(void **state)
> +static void test_crypt_1(void **state)
>  {
>  	(void)state;
>  
> @@ -71,7 +70,6 @@ static void test_crypt_nosalt(void **state)
>  	struct cryptdata crypt;
>  	hex2bin((crypt.key = calloc(1, strlen((const char *)KEY))), KEY);
>  	hex2bin((crypt.iv = calloc(1, strlen((const char *)IV))), IV);
> -	crypt.salt = NULL;
>  	hex2bin((crypt.crypttext = calloc(1, strlen((const char *)CRYPTTEXT))), CRYPTTEXT);
>  
>  	do_crypt(&crypt, &CRYPTTEXT[0], &PLAINTEXT[0]);
> @@ -81,27 +79,24 @@ static void test_crypt_nosalt(void **state)
>  	free(crypt.crypttext);
>  }
>  
> -static void test_crypt_salt(void **state)
> +static void test_crypt_2(void **state)
>  {
>  	(void)state;
>  
>  	unsigned char KEY[] = "69D54287F856D30B51B812FDF714556778CF31E1B104D9C68BD90C669C37D1AB";
>  	unsigned char IV[] = "E7039ABFCA63EB8EB1D320F7918275B2";
> -	unsigned char SALT[] = "F75A9C11F7F63C08";
>  	unsigned char CRYPTTEXT[] = "A17EBBB1A28459352FE3A994404E35AA";
>  	unsigned char PLAINTEXT[] = "CRYPTTEST";
>  
>  	struct cryptdata crypt;
>  	hex2bin((crypt.key = calloc(1, strlen((const char *)KEY))), KEY);
>  	hex2bin((crypt.iv = calloc(1, strlen((const char *)IV))), IV);
> -	hex2bin((crypt.salt = calloc(1, strlen((const char *)SALT))), SALT);
>  	hex2bin((crypt.crypttext = calloc(1, strlen((const char *)CRYPTTEXT))), CRYPTTEXT);
>  
>  	do_crypt(&crypt, &CRYPTTEXT[0], &PLAINTEXT[0]);
>  
>  	free(crypt.key);
>  	free(crypt.iv);
> -	free(crypt.salt);
>  	free(crypt.crypttext);
>  }
>  
> @@ -116,11 +111,10 @@ static void test_crypt_failure(void **state)
>  	struct cryptdata crypt;
>  	hex2bin((crypt.key = calloc(1, strlen((const char *)KEY))), KEY);
>  	hex2bin((crypt.iv = calloc(1, strlen((const char *)IV))), IV);
> -	crypt.salt = NULL;
>  	hex2bin((crypt.crypttext = calloc(1, strlen((const char *)CRYPTTEXT))), CRYPTTEXT);
>  
>  	int len;
> -	void *dcrypt = swupdate_DECRYPT_init(crypt.key, crypt.iv, crypt.salt);
> +	void *dcrypt = swupdate_DECRYPT_init(crypt.key, crypt.iv);
>  	assert_non_null(dcrypt);
>  
>  	unsigned char *buffer = calloc(1, strlen((const char *)CRYPTTEXT) + EVP_MAX_BLOCK_LENGTH);
> @@ -131,7 +125,6 @@ static void test_crypt_failure(void **state)
>  
>  	free(crypt.key);
>  	free(crypt.iv);
> -	free(crypt.salt);
>  	free(crypt.crypttext);
>  }
>  
> @@ -139,9 +132,9 @@ int main(void)
>  {
>  	int error_count = 0;
>  	const struct CMUnitTest crypt_tests[] = {
> -		cmocka_unit_test(test_crypt_nosalt),
> +		cmocka_unit_test(test_crypt_1),
>  		cmocka_unit_test(test_crypt_failure),
> -		cmocka_unit_test(test_crypt_salt)
> +		cmocka_unit_test(test_crypt_2)
>  	};
>  	error_count += cmocka_run_group_tests_name("crypt", crypt_tests, NULL, NULL);
>  	return error_count;
> diff --git a/doc/source/encrypted_images.rst b/doc/source/encrypted_images.rst
> index 113fdd3..781f484 100644
> --- a/doc/source/encrypted_images.rst
> +++ b/doc/source/encrypted_images.rst
> @@ -29,28 +29,25 @@ Then, encrypt an image using this information via
>  
>  ::
>  
> -        openssl enc -aes-256-cbc -in <INFILE> -out <OUTFILE> -K <KEY> -iv <IV> -S <SALT>
> +        openssl enc -aes-256-cbc -in <INFILE> -out <OUTFILE> -K <KEY> -iv <IV>
>  
>  where ``<INFILE>`` is the unencrypted source image file and ``<OUTFILE>`` is the
>  encrypted output image file to be referenced in ``sw-description``.
>  ``<KEY>`` is the hex value part of the 2nd line of output from the key generation
> -command above, ``<IV>`` is the hex value part of the 3rd line, and ``<SALT>`` is
> -the hex value part of the 1st line.
> +command above and ``<IV>`` is the hex value part of the 3rd line.
>  
>  Then, create a key file to be supplied to SWUpdate via the `-K` switch by 
> -putting the key, initialization vector, and salt hex values on one line
> +putting the key and initialization vector hex values on one line
>  separated by whitespace, e.g., for above example values
>  
>  ::
>  
> -        B78CC67DD3DC13042A1B575184D4E16D6A09412C242CE253ACEE0F06B5AD68FC 65D793B87B6724BB27954C7664F15FF3 CE7B0488EFBF0D1B
> +        B78CC67DD3DC13042A1B575184D4E16D6A09412C242CE253ACEE0F06B5AD68FC 65D793B87B6724BB27954C7664F15FF3
>  
>  
> -Note that, while not recommended and for backwards compatibility, OpenSSL may be
> -used without salt. For disabling salt, add the ``-nosalt`` parameter to the key
> -generation command above. Accordingly, drop the ``-S <SALT>`` parameter in the
> -encryption command and omit the 3rd field of the key file to be supplied to
> -SWUpdate being the salt.
> +For earlier versions of SWUpdate it was falsely noted that passing the SALT as a
> +3rd parameter would increase security. Key and IV are enough for maximum security,
> +salt doesn't add any value.
>  
>  Encryption of UBI volumes
>  -------------------------
> diff --git a/include/sslapi.h b/include/sslapi.h
> index 6f03ece..c66e005 100644
> --- a/include/sslapi.h
> +++ b/include/sslapi.h
> @@ -124,7 +124,7 @@ int swupdate_HASH_compare(unsigned char *hash1, unsigned char *hash2);
>  #endif
>  
>  #ifdef CONFIG_ENCRYPTED_IMAGES
> -struct swupdate_digest *swupdate_DECRYPT_init(unsigned char *key, unsigned char *iv, unsigned char *salt);
> +struct swupdate_digest *swupdate_DECRYPT_init(unsigned char *key, unsigned char *iv);
>  int swupdate_DECRYPT_update(struct swupdate_digest *dgst, unsigned char *buf, 
>  				int *outlen, unsigned char *cryptbuf, int inlen);
>  int swupdate_DECRYPT_final(struct swupdate_digest *dgst, unsigned char *buf,
> @@ -135,7 +135,7 @@ void swupdate_DECRYPT_cleanup(struct swupdate_digest *dgst);
>   * Note: macro for swupdate_DECRYPT_init is
>   * just to avoid compiler warnings
>   */
> -#define swupdate_DECRYPT_init(key, iv, salt) (((key != NULL) | (ivt != NULL) | (salt != NULL)) ? NULL : NULL)
> +#define swupdate_DECRYPT_init(key, iv) (((key != NULL) | (ivt != NULL)) ? NULL : NULL)
>  #define swupdate_DECRYPT_update(p, buf, len, cbuf, inlen) (-1)
>  #define swupdate_DECRYPT_final(p, buf, len) (-1)
>  #define swupdate_DECRYPT_cleanup(p)
> diff --git a/include/util.h b/include/util.h
> index 445e9f2..31c97b6 100644
> --- a/include/util.h
> +++ b/include/util.h
> @@ -194,7 +194,6 @@ void free_string_array(char **nodes);
>  int load_decryption_key(char *fname);
>  unsigned char *get_aes_key(void);
>  unsigned char *get_aes_ivt(void);
> -unsigned char *get_aes_salt(void);
>  
>  /* Getting global information */
>  int get_install_info(sourcetype *source, char *buf, size_t len);
> 

Best regards,
Stefano Babic

Patch
diff mbox series

diff --git a/core/cpio_utils.c b/core/cpio_utils.c
index a464108..182ff1a 100644
--- a/core/cpio_utils.c
+++ b/core/cpio_utils.c
@@ -299,7 +299,6 @@  int copyfile(int fdin, void *out, unsigned int nbytes, unsigned long *offs, unsi
 	unsigned int md_len = 0;
 	unsigned char *aes_key = NULL;
 	unsigned char *ivt = NULL;
-	unsigned char *salt;
 
 	struct InputState input_state = {
 		.fdin = fdin,
@@ -348,8 +347,7 @@  int copyfile(int fdin, void *out, unsigned int nbytes, unsigned long *offs, unsi
 	if (encrypted) {
 		aes_key = get_aes_key();
 		ivt = get_aes_ivt();
-		salt = get_aes_salt();
-		decrypt_state.dcrypt = swupdate_DECRYPT_init(aes_key, ivt, salt);
+		decrypt_state.dcrypt = swupdate_DECRYPT_init(aes_key, ivt);
 		if (!decrypt_state.dcrypt) {
 			ERROR("decrypt initialization failure, aborting");
 			ret = -EFAULT;
diff --git a/core/util.c b/core/util.c
index ea4ad8a..3a72b29 100644
--- a/core/util.c
+++ b/core/util.c
@@ -30,12 +30,10 @@ 
 /*
  * key  is 256 bit for aes_256
  * ivt  is 128 bit
- * salt is  64 bit
  */
 struct decryption_key {
 	unsigned char key[32];
 	unsigned char ivt[16];
-	unsigned char salt[8];
 };
 
 static struct decryption_key *aes_key = NULL;
@@ -416,26 +414,22 @@  int count_elem_list(struct imglist *list)
 int load_decryption_key(char *fname)
 {
 	FILE *fp;
-	char *b1 = NULL, *b2 = NULL, *b3 = NULL;
+	char *b1 = NULL, *b2 = NULL;
 	int ret;
 
 	fp = fopen(fname, "r");
 	if (!fp)
 		return -EBADF;
 
-	ret = fscanf(fp, "%ms %ms %ms", &b1, &b2, &b3);
+	ret = fscanf(fp, "%ms %ms", &b1, &b2);
 	switch (ret) {
 		case 2:
-			b3 = NULL;
 			DEBUG("Read decryption key and initialization vector from file %s.", fname);
 			break;
-		case 3:
-			DEBUG("Read decryption key, initialization vector, and salt from file %s.", fname);
-			break;
 		default:
 			if (b1 != NULL)
 				free(b1);
-			fprintf(stderr, "File with decryption key is not in the format <key> <ivt> nor <key> <ivt> <salt>\n");
+			fprintf(stderr, "File with decryption key is not in the format <key> <ivt>\n");
 			fclose(fp);
 			return -EINVAL;
 	}
@@ -449,15 +443,12 @@  int load_decryption_key(char *fname)
 		return -ENOMEM;
 
 	ret = ascii_to_bin(aes_key->key,  b1, sizeof(aes_key->key)  * 2) |
-	      ascii_to_bin(aes_key->ivt,  b2, sizeof(aes_key->ivt)  * 2) |
-	      ascii_to_bin(aes_key->salt, b3, sizeof(aes_key->salt) * 2);
+	      ascii_to_bin(aes_key->ivt,  b2, sizeof(aes_key->ivt)  * 2);
 
 	if (b1 != NULL)
 		free(b1);
 	if (b2 != NULL)
 		free(b2);
-	if (b3 != NULL)
-		free(b3);
 
 	if (ret) {
 		fprintf(stderr, "Keys are invalid\n");
@@ -479,12 +470,6 @@  unsigned char *get_aes_ivt(void) {
 	return aes_key->ivt;
 }
 
-unsigned char *get_aes_salt(void) {
-	if (!aes_key)
-		return NULL;
-	return aes_key->salt;
-}
-
 char** string_split(const char* in, const char d)
 {
 	char** result    = 0;
diff --git a/corelib/swupdate_decrypt.c b/corelib/swupdate_decrypt.c
index 273196e..e1043d8 100644
--- a/corelib/swupdate_decrypt.c
+++ b/corelib/swupdate_decrypt.c
@@ -17,7 +17,7 @@ 
 #include "sslapi.h"
 #include "util.h"
 
-struct swupdate_digest *swupdate_DECRYPT_init(unsigned char *key, unsigned char *iv, unsigned char *salt)
+struct swupdate_digest *swupdate_DECRYPT_init(unsigned char *key, unsigned char *iv)
 {
 	struct swupdate_digest *dgst;
 	int ret;
@@ -34,20 +34,6 @@  struct swupdate_digest *swupdate_DECRYPT_init(unsigned char *key, unsigned char
 		return NULL;
 	}
 
-	if (salt != NULL) {
-		unsigned char dummy_key[EVP_MAX_KEY_LENGTH];
-		unsigned char dummy_iv[EVP_MAX_IV_LENGTH];
-		unsigned char dummy_pwd[5] = "DUMMY";
-		if (!EVP_BytesToKey(cipher, EVP_sha1(), salt,
-							dummy_pwd, sizeof(dummy_pwd),
-							1,
-							(unsigned char *)&dummy_key, (unsigned char *)&dummy_iv)) {
-			ERROR("Cannot set salt.");
-			free(dgst);
-			return NULL;
-		}
-	}
-
 #if OPENSSL_VERSION_NUMBER < 0x10100000L || defined(LIBRESSL_VERSION_NUMBER)
 	EVP_CIPHER_CTX_init(&dgst->ctxdec);
 #else
diff --git a/corelib/test/test_crypt.c b/corelib/test/test_crypt.c
index 84ac6ed..f3b39c6 100644
--- a/corelib/test/test_crypt.c
+++ b/corelib/test/test_crypt.c
@@ -28,7 +28,6 @@ 
 struct cryptdata {
 	unsigned char *key;
 	unsigned char *iv;
-	unsigned char *salt;
 	unsigned char *crypttext;
 };
 
@@ -44,7 +43,7 @@  static void hex2bin(unsigned char *dest, const unsigned char *source)
 static void do_crypt(struct cryptdata *crypt, unsigned char *CRYPTTEXT, unsigned char *PLAINTEXT)
 {
 	int len;
-	void *dcrypt = swupdate_DECRYPT_init(crypt->key, crypt->iv, crypt->salt);
+	void *dcrypt = swupdate_DECRYPT_init(crypt->key, crypt->iv);
 	assert_non_null(dcrypt);
 
 	unsigned char *buffer = calloc(1, strlen((const char *)CRYPTTEXT) + EVP_MAX_BLOCK_LENGTH);
@@ -59,7 +58,7 @@  static void do_crypt(struct cryptdata *crypt, unsigned char *CRYPTTEXT, unsigned
 	free(buffer);
 }
 
-static void test_crypt_nosalt(void **state)
+static void test_crypt_1(void **state)
 {
 	(void)state;
 
@@ -71,7 +70,6 @@  static void test_crypt_nosalt(void **state)
 	struct cryptdata crypt;
 	hex2bin((crypt.key = calloc(1, strlen((const char *)KEY))), KEY);
 	hex2bin((crypt.iv = calloc(1, strlen((const char *)IV))), IV);
-	crypt.salt = NULL;
 	hex2bin((crypt.crypttext = calloc(1, strlen((const char *)CRYPTTEXT))), CRYPTTEXT);
 
 	do_crypt(&crypt, &CRYPTTEXT[0], &PLAINTEXT[0]);
@@ -81,27 +79,24 @@  static void test_crypt_nosalt(void **state)
 	free(crypt.crypttext);
 }
 
-static void test_crypt_salt(void **state)
+static void test_crypt_2(void **state)
 {
 	(void)state;
 
 	unsigned char KEY[] = "69D54287F856D30B51B812FDF714556778CF31E1B104D9C68BD90C669C37D1AB";
 	unsigned char IV[] = "E7039ABFCA63EB8EB1D320F7918275B2";
-	unsigned char SALT[] = "F75A9C11F7F63C08";
 	unsigned char CRYPTTEXT[] = "A17EBBB1A28459352FE3A994404E35AA";
 	unsigned char PLAINTEXT[] = "CRYPTTEST";
 
 	struct cryptdata crypt;
 	hex2bin((crypt.key = calloc(1, strlen((const char *)KEY))), KEY);
 	hex2bin((crypt.iv = calloc(1, strlen((const char *)IV))), IV);
-	hex2bin((crypt.salt = calloc(1, strlen((const char *)SALT))), SALT);
 	hex2bin((crypt.crypttext = calloc(1, strlen((const char *)CRYPTTEXT))), CRYPTTEXT);
 
 	do_crypt(&crypt, &CRYPTTEXT[0], &PLAINTEXT[0]);
 
 	free(crypt.key);
 	free(crypt.iv);
-	free(crypt.salt);
 	free(crypt.crypttext);
 }
 
@@ -116,11 +111,10 @@  static void test_crypt_failure(void **state)
 	struct cryptdata crypt;
 	hex2bin((crypt.key = calloc(1, strlen((const char *)KEY))), KEY);
 	hex2bin((crypt.iv = calloc(1, strlen((const char *)IV))), IV);
-	crypt.salt = NULL;
 	hex2bin((crypt.crypttext = calloc(1, strlen((const char *)CRYPTTEXT))), CRYPTTEXT);
 
 	int len;
-	void *dcrypt = swupdate_DECRYPT_init(crypt.key, crypt.iv, crypt.salt);
+	void *dcrypt = swupdate_DECRYPT_init(crypt.key, crypt.iv);
 	assert_non_null(dcrypt);
 
 	unsigned char *buffer = calloc(1, strlen((const char *)CRYPTTEXT) + EVP_MAX_BLOCK_LENGTH);
@@ -131,7 +125,6 @@  static void test_crypt_failure(void **state)
 
 	free(crypt.key);
 	free(crypt.iv);
-	free(crypt.salt);
 	free(crypt.crypttext);
 }
 
@@ -139,9 +132,9 @@  int main(void)
 {
 	int error_count = 0;
 	const struct CMUnitTest crypt_tests[] = {
-		cmocka_unit_test(test_crypt_nosalt),
+		cmocka_unit_test(test_crypt_1),
 		cmocka_unit_test(test_crypt_failure),
-		cmocka_unit_test(test_crypt_salt)
+		cmocka_unit_test(test_crypt_2)
 	};
 	error_count += cmocka_run_group_tests_name("crypt", crypt_tests, NULL, NULL);
 	return error_count;
diff --git a/doc/source/encrypted_images.rst b/doc/source/encrypted_images.rst
index 113fdd3..781f484 100644
--- a/doc/source/encrypted_images.rst
+++ b/doc/source/encrypted_images.rst
@@ -29,28 +29,25 @@  Then, encrypt an image using this information via
 
 ::
 
-        openssl enc -aes-256-cbc -in <INFILE> -out <OUTFILE> -K <KEY> -iv <IV> -S <SALT>
+        openssl enc -aes-256-cbc -in <INFILE> -out <OUTFILE> -K <KEY> -iv <IV>
 
 where ``<INFILE>`` is the unencrypted source image file and ``<OUTFILE>`` is the
 encrypted output image file to be referenced in ``sw-description``.
 ``<KEY>`` is the hex value part of the 2nd line of output from the key generation
-command above, ``<IV>`` is the hex value part of the 3rd line, and ``<SALT>`` is
-the hex value part of the 1st line.
+command above and ``<IV>`` is the hex value part of the 3rd line.
 
 Then, create a key file to be supplied to SWUpdate via the `-K` switch by 
-putting the key, initialization vector, and salt hex values on one line
+putting the key and initialization vector hex values on one line
 separated by whitespace, e.g., for above example values
 
 ::
 
-        B78CC67DD3DC13042A1B575184D4E16D6A09412C242CE253ACEE0F06B5AD68FC 65D793B87B6724BB27954C7664F15FF3 CE7B0488EFBF0D1B
+        B78CC67DD3DC13042A1B575184D4E16D6A09412C242CE253ACEE0F06B5AD68FC 65D793B87B6724BB27954C7664F15FF3
 
 
-Note that, while not recommended and for backwards compatibility, OpenSSL may be
-used without salt. For disabling salt, add the ``-nosalt`` parameter to the key
-generation command above. Accordingly, drop the ``-S <SALT>`` parameter in the
-encryption command and omit the 3rd field of the key file to be supplied to
-SWUpdate being the salt.
+For earlier versions of SWUpdate it was falsely noted that passing the SALT as a
+3rd parameter would increase security. Key and IV are enough for maximum security,
+salt doesn't add any value.
 
 Encryption of UBI volumes
 -------------------------
diff --git a/include/sslapi.h b/include/sslapi.h
index 6f03ece..c66e005 100644
--- a/include/sslapi.h
+++ b/include/sslapi.h
@@ -124,7 +124,7 @@  int swupdate_HASH_compare(unsigned char *hash1, unsigned char *hash2);
 #endif
 
 #ifdef CONFIG_ENCRYPTED_IMAGES
-struct swupdate_digest *swupdate_DECRYPT_init(unsigned char *key, unsigned char *iv, unsigned char *salt);
+struct swupdate_digest *swupdate_DECRYPT_init(unsigned char *key, unsigned char *iv);
 int swupdate_DECRYPT_update(struct swupdate_digest *dgst, unsigned char *buf, 
 				int *outlen, unsigned char *cryptbuf, int inlen);
 int swupdate_DECRYPT_final(struct swupdate_digest *dgst, unsigned char *buf,
@@ -135,7 +135,7 @@  void swupdate_DECRYPT_cleanup(struct swupdate_digest *dgst);
  * Note: macro for swupdate_DECRYPT_init is
  * just to avoid compiler warnings
  */
-#define swupdate_DECRYPT_init(key, iv, salt) (((key != NULL) | (ivt != NULL) | (salt != NULL)) ? NULL : NULL)
+#define swupdate_DECRYPT_init(key, iv) (((key != NULL) | (ivt != NULL)) ? NULL : NULL)
 #define swupdate_DECRYPT_update(p, buf, len, cbuf, inlen) (-1)
 #define swupdate_DECRYPT_final(p, buf, len) (-1)
 #define swupdate_DECRYPT_cleanup(p)
diff --git a/include/util.h b/include/util.h
index 445e9f2..31c97b6 100644
--- a/include/util.h
+++ b/include/util.h
@@ -194,7 +194,6 @@  void free_string_array(char **nodes);
 int load_decryption_key(char *fname);
 unsigned char *get_aes_key(void);
 unsigned char *get_aes_ivt(void);
-unsigned char *get_aes_salt(void);
 
 /* Getting global information */
 int get_install_info(sourcetype *source, char *buf, size_t len);