Re: [PATCH v7 3/8] storage: implement network profile encryption

Re: [PATCH v7 3/8] storage: implement network profile encryption

[James Prestwood]

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

Hi Marcel,

<snip>

> > +
> > +       ad[0].iov_base = (void *) salt;
> > +       ad[0].iov_len = 32;
> > +       ad[1].iov_base = (void *) ssid;
> > +       ad[1].iov_len = strlen(ssid);
> 
> you want to use the SSID as string here since that is how we store
> it? Or would you rather use the fixed size binary blob of the SSID on
> how it goes over the air? So a comment is required here.

So pad the SSID with zero's? I'm not sure this really adds much except
an extra step. But I can leave a comment, or if you have a specific
reason for doing it I'm all ears.
> 
> 

<snip>

> > +/*
> > + * 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)))
> 
> So I have a dislike putting the IWD name in front of it. They are
> daemon specific to begin with. I would leave it at “System Key”. Key
> names should be domain specific and any attempt to make the globally
> unique will fail anyway.
> 
> The actual key is required to be provided by a human user creating
> the unit file. And thus it is specific to iwd’s unit file. If they
> end up re-using this for another daemon, then that is just plain
> stupid.

Sure, makes sense.

> 
> Now that all said, I am not sure you are using this correctly. I know
> you have defined the extract and expand functions, but I was really
> more thinking about the simple version of this:
> 
>         interim-key = func(SALT, system-key)
>         encryption-key = func(interim-key, “System Key”)
> 
> While the SALT is just a 32 octet random string instead of the
> default sequence of 0x00.
> 
> Since later on your are using AES for the encryption with a 128-bit
> key, I would recommend to just go with AES-CMAC. That means you can
> define func as this:
> 
>         func(K, ID) = AES-CMAC(K, ID)
> 
> Where K is the 128-bit key into the AES-CMAS hash.

Maybe I'm missing something with how AES-CMAC works, but using a random
salt for the system key like this is going to result in a different
system key on each boot/run right? So I don't think that is going to
work. RFC 5869 mentions using a zero salt like I do here, basically
'salt' is optional.

Also I'm not sure why we want to define our own KDF (using AES-CMAC)
when extract-and-expand already exists for this purpose. I think NIST
even recommends using extract-and-expand KDFs.

> 
> > +               return;
> > +
> > +       L_WARN_ON(mlock(system_key, sizeof(system_key)) < 0);
> 
> Actually, better to just abort here. Let it fail if we can’t lock the
> memory. Otherwise also you call to unlock below is unbalanced.
> 
> > +
> > +       system_key_set = true;
> > +}
> > +
> > static int storage_init(void)
> > {
> >         const char *state_dir;
> > @@ -466,6 +726,11 @@ static void storage_exit(void)
> > {
> >         l_free(storage_path);
> >         l_free(storage_hotspot_path);
> > +
> > +       if (system_key_set) {
> > +               explicit_bzero(system_key, sizeof(system_key));
> > +               munlock(system_key, sizeof(system_key));
> > +       }
> > }
> > 
> > IWD_MODULE(storage, storage_init, storage_exit);
> > diff --git a/src/storage.h b/src/storage.h
> > index f75185f6..a3ae71be 100644
> > --- a/src/storage.h
> > +++ b/src/storage.h
> > @@ -48,3 +48,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(const 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);
> 
> Regards
> 
> Marcel
> 

Thanks,
James

Re: [PATCH v7 3/8] storage: implement network profile encryption

[Marcel Holtmann]

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

Hi James,

> 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 | 287 ++++++++++++++++++++++++++++++++++++++++++++++++--
> src/storage.h |   8 ++
> 3 files changed, 286 insertions(+), 12 deletions(-)
> 
> v7:
> * Added mlock/munlock to the system_key buffer
> 
> 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 6bf0616d..4918824e 100644
> --- a/src/storage.c
> +++ b/src/storage.c
> @@ -39,12 +39,15 @@
> #include <sys/time.h>
> #include <sys/types.h>
> #include <sys/stat.h>
> +#include <sys/mman.h>
> 
> #include <ell/ell.h>
> +#include "ell/useful.h"
> 
> #include "src/common.h"
> #include "src/storage.h"
> #include "src/module.h"
> +#include "src/crypto.h"
> 
> #define STORAGE_DIR_MODE (S_IRUSR | S_IWUSR | S_IXUSR)
> #define STORAGE_FILE_MODE (S_IRUSR | S_IWUSR)
> @@ -53,6 +56,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)
> {
> @@ -300,24 +305,254 @@ 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(const struct l_settings *settings, const char *ssid,
> +				size_t *out_len)
> +{
> +	struct iovec ad[2];
> +	uint8_t salt[32];
> +	size_t len;
> +	_auto_(l_settings_free) struct l_settings *to_encrypt = NULL;
> +	_auto_(l_settings_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);

you want to use the SSID as string here since that is how we store it? Or would you rather use the fixed size binary blob of the SSID on how it goes over the air? So a comment is required here.

> +
> +	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);

You do need to comment the + 16 since it is not obvious.

> +
> +	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;
> +	_auto_(l_strv_free) char **groups = NULL;
> +	char **i;
> +	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 any groups that are marked as encrypted (plus embedded),
> +	 * and copy the decrypted groups back into settings.
> +	 */
> +	groups = l_settings_get_groups(settings);
> +	for (i = groups; *i; i++) {
> +		if (encrypt_group(*i))
> +			l_settings_remove_group(settings, *i);
> +	}
> +
> +	l_settings_remove_embedded_groups(settings);
> +
> +	/*
> +	 * 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;
> +	}

Same as with the other functions. Comments required on the input/output choices.

> +
> +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)
> @@ -341,15 +576,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)
> @@ -417,6 +656,27 @@ 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)))

So I have a dislike putting the IWD name in front of it. They are daemon specific to begin with. I would leave it at “System Key”. Key names should be domain specific and any attempt to make the globally unique will fail anyway.

The actual key is required to be provided by a human user creating the unit file. And thus it is specific to iwd’s unit file. If they end up re-using this for another daemon, then that is just plain stupid.

Now that all said, I am not sure you are using this correctly. I know you have defined the extract and expand functions, but I was really more thinking about the simple version of this:

	interim-key = func(SALT, system-key)
	encryption-key = func(interim-key, “System Key”)

While the SALT is just a 32 octet random string instead of the default sequence of 0x00.

Since later on your are using AES for the encryption with a 128-bit key, I would recommend to just go with AES-CMAC. That means you can define func as this:

	func(K, ID) = AES-CMAC(K, ID)

Where K is the 128-bit key into the AES-CMAS hash.

> +		return;
> +
> +	L_WARN_ON(mlock(system_key, sizeof(system_key)) < 0);

Actually, better to just abort here. Let it fail if we can’t lock the memory. Otherwise also you call to unlock below is unbalanced.

> +
> +	system_key_set = true;
> +}
> +
> static int storage_init(void)
> {
> 	const char *state_dir;
> @@ -466,6 +726,11 @@ static void storage_exit(void)
> {
> 	l_free(storage_path);
> 	l_free(storage_hotspot_path);
> +
> +	if (system_key_set) {
> +		explicit_bzero(system_key, sizeof(system_key));
> +		munlock(system_key, sizeof(system_key));
> +	}
> }
> 
> IWD_MODULE(storage, storage_init, storage_exit);
> diff --git a/src/storage.h b/src/storage.h
> index f75185f6..a3ae71be 100644
> --- a/src/storage.h
> +++ b/src/storage.h
> @@ -48,3 +48,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(const 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);

Regards

Marcel

[PATCH v7 3/8] storage: implement network profile encryption

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 13697 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 | 287 ++++++++++++++++++++++++++++++++++++++++++++++++--
 src/storage.h |   8 ++
 3 files changed, 286 insertions(+), 12 deletions(-)

v7:
 * Added mlock/munlock to the system_key buffer

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 6bf0616d..4918824e 100644
--- a/src/storage.c
+++ b/src/storage.c
@@ -39,12 +39,15 @@
 #include <sys/time.h>
 #include <sys/types.h>
 #include <sys/stat.h>
+#include <sys/mman.h>
 
 #include <ell/ell.h>
+#include "ell/useful.h"
 
 #include "src/common.h"
 #include "src/storage.h"
 #include "src/module.h"
+#include "src/crypto.h"
 
 #define STORAGE_DIR_MODE (S_IRUSR | S_IWUSR | S_IXUSR)
 #define STORAGE_FILE_MODE (S_IRUSR | S_IWUSR)
@@ -53,6 +56,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)
 {
@@ -300,24 +305,254 @@ 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(const struct l_settings *settings, const char *ssid,
+				size_t *out_len)
+{
+	struct iovec ad[2];
+	uint8_t salt[32];
+	size_t len;
+	_auto_(l_settings_free) struct l_settings *to_encrypt = NULL;
+	_auto_(l_settings_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;
+	_auto_(l_strv_free) char **groups = NULL;
+	char **i;
+	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 any groups that are marked as encrypted (plus embedded),
+	 * and copy the decrypted groups back into settings.
+	 */
+	groups = l_settings_get_groups(settings);
+	for (i = groups; *i; i++) {
+		if (encrypt_group(*i))
+			l_settings_remove_group(settings, *i);
+	}
+
+	l_settings_remove_embedded_groups(settings);
+
+	/*
+	 * 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)
@@ -341,15 +576,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)
@@ -417,6 +656,27 @@ 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;
+
+	L_WARN_ON(mlock(system_key, sizeof(system_key)) < 0);
+
+	system_key_set = true;
+}
+
 static int storage_init(void)
 {
 	const char *state_dir;
@@ -466,6 +726,11 @@ static void storage_exit(void)
 {
 	l_free(storage_path);
 	l_free(storage_hotspot_path);
+
+	if (system_key_set) {
+		explicit_bzero(system_key, sizeof(system_key));
+		munlock(system_key, sizeof(system_key));
+	}
 }
 
 IWD_MODULE(storage, storage_init, storage_exit);
diff --git a/src/storage.h b/src/storage.h
index f75185f6..a3ae71be 100644
--- a/src/storage.h
+++ b/src/storage.h
@@ -48,3 +48,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(const 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.34.1