Re: [PATCH v3 1/7] storage: implement network profile encryption

Re: [PATCH v3 1/7] storage: implement network profile encryption

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 1872 bytes --]

Hi Joseph,

On Fri, 2022-02-04 at 15:55 -0700, Joseph Benden wrote:
> Hi!
> 
> It is my understanding that all secret data is to be kept as securely
> as 
> possible. I'm not really seeing this carried throughout the source
> code. 
> Maybe this specific code does not need that level of security?

Currently any profile (e.g. /var/lib/iwd/foo.psk) containing secret
data is unencypted, plaintext. We rely on the file system permissions
to 'secure' this data (which this patch series is 'fixing' for anyone
who wants these encrypted).

> 
> If I'm right, then:
> 
> The secret data should be kept in a mmap(), and mlock(), structure.
> The 
> prevents secret data from hitting swap memory. After every use of the
> secret data; it must be explicit_bzero() and then free(). This is so 
> critical and tedious that it should be a macro or something from
> libell. 
> And those are just initial thoughts for the basic safety of the
> secret data.

So this is how the systemd feature works: The IWD service is given a
path to a file. This file is decrypted by systemd and contains the
'secret'. IWD then reads this secret in, does some hashing, and stores
the hashed secret in RAM. Once IWD is done it never accesses the file
again.

So I don't think its even possible for IWD to do this mmap/mlock dance
you're describing. It really doesn't own this secret file, nor does it
keep the file contents around long. It just derives a fixed length key
and never uses the file again.

As far as explicit_bzero'ing the hashed secret after every use, then
reading it back in, and hashing it again when needed... this seems a
bit crazy :)

Thanks,
James

> 
> Best regards,
> -Joe
> _______________________________________________
> iwd mailing list -- iwd(a)lists.01.org
> To unsubscribe send an email to iwd-leave(a)lists.01.org

Re: [PATCH v3 1/7] storage: implement network profile encryption

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 3774 bytes --]

Hi Joseph,

On 2/4/22 18:52, Joseph Benden wrote:
> On 2/4/22 4:23 PM, James Prestwood wrote:
>> Hi Joseph,
>>
>> On Fri, 2022-02-04 at 15:55 -0700, Joseph Benden wrote:
>>> Hi!
>>>
>>> It is my understanding that all secret data is to be kept as securely
>>> as
>>> possible. I'm not really seeing this carried throughout the source
>>> code.
>>> Maybe this specific code does not need that level of security?
>> Currently any profile (e.g. /var/lib/iwd/foo.psk) containing secret
>> data is unencypted, plaintext. We rely on the file system permissions
>> to 'secure' this data (which this patch series is 'fixing' for anyone
>> who wants these encrypted).
> 
> Icky... really shouldn't do it this way; secure all data at rest - even against 
> root account, if at all possible.
> 

There are lots of interesting ideas in this thread... I'm not aware of any 
networking daemons actually doing any of the stuff you describe here.  iwd could 
be the first of course.

> 
> So this is a can of worms... As soon as the secret data hits disk, it is 
> out-of-ones own control; considered revealed and/or exploited. I sure do hope 
> systemd does the right things here... (facepalm)

Well, you may want to check what systemd is doing first.  I suspect the data 
never actually hits disk, but goes into tmpfs.

> 
>>
>> As far as explicit_bzero'ing the hashed secret after every use, then
>> reading it back in, and hashing it again when needed... this seems a
>> bit crazy :)
> 
> Maybe crazy, most certainly needed. If not now, probably needed in the near 
> future (after some horrible event).
> 
> So let's talk about data flows for a moment.
> 
> IWD service starts and initializes a secret data vault. This is a region of 
> memory which is mmap()-ed, mlock() protected, and which happens to point to the 
> start of a fixed struct - that is for IWD service's secret data store. [I 
> envision libell having support for application secret vaults]
> 
> The external secret data enters from a file; it is read to the secret's vault, 
> and is securely processed to have the derived secret data (these two values are 
> in the IWD service's secret data store).
> 

Sounds useful.  Any chance you would be willing to contribute?

> Providing compatible with systemd's process; the secret data file is shredded 
> (see DoD -style procedures for examples), and unlink()-ed from the file-system. 
> Also (shred) explicit_bzero()-ing the filenames used and any and all memory used 
> that is not inside of the IWD service's secret data store. [I envision libell 
> having a file_shred() function, using the 6/8-pass (iirc) or so methods.]

See above.  I doubt this is needed.  But even if systemd somehow writes the 
secret to disk, it is not up to iwd to destroy it.

> 
> Now, when the secret derived data is needed for incoming/outgoing frames; it is 
> always used directly from the service's secret data store. Try your darn-ist to 
> not copy it out of its place in the vault, and if it is: it must be securely 
> used and tracked for correctness.

Again, this sounds useful and could be fairly easily accomplished for many of 
the common scenarios.  For example, most of the derived keys for the current 
connection are already stored in a single object and such memory could be locked 
if desired.  But this is a separate topic from what this patch set is trying to 
accomplish.  Feel free to start a new thread on this topic specifically.

