Adding tipc-discusson to the recipient list of this disacussion.
///jon
> -----Original Message-----
> From: Jon Maloy
> Sent: 21-Dec-19 10:56
> To: Tuong Lien <tuong.t.l...@dektech.com.au>; tipc-...@dektech.com.au
> Subject: RE: [PATCH RFC] tipc: add automatic rekeying for TIPC encryption
>
> Hi Tuong,
> I am getting more and more ambivalent about this.
> Before introducing this we must weigh the pro and cons correctly:
>
> Pros:
> - It will give less time for a sniffer to crack the key, since we change it
> often.
> - If a sniffer cracks a key, he only gets access to messages from the node
> owning that key, not the whole
> cluster.
>
> Cons:
> - Larger code footprint.
> - No forward security. If a sniffer gets hold of a node key, he can listen
> forever, as you have already noted.
> - New nodes cannot be added to the cluster unless there is a way for the user
> land framework to read the
> current key on each node and distribute to the new node.
> - What happens when a node restarts, and forgets about all current keys, both
> its own and those of its
> peers.
>
> The last two bullets are showstoppers, in my view. We must find a way to
> add/re-insert nodes as easily as
> we do now, at least.
>
> This means we must come back to the solution with a "master" cluster key.
> I suspect a typical user will just aim for adding the key or keys to
> the cluster configuration file, and at a maximum update that key at regular
> intervals.
>
> So, maybe, apart from the 'current' node key we also keep a 'cluster' master
> key that can always be used
> for attaching new nodes, but allow for this to be updated via the tipc tool.
> The user may then, of he wants, replace/update this key at regular intervals.
> At the same time this key must be reserved for new/restarting nodes only, and
> not be permitted for use by
> existing nodes which already are using 'current' keys.
>
> This would at least make it somewhat more acceptable for the user than the
> current approach.
>
> Regards
> ///jon
>
>
>
>
> > -----Original Message-----
> > From: Jon Maloy
> > Sent: 18-Dec-19 19:33
> > To: Tuong Lien <tuong.t.l...@dektech.com.au>; tipc-...@dektech.com.au
> > Subject: RE: [PATCH RFC] tipc: add automatic rekeying for TIPC encryption
> >
> > I have a couple comments below, but in general it looks good.
> > So, after some hesitation I suggest you send this to tipc-discussion for
> > further review.
> >
> > ///jon
> >
> >
> > > -----Original Message-----
> > > From: Tuong Lien <tuong.t.l...@dektech.com.au>
> > > Sent: 10-Dec-19 06:48
> > > To: Jon Maloy <jon.ma...@ericsson.com>; tipc-...@dektech.com.au
> > > Subject: [PATCH RFC] tipc: add automatic rekeying for TIPC encryption
> > >
> > > This commit adds functionality for TIPC encryption to automatically
> > > generate a new symmetric key and attach as new TX key for the own node.
> > > The new key will also be distributed to peer nodes and set as RX keys,
> > > i.e. the per-node key mode. This process of rekeying will be repeated,
> > > for example every ~ 10 minutes (from the time a certain key becomes
> > > active), so new session keys will be created and applied regularly to
> > > the cluster.
> > >
> > > Since the key exchange needs a secure communication channel in advance,
> > > the commit simplifies this by only starting with an initial key (either
> > > in cluster or per-node key mode) which is set by user first (e.g. via
> > > the "tipc node set key ..." command), so the entire cluster is already
> > > secure and authenticated. After that, the rekeying will be scheduled as
> > > said above.
> > >
> > > Also, the key exchange utilizes the broadcast link which is reliable.
> > > And, the key switching with no impact on traffic is already supported
> > > by the previous commit.
> > >
> > > ----------------------------------------------
> > > Note: this solution has two issues:
> > > - No forward secrecy
> > > - how a new VM joins the cluster...?
> >
> > We still need a framework for distributing the initial key for a new node,
> > but it will probably be simpler
> > than before since we don't need to distribute keys regularly.
> >
> > > ----------------------------------------------
> > >
> > > Signed-off-by: Tuong Lien <tuong.t.l...@dektech.com.au>
> > > ---
> > > net/tipc/bcast.c | 4 +-
> > > net/tipc/bcast.h | 2 +
> > > net/tipc/crypto.c | 195
> > > +++++++++++++++++++++++++++++++++++++++++++++++++++++-
> > > net/tipc/crypto.h | 1 +
> > > net/tipc/link.c | 3 +
> > > net/tipc/msg.h | 1 +
> > > net/tipc/node.c | 8 +++
> > > net/tipc/node.h | 1 +
> > > 8 files changed, 211 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/net/tipc/bcast.c b/net/tipc/bcast.c
> > > index 55aeba681cf4..330b683f335e 100644
> > > --- a/net/tipc/bcast.c
> > > +++ b/net/tipc/bcast.c
> > > @@ -249,8 +249,8 @@ static void tipc_bcast_select_xmit_method(struct net
> > > *net, int dests,
> > > * Consumes the buffer chain.
> > > * Returns 0 if success, otherwise errno: -EHOSTUNREACH,-EMSGSIZE
> > > */
> > > -static int tipc_bcast_xmit(struct net *net, struct sk_buff_head *pkts,
> > > - u16 *cong_link_cnt)
> > > +int tipc_bcast_xmit(struct net *net, struct sk_buff_head *pkts,
> > > + u16 *cong_link_cnt)
> > > {
> > > struct tipc_link *l = tipc_bc_sndlink(net);
> > > struct sk_buff_head xmitq;
> > > diff --git a/net/tipc/bcast.h b/net/tipc/bcast.h
> > > index 9e847d9617d3..f095a2ac27cb 100644
> > > --- a/net/tipc/bcast.h
> > > +++ b/net/tipc/bcast.h
> > > @@ -89,6 +89,8 @@ void tipc_bcast_toggle_rcast(struct net *net, bool
> > > supp);
> > > int tipc_mcast_xmit(struct net *net, struct sk_buff_head *pkts,
> > > struct tipc_mc_method *method, struct tipc_nlist *dests,
> > > u16 *cong_link_cnt);
> > > +int tipc_bcast_xmit(struct net *net, struct sk_buff_head *pkts,
> > > + u16 *cong_link_cnt);
> > > int tipc_bcast_rcv(struct net *net, struct tipc_link *l, struct sk_buff
> > > *skb);
> > > void tipc_bcast_ack_rcv(struct net *net, struct tipc_link *l,
> > > struct tipc_msg *hdr);
> > > diff --git a/net/tipc/crypto.c b/net/tipc/crypto.c
> > > index 990a872cec46..b415c9adf14d 100644
> > > --- a/net/tipc/crypto.c
> > > +++ b/net/tipc/crypto.c
> > > @@ -36,12 +36,17 @@
> > >
> > > #include <crypto/aead.h>
> > > #include <crypto/aes.h>
> > > +#include <crypto/rng.h>
> > > #include "crypto.h"
> > > +#include "msg.h"
> > > +#include "bcast.h"
> > >
> > > #define TIPC_TX_PROBE_LIM msecs_to_jiffies(1000) /* > 1s */
> > > #define TIPC_TX_LASTING_LIM msecs_to_jiffies(120000) /* 2 mins */
> > > #define TIPC_RX_ACTIVE_LIM msecs_to_jiffies(3000) /* 3s */
> > > #define TIPC_RX_PASSIVE_LIM msecs_to_jiffies(180000) /* 3 mins */
> > > +#define TIPC_REKEYING_TIME msecs_to_jiffies(600000) /* 10 mins */
> > > +
> > > #define TIPC_MAX_TFMS_DEF 10
> > > #define TIPC_MAX_TFMS_LIM 1000
> > >
> > > @@ -132,6 +137,7 @@ struct tipc_tfm {
> > > * @mode: crypto mode is applied to the key
> > > * @hint[]: a hint for user key
> > > * @rcu: struct rcu_head
> > > + * @keylen: the aead key length
> > > * @seqno: the key seqno (cluster scope)
> > > * @refcnt: the key reference counter
> > > */
> > > @@ -146,6 +152,7 @@ struct tipc_aead {
> > > u8 mode;
> > > char hint[TIPC_AEAD_HINT_LEN + 1];
> > > struct rcu_head rcu;
> > > + unsigned int keylen;
> > >
> > > atomic64_t seqno ____cacheline_aligned;
> > > refcount_t refcnt ____cacheline_aligned;
> > > @@ -172,6 +179,8 @@ struct tipc_crypto_stats {
> > > * @timer1: general timer 1 (jiffies)
> > > * @timer2: general timer 1 (jiffies)
> > > * @lock: tipc_key lock
> > > + * @work: delayed work for rekeying
> > > + * @skey: session key
> > > */
> > > struct tipc_crypto {
> > > struct net *net;
> > > @@ -186,6 +195,8 @@ struct tipc_crypto {
> > > unsigned long timer1;
> > > unsigned long timer2;
> > > spinlock_t lock; /* crypto lock */
> > > + struct delayed_work work;
> > > + struct tipc_aead_key *skey;
> > >
> > > } ____cacheline_aligned;
> > >
> > > @@ -210,6 +221,8 @@ static void tipc_aead_users_inc(struct tipc_aead
> > > __rcu *aead, int lim);
> > > static void tipc_aead_users_dec(struct tipc_aead __rcu *aead, int lim);
> > > static void tipc_aead_users_set(struct tipc_aead __rcu *aead, int val);
> > > static struct crypto_aead *tipc_aead_tfm_next(struct tipc_aead *aead);
> > > +static struct tipc_aead_key *tipc_aead_key_generate(unsigned int keylen);
> > > +static int tipc_aead_key_distribute(struct net *net, struct
> > > tipc_aead_key *skey);
> > > static int tipc_aead_init(struct tipc_aead **aead, struct tipc_aead_key
> > > *ukey,
> > > u8 mode);
> > > static int tipc_aead_clone(struct tipc_aead **dst, struct tipc_aead
> > > *src);
> > > @@ -251,6 +264,7 @@ static char *tipc_crypto_key_dump(struct tipc_crypto
> > > *c, char *buf);
> > > static char *tipc_key_change_dump(struct tipc_key old, struct tipc_key
> > > new,
> > > char *buf);
> > > #endif
> > > +static void tipc_crypto_rekey(struct work_struct *work);
> > >
> > > #define key_next(cur) ((cur) % KEY_MAX + 1)
> > >
> > > @@ -506,6 +520,7 @@ static int tipc_aead_init(struct tipc_aead **aead,
> > > struct tipc_aead_key *ukey,
> > > tmp->mode = mode;
> > > tmp->cloned = NULL;
> > > tmp->authsize = TIPC_AES_GCM_TAG_SIZE;
> > > + tmp->keylen = ukey->keylen;
> > > memcpy(&tmp->salt, ukey->key + keylen, TIPC_AES_GCM_SALT_SIZE);
> > > atomic_set(&tmp->users, 0);
> > > atomic64_set(&tmp->seqno, 0);
> > > @@ -1368,8 +1383,9 @@ int tipc_crypto_start(struct tipc_crypto **crypto,
> > > struct net *net,
> > > c->timer1 = jiffies;
> > > c->timer2 = jiffies;
> > > spin_lock_init(&c->lock);
> > > - *crypto = c;
> > > + INIT_DELAYED_WORK(&c->work, tipc_crypto_rekey);
> > >
> > > + *crypto = c;
> > > return 0;
> > > }
> > >
> > > @@ -1382,6 +1398,10 @@ void tipc_crypto_stop(struct tipc_crypto **crypto)
> > > if (!*crypto)
> > > return;
> > >
> > > + /* Cancel any rekeying work */
> > > + c = *crypto;
> > > + cancel_delayed_work_sync(&c->work);
> > > +
> > > rcu_read_lock();
> > > /* RX stopping? => decrease TX key users if any */
> > > is_rx = !!((*crypto)->node);
> > > @@ -1397,7 +1417,6 @@ void tipc_crypto_stop(struct tipc_crypto **crypto)
> > > }
> > >
> > > /* Release AEAD keys */
> > > - c = *crypto;
> > > for (k = KEY_MIN; k <= KEY_MAX; k++)
> > > tipc_aead_put(rcu_dereference(c->aead[k]));
> > > rcu_read_unlock();
> > > @@ -1442,6 +1461,9 @@ void tipc_crypto_timeout(struct tipc_crypto *rx)
> > > pr_info("TX(%s): key %d is activated!\n", tipc_own_id_string(tx->net),
> > > key.pending);
> > >
> > > + /* Schedule rekeying */
> > > + schedule_delayed_work(&tx->work, TIPC_REKEYING_TIME);
> > > +
> > > s1:
> > > spin_unlock(&tx->lock);
> > >
> > > @@ -1984,3 +2006,172 @@ static char *tipc_key_change_dump(struct tipc_key
> > > old, struct tipc_key
> > > new,
> > > return buf;
> > > }
> > > #endif
> > > +
> > > +static struct tipc_aead_key *tipc_aead_key_generate(unsigned int keylen)
> > > +{
> > > + struct tipc_aead_key *skey;
> > > + int rc;
> > > +
> > > + if (!keylen)
> > > + return NULL;
> > > +
> > > + skey = kmalloc(sizeof(*skey) + keylen, GFP_ATOMIC);
> > > + skey->keylen = keylen;
> > > + strcpy(skey->alg_name, "gcm(aes)");
> > > +
> > > + /* Obtain a key via the default RNG (like ECC) */
> > > + rc = crypto_get_default_rng();
> > > + if (unlikely(rc))
> > > + goto exit;
> > > +
> > > + rc = crypto_rng_get_bytes(crypto_default_rng, skey->key, keylen);
> > > + crypto_put_default_rng();
> > > + if (unlikely(rc))
> > > + goto exit;
> > > +
> > > + return skey;
> > > +
> > > +exit:
> > > + kfree(skey);
> > > + return NULL;
> > > +}
> > > +
> > > +static int tipc_aead_key_distribute(struct net *net, struct
> > > tipc_aead_key *skey)
> > > +{
> > > + struct sk_buff_head pkts;
> > > + struct tipc_msg *hdr;
> > > + struct sk_buff *skb;
> > > + __be32 keylen = htonl(skey->keylen);
> > > + u16 size, cong_link_cnt;
> > > + u8 *data;
> > > + int rc;
> > > +
> > > + size = tipc_aead_key_size(skey);
> > > + skb = tipc_buf_acquire(INT_H_SIZE + size, GFP_ATOMIC);
> > > + if (!skb)
> > > + return -ENOMEM;
> > > +
> > > + hdr = buf_msg(skb);
> > > + tipc_msg_init(tipc_own_addr(net), hdr, KEY_EXCHANGER, 0,
> > > + INT_H_SIZE, 0);
> > > +
> > > + msg_set_non_seq(hdr, 1);
> > > + msg_set_size(hdr, INT_H_SIZE + size);
> > > +
> > > + data = msg_data(hdr);
> > > + memcpy(data, skey->alg_name, TIPC_AEAD_ALG_NAME);
> > > + memcpy(data + TIPC_AEAD_ALG_NAME, (u8 *)&keylen, sizeof(__be32));
> > > + memcpy(data + TIPC_AEAD_ALG_NAME + sizeof(__be32), skey->key,
> > > + skey->keylen);
> > > +
> > > + __skb_queue_head_init(&pkts);
> > > + __skb_queue_tail(&pkts, skb);
> > > + rc = tipc_bcast_xmit(net, &pkts, &cong_link_cnt);
> > > +
> > > + return rc;
> > > +}
> > > +
> > > +int tipc_aead_key_rcv(struct net *net, struct sk_buff *skb)
> > > +{
> > > + struct tipc_msg *hdr = buf_msg(skb);
> > > + struct tipc_crypto *rx;
> > > + u16 size = msg_data_sz(hdr);
> > > + u8 *data = msg_data(hdr);
> > > + unsigned int keylen;
> > > + int rc = 0;
> > > +
> > > + rx = tipc_node_crypto_rx_by_addr(net, msg_prevnode(hdr));
> > > + if (unlikely(!rx || rx->skey)) {
> > > + pr_err("RX(%s): no RX or one key has received\n",
> > > + (rx) ? tipc_node_get_id_str(rx->node) : "-");
> > > + rc = -EKEYREJECTED;
> > > + goto exit;
> > > + }
> > > +
> > > + /* Sanity check */
> > > + keylen = ntohl(*((__be32*)(data + TIPC_AEAD_ALG_NAME)));
> > > + if (unlikely(size != sizeof(struct tipc_aead_key) + keylen)) {
> > > + pr_err("RX(%s): key is not valid, keylen %d, size %d\n",
> > > + tipc_node_get_id_str(rx->node), keylen, size);
> > > + rc = -EFAULT;
> > > + goto exit;
> > > + }
> > > +
> > > + /* Allocate memory for the key */
> > > + rx->skey = kmalloc(size, GFP_ATOMIC);
> > > + if (unlikely(!rx->skey)) {
> > > + pr_err("RX(%s): no memory for session key\n",
> > > + tipc_node_get_id_str(rx->node));
> > > + rc = -ENOMEM;
> > > + goto exit;
> > > + }
> > > +
> > > + /* Copy key from msg data */
> > > + rx->skey->keylen = keylen;
> > > + memcpy(rx->skey->alg_name, data, TIPC_AEAD_ALG_NAME);
> > > + memcpy(rx->skey->key, data + TIPC_AEAD_ALG_NAME + sizeof(__be32),
> > > + rx->skey->keylen);
> > > +
> > > + /* Schedule key attaching on this RX */
> > > + schedule_delayed_work(&rx->work, 0);
> > > +
> > > +exit:
> > > + if (rx)
> > > + tipc_node_put(rx->node);
> > > + kfree_skb(skb);
> > > + return rc;
> > > +}
> > > +
> > > +static void tipc_crypto_rekey(struct work_struct *work)
> > > +{
> > > + struct delayed_work *delayed_work = to_delayed_work(work);
> > > + struct tipc_crypto *c = container_of(delayed_work, struct tipc_crypto,
> > > + work);
> > > + struct tipc_aead *aead;
> > > + struct tipc_key key;
> > > + unsigned int keylen = 0;
> > > + int rc;
> > > +
> > > + /* Generate new session key for TX crypto if not yet */
> > > + if (!c->node && !c->skey) {
> > > + /* Make sure we had a TX key active i.e. secured channel */
> > > + key = c->key;
> > > + if (!key.active || key.pending)
> > > + return;
> > > +
> > > + rcu_read_lock();
> > > + aead = tipc_aead_get(c->aead[key.active]);
> > > + if (aead) {
> > > + keylen = aead->keylen;
> > > + tipc_aead_put(aead);
> > > + }
> > > + rcu_read_unlock();
> > > +
> > > + /* Generate new key */
> > > + c->skey = tipc_aead_key_generate(keylen);
> > > + if (unlikely(!c->skey))
> > > + goto retry;
> > > +
> > > + /* Distribute key to peers */
> > > + rc = tipc_aead_key_distribute(c->net, c->skey);
> > > + if (unlikely(rc)) {
> > > + kfree(c->skey);
> > > + c->skey = NULL;
> > > + goto retry;
> > > + }
> > > + }
> > > +
> > > + /* Attach the new key to TX/RX */
> > > + BUG_ON(!c->skey);
> >
> > BUG_ON() in production code is always frowned upon.
> > Better a warning and (if possible) continue with the old key.
> >
> >
> > > + rc = tipc_crypto_key_init(c, c->skey, PER_NODE_KEY);
> > > + if (unlikely(rc == -ENOMEM || rc == -EBUSY))
> > > + goto retry;
> > > +
> > > + kfree(c->skey);
> > > + c->skey = NULL;
> > > + return;
> > > +
> > > +retry:
> > > + /* Let's try again after 1 min */
> > > + schedule_delayed_work(&c->work, msecs_to_jiffies(60000));
> > > +}
> > > diff --git a/net/tipc/crypto.h b/net/tipc/crypto.h
> > > index c3de769f49e8..d222fcafa88d 100644
> > > --- a/net/tipc/crypto.h
> > > +++ b/net/tipc/crypto.h
> > > @@ -161,6 +161,7 @@ int tipc_crypto_key_init(struct tipc_crypto *c,
> > > struct tipc_aead_key *ukey,
> > > u8 mode);
> > > void tipc_crypto_key_flush(struct tipc_crypto *c);
> > > int tipc_aead_key_validate(struct tipc_aead_key *ukey);
> > > +int tipc_aead_key_rcv(struct net *net, struct sk_buff *skb);
> > > bool tipc_ehdr_validate(struct sk_buff *skb);
> > >
> > > #endif /* _TIPC_CRYPTO_H */
> > > diff --git a/net/tipc/link.c b/net/tipc/link.c
> > > index 24d4d10756d3..6e153f5d3a68 100644
> > > --- a/net/tipc/link.c
> > > +++ b/net/tipc/link.c
> > > @@ -1217,6 +1217,9 @@ static bool tipc_data_input(struct tipc_link *l,
> > > struct sk_buff *skb,
> > > case MSG_FRAGMENTER:
> > > case BCAST_PROTOCOL:
> > > return false;
> > > + case KEY_EXCHANGER:
> > > + tipc_aead_key_rcv(l->net, skb);
> > > + return true;
> > > default:
> > > pr_warn("Dropping received illegal msg type\n");
> > > kfree_skb(skb);
> > > diff --git a/net/tipc/msg.h b/net/tipc/msg.h
> > > index 6d466ebdb64f..227fb708c32c 100644
> > > --- a/net/tipc/msg.h
> > > +++ b/net/tipc/msg.h
> > > @@ -82,6 +82,7 @@ struct plist;
> > > #define NAME_DISTRIBUTOR 11
> > > #define MSG_FRAGMENTER 12
> > > #define LINK_CONFIG 13
> > > +#define KEY_EXCHANGER 14
> > > #define SOCK_WAKEUP 14 /* pseudo user */
> > > #define TOP_SRV 15 /* pseudo user */
> > >
> > > diff --git a/net/tipc/node.c b/net/tipc/node.c
> > > index ab04e00cb95b..19a560cfb10f 100644
> > > --- a/net/tipc/node.c
> > > +++ b/net/tipc/node.c
> > > @@ -276,6 +276,14 @@ struct tipc_crypto
> > > *tipc_node_crypto_rx_by_list(struct list_head *pos)
> > > {
> > > return container_of(pos, struct tipc_node, list)->crypto_rx;
> > > }
> > > +
> > > +struct tipc_crypto *tipc_node_crypto_rx_by_addr(struct net *net, u32
> > > addr)
> > > +{
> > > + struct tipc_node *n;
> > > +
> > > + n = tipc_node_find(net, addr);
> > > + return (n) ? n->crypto_rx : NULL;
> > > +}
> > > #endif
> > >
> > > void tipc_node_free(struct rcu_head *rp)
> > > diff --git a/net/tipc/node.h b/net/tipc/node.h
> > > index a6803b449a2c..7b52a2a2b855 100644
> > > --- a/net/tipc/node.h
> > > +++ b/net/tipc/node.h
> > > @@ -83,6 +83,7 @@ struct tipc_node *tipc_node_create(struct net *net, u32
> > > addr, u8 *peer_id,
> > > #ifdef CONFIG_TIPC_CRYPTO
> >
> > #ifdefs directly in the code never looks good, and should be avoided.
> > I think you in many cases can replace the #ifdef sections with macros, and
> > put the #ifdef inside the
> macro.
> > This would of course require another patch, since we already have some such
> > sections in the existing
> code.
> >
> > > struct tipc_crypto *tipc_node_crypto_rx(struct tipc_node *__n);
> > > struct tipc_crypto *tipc_node_crypto_rx_by_list(struct list_head *pos);
> > > +struct tipc_crypto *tipc_node_crypto_rx_by_addr(struct net *net, u32
> > > addr);
> > > #endif
> > > u32 tipc_node_try_addr(struct net *net, u8 *id, u32 addr);
> > > void tipc_node_check_dest(struct net *net, u32 onode, u8 *peer_id128,
> > > --
> > > 2.13.7
_______________________________________________
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion
