netdev - Re: [PATCH net-next v5 12/25] ovpn: implement packet processing

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <9bcdabb0-7682-4723-aa2f-ca64f1b8202a@openvpn.net>
Date: Wed, 10 Jul 2024 13:38:28 +0200
From: Antonio Quartulli <antonio@...nvpn.net>
To: Sabrina Dubroca <sd@...asysnail.net>
Cc: netdev@...r.kernel.org, kuba@...nel.org, ryazanov.s.a@...il.com,
 pabeni@...hat.com, edumazet@...gle.com, andrew@...n.ch
Subject: Re: [PATCH net-next v5 12/25] ovpn: implement packet processing

Hi,

On 09/07/2024 10:51, Sabrina Dubroca wrote:
> 2024-06-27, 15:08:30 +0200, Antonio Quartulli wrote:
>> +/* removes the primary key from the crypto context */
>> +void ovpn_crypto_kill_primary(struct ovpn_crypto_state *cs)
>> +{
>> +	struct ovpn_crypto_key_slot *ks;
>> +
>> +	mutex_lock(&cs->mutex);
>> +	ks = rcu_replace_pointer(cs->primary, NULL,
>> +				 lockdep_is_held(&cs->mutex));
> 
> Should there be a check that we're killing the key that has expired
> and not some other key?  I'm wondering if this could happen:
> 
> ovpn_encrypt_one
>      ovpn_aead_encrypt
>          ovpn_pktid_xmit_next
>              seq_num reaches threshold
>              returns -ERANGE
>          returns -ERANGE
> 
>                                              ovpn_crypto_key_slots_swap
>                                                  replaces cs->primary with cs->secondary
> 
>      ovpn_encrypt_post
>          ret = -ERANGE
>          ovpn_crypto_kill_primary
>              kills the freshly installed primary key
> 
>> +	ovpn_crypto_key_slot_put(ks);
>> +	mutex_unlock(&cs->mutex);
>> +}

hm I think you're right.

This is theoretically possible...
Userspace might be reneweing the key exactly when the primary key is 
expiring..

We need to specify the key_id and kill exactly that key.


>> +
> 
> [...]
>> +static void ovpn_aead_encrypt_done(void *data, int ret)
>> +{
>> +	struct sk_buff *skb = data;
>> +
>> +	aead_request_free(ovpn_skb_cb(skb)->req);
>> +	ovpn_encrypt_post(skb, ret);
>> +}
>> +
>> +int ovpn_aead_encrypt(struct ovpn_crypto_key_slot *ks, struct sk_buff *skb,
>> +		      u32 peer_id)
>> +{
>> +	const unsigned int tag_size = crypto_aead_authsize(ks->encrypt);
>> +	const unsigned int head_size = ovpn_aead_encap_overhead(ks);
>> +	struct scatterlist sg[MAX_SKB_FRAGS + 2];
>> +	DECLARE_CRYPTO_WAIT(wait);
> 
> unused? (also in _decrypt)

Right. Leftover from the sync crypto approach. Will drop it.

> 
> [...]
>> +
>> +	req = aead_request_alloc(ks->encrypt, GFP_ATOMIC);
>> +	if (unlikely(!req))
>> +		return -ENOMEM;
>> +
>> +	/* setup async crypto operation */
>> +	aead_request_set_tfm(req, ks->encrypt);
>> +	aead_request_set_callback(req, 0, ovpn_aead_encrypt_done, NULL);
> 
> NULL? That should be skb, ovpn_aead_encrypt_done needs it (same for
> decrypt).

Ouch

> 
> I suspect you haven't triggered the async path in testing. For that,
> you can use crconf:

Yeah, I doubt I did, although I tried pumping as much traffic as 
possible (but it may not have been enough to trigger the needed conditions).

> 
> git clone https://git.code.sf.net/p/crconf/code
> cd code && make
> ./src/crconf add driver 'pcrypt(generic-gcm-aesni)' type 3  priority 10000
> 
> Then all packets encrypted with gcm(aes) should go through the async
> code.

Amazing! Thanks! Will give it a go.

> 
>> +	aead_request_set_crypt(req, sg, sg, skb->len - head_size, iv);
>> +	aead_request_set_ad(req, OVPN_OP_SIZE_V2 + NONCE_WIRE_SIZE);
>> +
>> +	ovpn_skb_cb(skb)->req = req;
>> +	ovpn_skb_cb(skb)->ks = ks;
>> +
>> +	/* encrypt it */
>> +	return crypto_aead_encrypt(req);
>> +}
> 
> [...]
>> @@ -77,14 +133,45 @@ static void ovpn_decrypt_post(struct sk_buff *skb, int ret)
>>   /* pick next packet from RX queue, decrypt and forward it to the device */
>>   void ovpn_recv(struct ovpn_peer *peer, struct sk_buff *skb)
>>   {
>> +	struct ovpn_crypto_key_slot *ks;
>> +	u8 key_id;
>> +
>> +	/* get the key slot matching the key ID in the received packet */
>> +	key_id = ovpn_key_id_from_skb(skb);
>> +	ks = ovpn_crypto_key_id_to_slot(&peer->crypto, key_id);
> 
> This takes a reference on the keyslot (ovpn_crypto_key_slot_hold), but
> I don't see it getting released in ovpn_decrypt_post. In
> ovpn_encrypt_post you're adding a ovpn_crypto_key_slot_put (to match
> ovpn_crypto_key_slot_primary), but nothing equivalent in
> ovpn_decrypt_post?

good catch! I don't think it triggered any critical side effect (Except 
from the leak) hence it went unnoticed.

Will add the ovpn_crypto_key_slot_put in the exit path.


Thanks!


> 
>> +	if (unlikely(!ks)) {
>> +		net_info_ratelimited("%s: no available key for peer %u, key-id: %u\n",
>> +				     peer->ovpn->dev->name, peer->id, key_id);
>> +		dev_core_stats_rx_dropped_inc(peer->ovpn->dev);
>> +		kfree_skb(skb);
>> +		return;
>> +	}
>> +
>>   	ovpn_skb_cb(skb)->peer = peer;
>> -	ovpn_decrypt_post(skb, 0);
>> +	ovpn_decrypt_post(skb, ovpn_aead_decrypt(ks, skb));
>>   }
> 

-- 
Antonio Quartulli
OpenVPN Inc.