> 
> Finally at program shutdown, the IWD service's secret data store is shredded 
> (this is explicit_bzero()-ed), then munlock(), and munmap()-ed.
> 

See above.  systemd owns the secret, so there's nothing for iwd to do here.

Regards,
-Denis

Re: [PATCH v3 1/7] storage: implement network profile encryption

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 5235 bytes --]

Hi Joe,

On Fri, 2022-02-04 at 17:52 -0700, Joseph Benden wrote:
> On 2/4/22 4:23 PM, James Prestwood wrote:
> > Hi Joseph,
> > 
> > On Fri, 2022-02-04 at 15:55 -0700, Joseph Benden wrote:
> > > Hi!
> > > 
> > > It is my understanding that all secret data is to be kept as
> > > securely
> > > as
> > > possible. I'm not really seeing this carried throughout the
> > > source
> > > code.
> > > Maybe this specific code does not need that level of security?
> > Currently any profile (e.g. /var/lib/iwd/foo.psk) containing secret
> > data is unencypted, plaintext. We rely on the file system
> > permissions
> > to 'secure' this data (which this patch series is 'fixing' for
> > anyone
> > who wants these encrypted).
> 
> Icky... really shouldn't do it this way; secure all data at rest -
> even 
> against root account, if at all possible.
> 
> > 
> > > If I'm right, then:
> > > 
> > > The secret data should be kept in a mmap(), and mlock(),
> > > structure.
> > > The
> > > prevents secret data from hitting swap memory. After every use of
> > > the
> > > secret data; it must be explicit_bzero() and then free(). This is
> > > so
> > > critical and tedious that it should be a macro or something from
> > > libell.
> > > And those are just initial thoughts for the basic safety of the
> > > secret data.
> > So this is how the systemd feature works: The IWD service is given
> > a
> > path to a file. This file is decrypted by systemd and contains the
> > 'secret'. IWD then reads this secret in, does some hashing, and
> > stores
> > the hashed secret in RAM. Once IWD is done it never accesses the
> > file
> > again.
> > 
> > So I don't think its even possible for IWD to do this mmap/mlock
> > dance
> > you're describing. It really doesn't own this secret file, nor does
> > it
> > keep the file contents around long. It just derives a fixed length
> > key
> > and never uses the file again.
> 
> So this is a can of worms... As soon as the secret data hits disk, it
> is 
> out-of-ones own control; considered revealed and/or exploited. I sure
> do 
> hope systemd does the right things here... (facepalm)
> 
> > 
> > As far as explicit_bzero'ing the hashed secret after every use,
> > then
> > reading it back in, and hashing it again when needed... this seems
> > a
> > bit crazy :)
> 
> Maybe crazy, most certainly needed. If not now, probably needed in
> the 
> near future (after some horrible event).
> 
> So let's talk about data flows for a moment.
> 
> IWD service starts and initializes a secret data vault. This is a
> region 
> of memory which is mmap()-ed, mlock() protected, and which happens to
> point to the start of a fixed struct - that is for IWD service's
> secret 
> data store. [I envision libell having support for application secret
> vaults]
> 
> The external secret data enters from a file; it is read to the
> secret's 
> vault, and is securely processed to have the derived secret data
> (these 
> two values are in the IWD service's secret data store).
> 
> Providing compatible with systemd's process; the secret data file is 
> shredded (see DoD -style procedures for examples), and unlink()-ed
> from 
> the file-system. Also (shred) explicit_bzero()-ing the filenames used
> and any and all memory used that is not inside of the IWD service's 
> secret data store. [I envision libell having a file_shred() function,
> using the 6/8-pass (iirc) or so methods.]
> 
> Now, when the secret derived data is needed for incoming/outgoing 
> frames; it is always used directly from the service's secret data
> store. 
> Try your darn-ist to not copy it out of its place in the vault, and
> if 
> it is: it must be securely used and tracked for correctness.
> 
> Finally at program shutdown, the IWD service's secret data store is 
> shredded (this is explicit_bzero()-ed), then munlock(), and munmap()-
> ed.

I totally understand but we do have to balance what is actually
feasible to do right now. At least with how this systemd feature works
the above isn't possible at least from IWD's perspective.

It also comes down the work vs benefit and how far we are willing to go
to protect these "secrets", which are wifi credentials. If someone has
root access on your box its all over, leaking your wifi password isn't
going to change much. Not trying to downplay the importance of
security, but this is my honest take on it.

I see your point from an enterprise security perspective, and maybe
this is something we need to look into in the future but for now we
just gotta do what we can. Encrypting credentials with systemd is
certainly better than plaintext. Also this feature works with TPM so
even better (but yes, they way systemd did it still write a file to
disk).

For a person who absolutely cannot store credentials on disk you have
the option to not write any secrets to the profile, i.e. enter your
passphrase/username etc each time you connect.

Thanks,
James


> 
> Best regards,
> -Joe
> 
> _______________________________________________
> iwd mailing list -- iwd(a)lists.01.org
> To unsubscribe send an email to iwd-leave(a)lists.01.org

Re: [PATCH v3 1/7] storage: implement network profile encryption

[Joseph Benden]

[-- Attachment #1: Type: text/plain, Size: 3611 bytes --]

On 2/4/22 4:23 PM, James Prestwood wrote:
> Hi Joseph,
>
> On Fri, 2022-02-04 at 15:55 -0700, Joseph Benden wrote:
>> Hi!
>>
>> It is my understanding that all secret data is to be kept as securely
>> as
>> possible. I'm not really seeing this carried throughout the source
>> code.
>> Maybe this specific code does not need that level of security?
> Currently any profile (e.g. /var/lib/iwd/foo.psk) containing secret
> data is unencypted, plaintext. We rely on the file system permissions
> to 'secure' this data (which this patch series is 'fixing' for anyone
> who wants these encrypted).

Icky... really shouldn't do it this way; secure all data at rest - even 
against root account, if at all possible.

>
>> If I'm right, then:
>>
>> The secret data should be kept in a mmap(), and mlock(), structure.
>> The
>> prevents secret data from hitting swap memory. After every use of the
>> secret data; it must be explicit_bzero() and then free(). This is so
>> critical and tedious that it should be a macro or something from
>> libell.
>> And those are just initial thoughts for the basic safety of the
>> secret data.
> So this is how the systemd feature works: The IWD service is given a
> path to a file. This file is decrypted by systemd and contains the
> 'secret'. IWD then reads this secret in, does some hashing, and stores
> the hashed secret in RAM. Once IWD is done it never accesses the file
> again.
>
> So I don't think its even possible for IWD to do this mmap/mlock dance
> you're describing. It really doesn't own this secret file, nor does it
> keep the file contents around long. It just derives a fixed length key
> and never uses the file again.

So this is a can of worms... As soon as the secret data hits disk, it is 
out-of-ones own control; considered revealed and/or exploited. I sure do 
hope systemd does the right things here... (facepalm)

>
> As far as explicit_bzero'ing the hashed secret after every use, then
> reading it back in, and hashing it again when needed... this seems a
> bit crazy :)

Maybe crazy, most certainly needed. If not now, probably needed in the 
near future (after some horrible event).

So let's talk about data flows for a moment.

IWD service starts and initializes a secret data vault. This is a region 
of memory which is mmap()-ed, mlock() protected, and which happens to 
point to the start of a fixed struct - that is for IWD service's secret 
data store. [I envision libell having support for application secret vaults]

The external secret data enters from a file; it is read to the secret's 
vault, and is securely processed to have the derived secret data (these 
two values are in the IWD service's secret data store).

Providing compatible with systemd's process; the secret data file is 
shredded (see DoD -style procedures for examples), and unlink()-ed from 
the file-system. Also (shred) explicit_bzero()-ing the filenames used 
and any and all memory used that is not inside of the IWD service's 
secret data store. [I envision libell having a file_shred() function, 
using the 6/8-pass (iirc) or so methods.]

Now, when the secret derived data is needed for incoming/outgoing 
frames; it is always used directly from the service's secret data store. 
Try your darn-ist to not copy it out of its place in the vault, and if 
it is: it must be securely used and tracked for correctness.

Finally at program shutdown, the IWD service's secret data store is 
shredded (this is explicit_bzero()-ed), then munlock(), and munmap()-ed.

Best regards,
-Joe

Re: [PATCH v3 1/7] storage: implement network profile encryption

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 2226 bytes --]

On Fri, 2022-02-04 at 15:23 -0800, James Prestwood wrote:
> Hi Joseph,
> 
> On Fri, 2022-02-04 at 15:55 -0700, Joseph Benden wrote:
> > Hi!
> > 
> > It is my understanding that all secret data is to be kept as
> > securely
> > as 
> > possible. I'm not really seeing this carried throughout the source
> > code. 
> > Maybe this specific code does not need that level of security?
> 
> Currently any profile (e.g. /var/lib/iwd/foo.psk) containing secret
> data is unencypted, plaintext. We rely on the file system permissions
> to 'secure' this data (which this patch series is 'fixing' for anyone
> who wants these encrypted).
> 
> > 
> > If I'm right, then:
> > 
> > The secret data should be kept in a mmap(), and mlock(), structure.
> > The 
> > prevents secret data from hitting swap memory. After every use of
> > the
> > secret data; it must be explicit_bzero() and then free(). This is
> > so 
> > critical and tedious that it should be a macro or something from
> > libell. 
> > And those are just initial thoughts for the basic safety of the
> > secret data.
> 
> So this is how the systemd feature works: The IWD service is given a
> path to a file. This file is decrypted by systemd and contains the
> 'secret'. IWD then reads this secret in, does some hashing, and
> stores
> the hashed secret in RAM. Once IWD is done it never accesses the file
> again.
> 
> So I don't think its even possible for IWD to do this mmap/mlock
> dance
> you're describing. It really doesn't own this secret file, nor does
> it
> keep the file contents around long. It just derives a fixed length
> key
> and never uses the file again.

Sorry I misunderstood. We could do this (the mmap/mlock part) but as
far as doing it every time we need the secret, I don't see that being
feasible.

> 
> As far as explicit_bzero'ing the hashed secret after every use, then
> reading it back in, and hashing it again when needed... this seems a
> bit crazy :)
> 
> Thanks,
> James
> 
> > 
> > Best regards,
> > -Joe
> > _______________________________________________
> > iwd mailing list -- iwd(a)lists.01.org
> > To unsubscribe send an email to iwd-leave(a)lists.01.org
> 
> 

Re: [PATCH v3 1/7] storage: implement network profile encryption

[Joseph Benden]

[-- Attachment #1: Type: text/plain, Size: 990 bytes --]

On 2/4/22 3:55 PM, Joseph Benden wrote:
> Hi!
>
> It is my understanding that all secret data is to be kept as securely 
> as possible. I'm not really seeing this carried throughout the source 
> code. Maybe this specific code does not need that level of security?
>
> If I'm right, then:
>
> The secret data should be kept in a mmap(), and mlock(), structure. 
> The prevents secret data from hitting swap memory. After every use of 
> the secret data; it must be explicit_bzero() and then free(). This is 
> so critical and tedious that it should be a macro or something from 
> libell. And those are just initial thoughts for the basic safety of 
> the secret data.
>
Also copies must be mlock()'d as well. Also minimize the number of 
copies, due to all the overhead needed on each one.
> Best regards,
> -Joe
> _______________________________________________
> iwd mailing list -- iwd(a)lists.01.org
> To unsubscribe send an email to iwd-leave(a)lists.01.org
>

Re: [PATCH v3 1/7] storage: implement network profile encryption

[Joseph Benden]

[-- Attachment #1: Type: text/plain, Size: 639 bytes --]

Hi!

It is my understanding that all secret data is to be kept as securely as 
possible. I'm not really seeing this carried throughout the source code. 
Maybe this specific code does not need that level of security?

If I'm right, then:

The secret data should be kept in a mmap(), and mlock(), structure. The 
prevents secret data from hitting swap memory. After every use of the 
secret data; it must be explicit_bzero() and then free(). This is so 
critical and tedious that it should be a macro or something from libell. 
And those are just initial thoughts for the basic safety of the secret data.

Best regards,
-Joe

Re: [PATCH v3 1/7] storage: implement network profile encryption

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 8430 bytes --]

Hi James,

On 2/3/22 15:23, James Prestwood wrote:
> Some users don't like the idea of storing network credentials in plaintext
> on the file system. As far as IWD goes the credential directory permissions
> should be set up such that nobody but the owner/root have access but
> nevertheless they are still plaintext.
> 
> For added security these credentials can be encrypted using a secret key.
> In this patch the origination of the key does not matter, but in this
> patch series IWD will support a systemd provided key.
> 
> The encryption itself will operate on the entire [Security] group as well
> as all embedded groups. Once encrypted the [Security] group will be replaced
> with two key/values:
> 
> EncryptedSalt - A random string of bytes used for the encryption
> EncryptedSecurity - A string of bytes containing the encrypted [Security]
>                      group, as well as all embedded groups.
> 
> After the profile has been encrypted these values should not be modified.
> Note that any values added to [Security] after encryption has no effect.
> Once the profile is encrypted there is no way to modify [Security] without
> manually decrypting first, or just removing it entirely which effectively
> treated a 'new' profile.
> 
> The encryption/decryption is done using AES-SIV with a salt value and the
> network SSID as the IV.
> 
> Once a key is set any profiles opened will automatically be encrypted and
> re-written to disk. Modules using network_storage_open will be provided
> the decrypted profile, and will be unaware it was ever encrypted in the
> first place. Similarly when network_storage_sync is called the profile
> will by automatically encrypted and written to disk without the caller
> needing to do anything special.
> 
> A few private storage.c helpers were added to serve several purposes:
> 
> __storage_set_encryption_key():
> This sets the encryption key direct from systemd then uses extract and expand
> to create a new fixed length key to do encryption/decryption.
> 
> __storage_decrypt():
> Low level API to decrypt an l_settings object using a previously set key and
> the SSID/name for the network. This returns a 'changed' out parameter signifying
> that the settings need to be encrypted and re-written to disk. The purpose of
> exposing this is for a standalone decryption tool which does not re-write any
> settings.
> 
> __storage_open():
> Wrapper around __storage_decrypt() that handles re-writing a new profile to
> disk. This was exposed in order to support hotspot profiles.
> 
> __storage_encrypt():
> Encrypts an l_settings object and returns the full profile as data
> ---
>   Makefile.am   |   3 +-
>   src/storage.c | 268 +++++++++++++++++++++++++++++++++++++++++++++++---
>   src/storage.h |   8 ++
>   3 files changed, 267 insertions(+), 12 deletions(-)
> 
> v3:
>   * Created and exposed __storage_open for use in hotspot.
>   * Use extract-and-expand APIs to create encryption key
> 

This is looking pretty good to me now.  Just a few minor points:

> +/*
> + * Encrypt needed groups of 'settings' without modifying the object. Returns
> + * the entire settings object as data, with encrypted groups as a bytestring
> + * set as the value to [Security].EncryptedSecurity. This also includes any
> + * embedded groups.
> + *
> + * Note: If encryption is not enabled or there is no Security group this is
> + *       effectively l_settings_to_data.
> + */
> +char *__storage_encrypt(struct l_settings *settings, const char *ssid,

should the settings object be const?

> +				size_t *out_len)
> +{
> +	struct iovec ad[2];
> +	uint8_t salt[32];
> +	size_t len;
> +	_auto_(l_free) struct l_settings *to_encrypt = NULL;
> +	_auto_(l_free) struct l_settings *original = NULL;

I think you mean l_settings_free here.

> +	_auto_(l_free) char *plaintext = NULL;
> +	_auto_(l_free) uint8_t *enc = NULL;
> +	_auto_(l_strv_free) char **groups = NULL;
> +	char **i;
> +
> +	if (!system_key_set || !l_settings_has_group(settings, "Security"))
> +		return l_settings_to_data(settings, out_len);
> +
> +	/*
> +	 * Make two copies of the settings: One will contain only data to be
> +	 * encrypted (to_encrypt), the other will contain data to be left
> +	 * unencrypted (original). At the end any encrypted data will be set
> +	 * into 'original' as EncryptedSecurity.
> +	 */
> +	to_encrypt = l_settings_clone(settings);
> +	original = l_settings_clone(settings);
> +
> +	groups = l_settings_get_groups(to_encrypt);
> +	for (i = groups; *i; i++) {
> +		if (encrypt_group(*i))
> +			l_settings_remove_group(original, *i);
> +		else
> +			l_settings_remove_group(to_encrypt, *i);
> +	}
> +
> +	l_settings_remove_embedded_groups(original);
> +
> +	plaintext = l_settings_to_data(to_encrypt, &len);
> +	if (!plaintext)
> +		return NULL;
> +
> +	l_getrandom(salt, 32);
> +
> +	ad[0].iov_base = (void *) salt;
> +	ad[0].iov_len = 32;
> +	ad[1].iov_base = (void *) ssid;
> +	ad[1].iov_len = strlen(ssid);
> +
> +	enc = l_malloc(len + 16);
> +
> +	if (!aes_siv_encrypt(system_key, sizeof(system_key), plaintext, len,
> +				ad, 2, enc)) {
> +		l_error("Could not encrypt [Security] group");
> +		return NULL;
> +	}
> +
> +	l_settings_set_bytes(original, "Security", "EncryptedSalt", salt, 32);
> +	l_settings_set_bytes(original, "Security", "EncryptedSecurity",
> +				enc, len + 16);
> +
> +	return l_settings_to_data(original, out_len);
> +}
> +
> +/*
> + * Decrypt data in [Security].EncryptedSecurity. This data also includes
> + * embedded groups potentially. Once decrypted the data is put back into the
> + * object.
> + *
> + * Note: if encryption is not enabled or there is no Security group settings
> + *       is not modified.
> + */
> +int __storage_decrypt(struct l_settings *settings, const char *ssid,
> +				bool *changed)
> +{
> +	_auto_(l_settings_free) struct l_settings *security = NULL;
> +	_auto_(l_free) uint8_t *encrypted = NULL;
> +	_auto_(l_free) uint8_t *decrypted = NULL;
> +	_auto_(l_free) uint8_t *salt = NULL;
> +	_auto_(l_strv_free) char **embedded = NULL;
> +	size_t elen, slen;
> +	struct iovec ad[2];
> +
> +	if (!system_key_set)
> +		goto done;
> +
> +	if (!l_settings_has_group(settings, "Security"))
> +		goto done;
> +
> +	encrypted = l_settings_get_bytes(settings, "Security",
> +						"EncryptedSecurity", &elen);
> +	salt = l_settings_get_bytes(settings, "Security",
> +						"EncryptedSalt", &slen);
> +
> +	/*
> +	 * Either profile has never been loaded after enabling encryption or is
> +	 * missing Encrypted{Salt,Security} values. If either are missing this
> +	 * profile is corrupted and must be fixed.
> +	 */
> +	if (!(encrypted && salt)) {
> +		/* Profile corrupted */
> +		if (encrypted || salt) {
> +			l_warn("Profile %s is corrupted reconfigure manually",
> +					ssid);
> +			return -EBADMSG;
> +		}
> +
> +		if (changed)
> +			*changed = true;
> +
> +		return 0;
> +	}
> +
> +	decrypted = l_malloc(elen - 16 + 1);
> +
> +	ad[0].iov_base = (void *)salt;
> +	ad[0].iov_len = slen;
> +	ad[1].iov_base = (void *)ssid;
> +	ad[1].iov_len = strlen(ssid);
> +
> +	if (!aes_siv_decrypt(system_key, sizeof(system_key), encrypted, elen,
> +				ad, 2, decrypted)) {
> +		l_error("Could not decrypt %s profile, did the secret change?",
> +				ssid);
> +		return -ENOKEY;
> +	}
> +
> +	decrypted[elen - 16] = '\0';
> +
> +	/* Remove encrypted [Security], and copy the decrypted one */
> +	l_settings_remove_group(settings, "Security");

Do you want to replace this with a for loop removing all groups listed in the 
encrypt_groups array?

Should you also be removing any embedded groups just in case?

> +
> +	/*
> +	 * Load decrypted data into existing settings. This is not how the API
> +	 * is indended to be used (since this could result in duplicate groups)
> +	 * but since the Security group was just removed and EncryptedSecurity
> +	 * should only contain a Security group its safe to use it this way.
> +	 */
> +	if (!l_settings_load_from_data(settings, (const char *) decrypted,
> +					elen - 16)) {
> +		l_error("Could not load decrypted security group");
> +		return -EBADMSG;
> +	}
> +
> +done:
> +	if (changed)
> +		*changed = false;
> +
> +	return 0;
> +}
> +

Regards,
-Denis

[PATCH v3 1/7] storage: implement network profile encryption

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 12924 bytes --]

Some users don't like the idea of storing network credentials in plaintext
on the file system. As far as IWD goes the credential directory permissions
should be set up such that nobody but the owner/root have access but
nevertheless they are still plaintext.

For added security these credentials can be encrypted using a secret key.
In this patch the origination of the key does not matter, but in this
patch series IWD will support a systemd provided key.

The encryption itself will operate on the entire [Security] group as well
as all embedded groups. Once encrypted the [Security] group will be replaced
with two key/values:

EncryptedSalt - A random string of bytes used for the encryption
EncryptedSecurity - A string of bytes containing the encrypted [Security]
                    group, as well as all embedded groups.

After the profile has been encrypted these values should not be modified.
Note that any values added to [Security] after encryption has no effect.
Once the profile is encrypted there is no way to modify [Security] without
manually decrypting first, or just removing it entirely which effectively
treated a 'new' profile.

The encryption/decryption is done using AES-SIV with a salt value and the
network SSID as the IV.

Once a key is set any profiles opened will automatically be encrypted and
re-written to disk. Modules using network_storage_open will be provided
the decrypted profile, and will be unaware it was ever encrypted in the
first place. Similarly when network_storage_sync is called the profile
will by automatically encrypted and written to disk without the caller
needing to do anything special.

A few private storage.c helpers were added to serve several purposes:

__storage_set_encryption_key():
This sets the encryption key direct from systemd then uses extract and expand
to create a new fixed length key to do encryption/decryption.

__storage_decrypt():
Low level API to decrypt an l_settings object using a previously set key and
the SSID/name for the network. This returns a 'changed' out parameter signifying
that the settings need to be encrypted and re-written to disk. The purpose of
exposing this is for a standalone decryption tool which does not re-write any
settings.

__storage_open():
Wrapper around __storage_decrypt() that handles re-writing a new profile to
disk. This was exposed in order to support hotspot profiles.

__storage_encrypt():
Encrypts an l_settings object and returns the full profile as data
---
 Makefile.am   |   3 +-
 src/storage.c | 268 +++++++++++++++++++++++++++++++++++++++++++++++---
 src/storage.h |   8 ++
 3 files changed, 267 insertions(+), 12 deletions(-)

v3:
 * Created and exposed __storage_open for use in hotspot.
 * Use extract-and-expand APIs to create encryption key

diff --git a/Makefile.am b/Makefile.am
index 89c053a6..35938d22 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -381,7 +381,8 @@ tools_hwsim_SOURCES = tools/hwsim.c src/mpdu.h \
 					src/nl80211util.h src/nl80211util.c \
 					src/storage.h src/storage.c \
 					src/common.h src/common.c \
-					src/band.h src/band.c
+					src/band.h src/band.c \
+					src/crypto.h src/crypto.c
 tools_hwsim_LDADD = $(ell_ldadd)
 
 if DBUS_POLICY
diff --git a/src/storage.c b/src/storage.c
index 4b89c615..026236d0 100644
--- a/src/storage.c
+++ b/src/storage.c
@@ -41,9 +41,11 @@
 #include <sys/stat.h>
 
 #include <ell/ell.h>
+#include "ell/useful.h"
 
 #include "src/common.h"
 #include "src/storage.h"
+#include "src/crypto.h"
 
 #define STORAGE_DIR_MODE (S_IRUSR | S_IWUSR | S_IXUSR)
 #define STORAGE_FILE_MODE (S_IRUSR | S_IWUSR)
@@ -52,6 +54,8 @@
 
 static char *storage_path = NULL;
 static char *storage_hotspot_path = NULL;
+static uint8_t system_key[32];
+static bool system_key_set = false;
 
 static int create_dirs(const char *filename)
 {
@@ -347,24 +351,243 @@ const char *storage_network_ssid_from_path(const char *path,
 	return buf;
 }
 
+/* Groups requiring encryption (if enabled) */
+static char *encrypt_groups[] = {
+	"Security",
+	NULL
+};
+
+static bool encrypt_group(const char *group)
+{
+	char **g;
+
+	for (g = encrypt_groups; *g; g++) {
+		if (!strcmp(*g, group))
+			return true;
+	}
+
+	return false;
+}
+
+/*
+ * Encrypt needed groups of 'settings' without modifying the object. Returns
+ * the entire settings object as data, with encrypted groups as a bytestring
+ * set as the value to [Security].EncryptedSecurity. This also includes any
+ * embedded groups.
+ *
+ * Note: If encryption is not enabled or there is no Security group this is
+ *       effectively l_settings_to_data.
+ */
+char *__storage_encrypt(struct l_settings *settings, const char *ssid,
+				size_t *out_len)
+{
+	struct iovec ad[2];
+	uint8_t salt[32];
+	size_t len;
+	_auto_(l_free) struct l_settings *to_encrypt = NULL;
+	_auto_(l_free) struct l_settings *original = NULL;
+	_auto_(l_free) char *plaintext = NULL;
+	_auto_(l_free) uint8_t *enc = NULL;
+	_auto_(l_strv_free) char **groups = NULL;
+	char **i;
+
+	if (!system_key_set || !l_settings_has_group(settings, "Security"))
+		return l_settings_to_data(settings, out_len);
+
+	/*
+	 * Make two copies of the settings: One will contain only data to be
+	 * encrypted (to_encrypt), the other will contain data to be left
+	 * unencrypted (original). At the end any encrypted data will be set
+	 * into 'original' as EncryptedSecurity.
+	 */
+	to_encrypt = l_settings_clone(settings);
+	original = l_settings_clone(settings);
+
+	groups = l_settings_get_groups(to_encrypt);
+	for (i = groups; *i; i++) {
+		if (encrypt_group(*i))
+			l_settings_remove_group(original, *i);
+		else
+			l_settings_remove_group(to_encrypt, *i);
+	}
+
+	l_settings_remove_embedded_groups(original);
+
+	plaintext = l_settings_to_data(to_encrypt, &len);
+	if (!plaintext)
+		return NULL;
+
+	l_getrandom(salt, 32);
+
+	ad[0].iov_base = (void *) salt;
+	ad[0].iov_len = 32;
+	ad[1].iov_base = (void *) ssid;
+	ad[1].iov_len = strlen(ssid);
+
+	enc = l_malloc(len + 16);
+
+	if (!aes_siv_encrypt(system_key, sizeof(system_key), plaintext, len,
+				ad, 2, enc)) {
+		l_error("Could not encrypt [Security] group");
+		return NULL;
+	}
+
+	l_settings_set_bytes(original, "Security", "EncryptedSalt", salt, 32);
+	l_settings_set_bytes(original, "Security", "EncryptedSecurity",
+				enc, len + 16);
+
+	return l_settings_to_data(original, out_len);
+}
+
+/*
+ * Decrypt data in [Security].EncryptedSecurity. This data also includes
+ * embedded groups potentially. Once decrypted the data is put back into the
+ * object.
+ *
+ * Note: if encryption is not enabled or there is no Security group settings
+ *       is not modified.
+ */
+int __storage_decrypt(struct l_settings *settings, const char *ssid,
+				bool *changed)
+{
+	_auto_(l_settings_free) struct l_settings *security = NULL;
+	_auto_(l_free) uint8_t *encrypted = NULL;
+	_auto_(l_free) uint8_t *decrypted = NULL;
+	_auto_(l_free) uint8_t *salt = NULL;
+	_auto_(l_strv_free) char **embedded = NULL;
+	size_t elen, slen;
+	struct iovec ad[2];
+
+	if (!system_key_set)
+		goto done;
+
+	if (!l_settings_has_group(settings, "Security"))
+		goto done;
+
+	encrypted = l_settings_get_bytes(settings, "Security",
+						"EncryptedSecurity", &elen);
+	salt = l_settings_get_bytes(settings, "Security",
+						"EncryptedSalt", &slen);
+
+	/*
+	 * Either profile has never been loaded after enabling encryption or is
+	 * missing Encrypted{Salt,Security} values. If either are missing this
+	 * profile is corrupted and must be fixed.
+	 */
+	if (!(encrypted && salt)) {
+		/* Profile corrupted */
+		if (encrypted || salt) {
+			l_warn("Profile %s is corrupted reconfigure manually",
+					ssid);
+			return -EBADMSG;
+		}
+
+		if (changed)
+			*changed = true;
+
+		return 0;
+	}
+
+	decrypted = l_malloc(elen - 16 + 1);
+
+	ad[0].iov_base = (void *)salt;
+	ad[0].iov_len = slen;
+	ad[1].iov_base = (void *)ssid;
+	ad[1].iov_len = strlen(ssid);
+
+	if (!aes_siv_decrypt(system_key, sizeof(system_key), encrypted, elen,
+				ad, 2, decrypted)) {
+		l_error("Could not decrypt %s profile, did the secret change?",
+				ssid);
+		return -ENOKEY;
+	}
+
+	decrypted[elen - 16] = '\0';
+
+	/* Remove encrypted [Security], and copy the decrypted one */
+	l_settings_remove_group(settings, "Security");
+
+	/*
+	 * Load decrypted data into existing settings. This is not how the API
+	 * is indended to be used (since this could result in duplicate groups)
+	 * but since the Security group was just removed and EncryptedSecurity
+	 * should only contain a Security group its safe to use it this way.
+	 */
+	if (!l_settings_load_from_data(settings, (const char *) decrypted,
+					elen - 16)) {
+		l_error("Could not load decrypted security group");
+		return -EBADMSG;
+	}
+
+done:
+	if (changed)
+		*changed = false;
+
+	return 0;
+}
+
+/*
+ * Direct open() by path. 'name' is used for decryption and depends on the
+ * module calling. For regular storage operations 'name' is the SSID. For
+ * hotspot 'name' is the consortium name (since the SSID is not always the
+ * same).
+ */
+bool __storage_open(struct l_settings *settings, const char *path,
+			enum security type, const char *name)
+{
+	bool changed;
+	_auto_(l_free) char *encrypted = NULL;
+	size_t elen;
+
+	if (type == SECURITY_NONE)
+		return true;
+
+	if (__storage_decrypt(settings, name, &changed) < 0)
+		return false;
+
+	if (!changed)
+		return true;
+
+	/* Profile never encrypted before. Encrypt and write to disk */
+	encrypted = __storage_encrypt(settings, name, &elen);
+	if (!encrypted) {
+		l_error("Could not encrypt new profile %s", name);
+		return false;
+	}
+
+	if (write_file(encrypted, elen, false, "%s", path) < 0) {
+		l_error("Failed to write out encrypted profile");
+		return false;
+	}
+
+	l_debug("Encrypted a new profile %s", path);
+
+	return true;
+}
+
 struct l_settings *storage_network_open(enum security type, const char *ssid)
 {
 	struct l_settings *settings;
-	char *path;
+	_auto_(l_free) char *path = NULL;
 
 	if (ssid == NULL)
 		return NULL;
 
 	path = storage_get_network_file_path(type, ssid);
+
 	settings = l_settings_new();
 
-	if (!l_settings_load_from_file(settings, path)) {
-		l_settings_free(settings);
-		settings = NULL;
-	}
+	if (!l_settings_load_from_file(settings, path))
+		goto error;
+
+	if (!__storage_open(settings, path, type, ssid))
+		goto error;
 
-	l_free(path);
 	return settings;
+
+error:
+	l_settings_free(settings);
+	return NULL;
 }
 
 int storage_network_touch(enum security type, const char *ssid)
@@ -388,15 +611,19 @@ int storage_network_touch(enum security type, const char *ssid)
 void storage_network_sync(enum security type, const char *ssid,
 				struct l_settings *settings)
 {
-	char *data;
+	_auto_(l_free) char *data = NULL;
+	_auto_(l_free) char *path = NULL;
 	size_t length = 0;
-	char *path;
 
 	path = storage_get_network_file_path(type, ssid);
-	data = l_settings_to_data(settings, &length);
+	data = __storage_encrypt(settings, ssid, &length);
+
+	if (!data) {
+		l_error("Unable to sync profile %s", ssid);
+		return;
+	}
+
 	write_file(data, length, true, "%s", path);
-	l_free(data);
-	l_free(path);
 }
 
 int storage_network_remove(enum security type, const char *ssid)
@@ -463,3 +690,22 @@ bool storage_is_file(const char *filename)
 
 	return false;
 }
+
+/*
+ * Initialize a systemd encryption key for encrypting/decrypting credentials.
+ * This uses the 'extract and expand' concept from RFC 5869 to derive a new,
+ * fixed length, key.
+ */
+void __storage_set_encryption_key(const uint8_t *key, size_t key_len)
+{
+	uint8_t tmp[32];
+
+	if (!hkdf_extract(L_CHECKSUM_SHA256, NULL, 0, 1, tmp, key, key_len))
+		return;
+
+	if (!hkdf_expand(L_CHECKSUM_SHA256, tmp, sizeof(tmp), "IWD System Key",
+				system_key, sizeof(system_key)))
+		return;
+
+	system_key_set = true;
+}
diff --git a/src/storage.h b/src/storage.h
index e1ec2cd4..93f23c6e 100644
--- a/src/storage.h
+++ b/src/storage.h
@@ -50,3 +50,11 @@ int storage_network_remove(enum security type, const char *ssid);
 
 struct l_settings *storage_known_frequencies_load(void);
 void storage_known_frequencies_sync(struct l_settings *known_freqs);
+
+void __storage_set_encryption_key(const uint8_t *key, size_t key_len);
+int __storage_decrypt(struct l_settings *settings, const char *ssid,
+				bool *changed);
+char *__storage_encrypt(struct l_settings *settings, const char *ssid,
+				size_t *out_len);
+bool __storage_open(struct l_settings *settings, const char *path,
+			enum security type, const char *name);
-- 
2.31.1