Commit af52020f authored by Antonio Quartulli's avatar Antonio Quartulli
Browse files

ovpn: reject unexpected netlink attributes 



Netlink ops do not expect all attributes to be always set, however
this condition is not explicitly coded any where, leading the user
to believe that all sent attributes are somewhat processed.

Fix this behaviour by introducing explicit checks.

For CMD_OVPN_PEER_GET and CMD_OVPN_KEY_GET directly open-code the
needed condition in the related ops handlers.
While for all other ops use attribute subsets in the ovpn.yaml spec file.

Fixes: b7a63391 ("ovpn: add basic netlink support")
Reported-by: default avatarRalf Lici <ralf@mandelbit.com>
Closes: https://github.com/OpenVPN/ovpn-net-next/issues/19


Signed-off-by: default avatarAntonio Quartulli <antonio@openvpn.net>
parent 4c88cfcc

Documentation/netlink/specs/ovpn.yaml

+147 −6
Original line number Diff line number Diff line
@@ -160,6 +160,66 @@ attribute-sets: 
        name: link-tx-packets

        type: uint

        doc: Number of packets transmitted at the transport level

  -

    name: peer-new-input

    subset-of: peer

    attributes:

      -

        name: id

      -

        name: remote-ipv4

      -

        name: remote-ipv6

      -

        name: remote-ipv6-scope-id

      -

        name: remote-port

      -

        name: socket

      -

        name: vpn-ipv4

      -

        name: vpn-ipv6

      -

        name: local-ipv4

      -

        name: local-ipv6

      -

        name: keepalive-interval

      -

        name: keepalive-timeout

  -

    name: peer-set-input

    subset-of: peer

    attributes:

      -

        name: id

      -

        name: remote-ipv4

      -

        name: remote-ipv6

      -

        name: remote-ipv6-scope-id

      -

        name: remote-port

      -

        name: vpn-ipv4

      -

        name: vpn-ipv6

      -

        name: local-ipv4

      -

        name: local-ipv6

      -

        name: keepalive-interval

      -

        name: keepalive-timeout

  -

    name: peer-del-input

    subset-of: peer

    attributes:

      -

        name: id

  -

    name: keyconf

    attributes:
@@ -216,6 +276,33 @@ attribute-sets: 
          obtain the actual cipher IV

        checks:

          exact-len: nonce-tail-size



  -

    name: keyconf-get

    subset-of: keyconf

    attributes:

      -

        name: peer-id

      -

        name: slot

      -

        name: key-id

      -

        name: cipher-alg

  -

    name: keyconf-swap-input

    subset-of: keyconf

    attributes:

      -

        name: peer-id

  -

    name: keyconf-del-input

    subset-of: keyconf

    attributes:

      -

        name: peer-id

      -

        name: slot

  -

    name: ovpn

    attributes:
@@ -235,12 +322,66 @@ attribute-sets: 
        type: nest

        doc: Peer specific cipher configuration

        nested-attributes: keyconf

  -

    name: ovpn-peer-new-input

    subset-of: ovpn

    attributes:

      -

        name: ifindex

      -

        name: peer

        nested-attributes: peer-new-input

  -

    name: ovpn-peer-set-input

    subset-of: ovpn

    attributes:

      -

        name: ifindex

      -

        name: peer

        nested-attributes: peer-set-input

  -

    name: ovpn-peer-del-input

    subset-of: ovpn

    attributes:

      -

        name: ifindex

      -

        name: peer

        nested-attributes: peer-del-input

  -

    name: ovpn-keyconf-get

    subset-of: ovpn

    attributes:

      -

        name: ifindex

      -

        name: keyconf

        nested-attributes: keyconf-get

  -

    name: ovpn-keyconf-swap-input

    subset-of: ovpn

    attributes:

      -

        name: ifindex

      -

        name: keyconf

        nested-attributes: keyconf-swap-input

  -

    name: ovpn-keyconf-del-input

    subset-of: ovpn

    attributes:

      -

        name: ifindex

      -

        name: keyconf

        nested-attributes: keyconf-del-input



operations:

  list:

    -

      name: peer-new

      attribute-set: ovpn

      attribute-set: ovpn-peer-new-input

      flags: [ admin-perm ]

      doc: Add a remote peer

      do:
@@ -252,7 +393,7 @@ operations: 
            - peer

    -

      name: peer-set

      attribute-set: ovpn

      attribute-set: ovpn-peer-set-input

      flags: [ admin-perm ]

      doc: modify a remote peer

      do:
@@ -286,7 +427,7 @@ operations: 
            - peer

    -

      name: peer-del

      attribute-set: ovpn

      attribute-set: ovpn-peer-del-input

      flags: [ admin-perm ]

      doc: Delete existing remote peer

      do:
@@ -316,7 +457,7 @@ operations: 
            - keyconf

    -

      name: key-get

      attribute-set: ovpn

      attribute-set: ovpn-keyconf-get

      flags: [ admin-perm ]

      doc: Retrieve non-sensitive data about peer key and cipher

      do:
@@ -331,7 +472,7 @@ operations: 
            - keyconf

    -

      name: key-swap

      attribute-set: ovpn

      attribute-set: ovpn-keyconf-swap-input

      flags: [ admin-perm ]

      doc: Swap primary and secondary session keys for a specific peer

      do:
@@ -350,7 +491,7 @@ operations: 
      mcgrp: peers

    -

      name: key-del

      attribute-set: ovpn

      attribute-set: ovpn-keyconf-del-input

      flags: [ admin-perm ]

      doc: Delete cipher key for a specific peer

      do:

drivers/net/ovpn/netlink-gen.c

+55 −6
Original line number Diff line number Diff line
@@ -29,6 +29,22 @@ const struct nla_policy ovpn_keyconf_nl_policy[OVPN_A_KEYCONF_DECRYPT_DIR + 1] = 
	[OVPN_A_KEYCONF_DECRYPT_DIR] = NLA_POLICY_NESTED(ovpn_keydir_nl_policy),

};



const struct nla_policy ovpn_keyconf_del_input_nl_policy[OVPN_A_KEYCONF_SLOT + 1] = {

	[OVPN_A_KEYCONF_PEER_ID] = NLA_POLICY_FULL_RANGE(NLA_U32, &ovpn_a_keyconf_peer_id_range),

	[OVPN_A_KEYCONF_SLOT] = NLA_POLICY_MAX(NLA_U32, 1),

};



const struct nla_policy ovpn_keyconf_get_nl_policy[OVPN_A_KEYCONF_CIPHER_ALG + 1] = {

	[OVPN_A_KEYCONF_PEER_ID] = NLA_POLICY_FULL_RANGE(NLA_U32, &ovpn_a_keyconf_peer_id_range),

	[OVPN_A_KEYCONF_SLOT] = NLA_POLICY_MAX(NLA_U32, 1),

	[OVPN_A_KEYCONF_KEY_ID] = NLA_POLICY_MAX(NLA_U32, 7),

	[OVPN_A_KEYCONF_CIPHER_ALG] = NLA_POLICY_MAX(NLA_U32, 2),

};



const struct nla_policy ovpn_keyconf_swap_input_nl_policy[OVPN_A_KEYCONF_PEER_ID + 1] = {

	[OVPN_A_KEYCONF_PEER_ID] = NLA_POLICY_FULL_RANGE(NLA_U32, &ovpn_a_keyconf_peer_id_range),

};



const struct nla_policy ovpn_keydir_nl_policy[OVPN_A_KEYDIR_NONCE_TAIL + 1] = {

	[OVPN_A_KEYDIR_CIPHER_KEY] = NLA_POLICY_MAX_LEN(256),

	[OVPN_A_KEYDIR_NONCE_TAIL] = NLA_POLICY_EXACT_LEN(OVPN_NONCE_TAIL_SIZE),
@@ -60,16 +76,49 @@ const struct nla_policy ovpn_peer_nl_policy[OVPN_A_PEER_LINK_TX_PACKETS + 1] = { 
	[OVPN_A_PEER_LINK_TX_PACKETS] = { .type = NLA_UINT, },

};



const struct nla_policy ovpn_peer_del_input_nl_policy[OVPN_A_PEER_ID + 1] = {

	[OVPN_A_PEER_ID] = NLA_POLICY_FULL_RANGE(NLA_U32, &ovpn_a_peer_id_range),

};



const struct nla_policy ovpn_peer_new_input_nl_policy[OVPN_A_PEER_KEEPALIVE_TIMEOUT + 1] = {

	[OVPN_A_PEER_ID] = NLA_POLICY_FULL_RANGE(NLA_U32, &ovpn_a_peer_id_range),

	[OVPN_A_PEER_REMOTE_IPV4] = { .type = NLA_BE32, },

	[OVPN_A_PEER_REMOTE_IPV6] = NLA_POLICY_EXACT_LEN(16),

	[OVPN_A_PEER_REMOTE_IPV6_SCOPE_ID] = { .type = NLA_U32, },

	[OVPN_A_PEER_REMOTE_PORT] = NLA_POLICY_MIN(NLA_BE16, 1),

	[OVPN_A_PEER_SOCKET] = { .type = NLA_U32, },

	[OVPN_A_PEER_VPN_IPV4] = { .type = NLA_BE32, },

	[OVPN_A_PEER_VPN_IPV6] = NLA_POLICY_EXACT_LEN(16),

	[OVPN_A_PEER_LOCAL_IPV4] = { .type = NLA_BE32, },

	[OVPN_A_PEER_LOCAL_IPV6] = NLA_POLICY_EXACT_LEN(16),

	[OVPN_A_PEER_KEEPALIVE_INTERVAL] = { .type = NLA_U32, },

	[OVPN_A_PEER_KEEPALIVE_TIMEOUT] = { .type = NLA_U32, },

};



const struct nla_policy ovpn_peer_set_input_nl_policy[OVPN_A_PEER_KEEPALIVE_TIMEOUT + 1] = {

	[OVPN_A_PEER_ID] = NLA_POLICY_FULL_RANGE(NLA_U32, &ovpn_a_peer_id_range),

	[OVPN_A_PEER_REMOTE_IPV4] = { .type = NLA_BE32, },

	[OVPN_A_PEER_REMOTE_IPV6] = NLA_POLICY_EXACT_LEN(16),

	[OVPN_A_PEER_REMOTE_IPV6_SCOPE_ID] = { .type = NLA_U32, },

	[OVPN_A_PEER_REMOTE_PORT] = NLA_POLICY_MIN(NLA_BE16, 1),

	[OVPN_A_PEER_VPN_IPV4] = { .type = NLA_BE32, },

	[OVPN_A_PEER_VPN_IPV6] = NLA_POLICY_EXACT_LEN(16),

	[OVPN_A_PEER_LOCAL_IPV4] = { .type = NLA_BE32, },

	[OVPN_A_PEER_LOCAL_IPV6] = NLA_POLICY_EXACT_LEN(16),

	[OVPN_A_PEER_KEEPALIVE_INTERVAL] = { .type = NLA_U32, },

	[OVPN_A_PEER_KEEPALIVE_TIMEOUT] = { .type = NLA_U32, },

};



/* OVPN_CMD_PEER_NEW - do */

static const struct nla_policy ovpn_peer_new_nl_policy[OVPN_A_PEER + 1] = {

	[OVPN_A_IFINDEX] = { .type = NLA_U32, },

	[OVPN_A_PEER] = NLA_POLICY_NESTED(ovpn_peer_nl_policy),

	[OVPN_A_PEER] = NLA_POLICY_NESTED(ovpn_peer_new_input_nl_policy),

};



/* OVPN_CMD_PEER_SET - do */

static const struct nla_policy ovpn_peer_set_nl_policy[OVPN_A_PEER + 1] = {

	[OVPN_A_IFINDEX] = { .type = NLA_U32, },

	[OVPN_A_PEER] = NLA_POLICY_NESTED(ovpn_peer_nl_policy),

	[OVPN_A_PEER] = NLA_POLICY_NESTED(ovpn_peer_set_input_nl_policy),

};



/* OVPN_CMD_PEER_GET - do */
@@ -86,7 +135,7 @@ static const struct nla_policy ovpn_peer_get_dump_nl_policy[OVPN_A_IFINDEX + 1] 
/* OVPN_CMD_PEER_DEL - do */

static const struct nla_policy ovpn_peer_del_nl_policy[OVPN_A_PEER + 1] = {

	[OVPN_A_IFINDEX] = { .type = NLA_U32, },

	[OVPN_A_PEER] = NLA_POLICY_NESTED(ovpn_peer_nl_policy),

	[OVPN_A_PEER] = NLA_POLICY_NESTED(ovpn_peer_del_input_nl_policy),

};



/* OVPN_CMD_KEY_NEW - do */
@@ -98,19 +147,19 @@ static const struct nla_policy ovpn_key_new_nl_policy[OVPN_A_KEYCONF + 1] = { 
/* OVPN_CMD_KEY_GET - do */

static const struct nla_policy ovpn_key_get_nl_policy[OVPN_A_KEYCONF + 1] = {

	[OVPN_A_IFINDEX] = { .type = NLA_U32, },

	[OVPN_A_KEYCONF] = NLA_POLICY_NESTED(ovpn_keyconf_nl_policy),

	[OVPN_A_KEYCONF] = NLA_POLICY_NESTED(ovpn_keyconf_get_nl_policy),

};



/* OVPN_CMD_KEY_SWAP - do */

static const struct nla_policy ovpn_key_swap_nl_policy[OVPN_A_KEYCONF + 1] = {

	[OVPN_A_IFINDEX] = { .type = NLA_U32, },

	[OVPN_A_KEYCONF] = NLA_POLICY_NESTED(ovpn_keyconf_nl_policy),

	[OVPN_A_KEYCONF] = NLA_POLICY_NESTED(ovpn_keyconf_swap_input_nl_policy),

};



/* OVPN_CMD_KEY_DEL - do */

static const struct nla_policy ovpn_key_del_nl_policy[OVPN_A_KEYCONF + 1] = {

	[OVPN_A_IFINDEX] = { .type = NLA_U32, },

	[OVPN_A_KEYCONF] = NLA_POLICY_NESTED(ovpn_keyconf_nl_policy),

	[OVPN_A_KEYCONF] = NLA_POLICY_NESTED(ovpn_keyconf_del_input_nl_policy),

};



/* Ops table for ovpn */

drivers/net/ovpn/netlink-gen.h

+6 −0
Original line number Diff line number Diff line
@@ -13,8 +13,14 @@ 


/* Common nested types */

extern const struct nla_policy ovpn_keyconf_nl_policy[OVPN_A_KEYCONF_DECRYPT_DIR + 1];

extern const struct nla_policy ovpn_keyconf_del_input_nl_policy[OVPN_A_KEYCONF_SLOT + 1];

extern const struct nla_policy ovpn_keyconf_get_nl_policy[OVPN_A_KEYCONF_CIPHER_ALG + 1];

extern const struct nla_policy ovpn_keyconf_swap_input_nl_policy[OVPN_A_KEYCONF_PEER_ID + 1];

extern const struct nla_policy ovpn_keydir_nl_policy[OVPN_A_KEYDIR_NONCE_TAIL + 1];

extern const struct nla_policy ovpn_peer_nl_policy[OVPN_A_PEER_LINK_TX_PACKETS + 1];

extern const struct nla_policy ovpn_peer_del_input_nl_policy[OVPN_A_PEER_ID + 1];

extern const struct nla_policy ovpn_peer_new_input_nl_policy[OVPN_A_PEER_KEEPALIVE_TIMEOUT + 1];

extern const struct nla_policy ovpn_peer_set_input_nl_policy[OVPN_A_PEER_KEEPALIVE_TIMEOUT + 1];



int ovpn_nl_pre_doit(const struct genl_split_ops *ops, struct sk_buff *skb,

		     struct genl_info *info);

drivers/net/ovpn/netlink.c

+43 −8
Original line number Diff line number Diff line
@@ -352,7 +352,7 @@ int ovpn_nl_peer_new_doit(struct sk_buff *skb, struct genl_info *info) 
		return -EINVAL;



	ret = nla_parse_nested(attrs, OVPN_A_PEER_MAX, info->attrs[OVPN_A_PEER],

			       ovpn_peer_nl_policy, info->extack);

			       ovpn_peer_new_input_nl_policy, info->extack);

	if (ret)

		return ret;
@@ -476,7 +476,7 @@ int ovpn_nl_peer_set_doit(struct sk_buff *skb, struct genl_info *info) 
		return -EINVAL;



	ret = nla_parse_nested(attrs, OVPN_A_PEER_MAX, info->attrs[OVPN_A_PEER],

			       ovpn_peer_nl_policy, info->extack);

			       ovpn_peer_set_input_nl_policy, info->extack);

	if (ret)

		return ret;
@@ -654,7 +654,7 @@ int ovpn_nl_peer_get_doit(struct sk_buff *skb, struct genl_info *info) 
	struct ovpn_peer *peer;

	struct sk_buff *msg;

	u32 peer_id;

	int ret;

	int ret, i;



	if (GENL_REQ_ATTR_CHECK(info, OVPN_A_PEER))

		return -EINVAL;
@@ -668,6 +668,23 @@ int ovpn_nl_peer_get_doit(struct sk_buff *skb, struct genl_info *info) 
			      OVPN_A_PEER_ID))

		return -EINVAL;



	/* OVPN_CMD_PEER_GET expects only the PEER_ID, therefore

	 * ensure that the user hasn't specified any other attribute.

	 *

	 * Unfortunately this check cannot be performed via netlink

	 * spec/policy and must be open-coded.

	 */

	for (i = 0; i < OVPN_A_PEER_MAX + 1; i++) {

		if (i == OVPN_A_PEER_ID)

			continue;



		if (attrs[i]) {

			NL_SET_ERR_MSG_FMT_MOD(info->extack,

					       "unexpected attribute %u", i);

			return -EINVAL;

		}

	}



	peer_id = nla_get_u32(attrs[OVPN_A_PEER_ID]);

	peer = ovpn_peer_get_by_id(ovpn, peer_id);

	if (!peer) {
@@ -768,7 +785,7 @@ int ovpn_nl_peer_del_doit(struct sk_buff *skb, struct genl_info *info) 
		return -EINVAL;



	ret = nla_parse_nested(attrs, OVPN_A_PEER_MAX, info->attrs[OVPN_A_PEER],

			       ovpn_peer_nl_policy, info->extack);

			       ovpn_peer_del_input_nl_policy, info->extack);

	if (ret)

		return ret;
@@ -969,14 +986,14 @@ int ovpn_nl_key_get_doit(struct sk_buff *skb, struct genl_info *info) 
	struct ovpn_peer *peer;

	struct sk_buff *msg;

	u32 peer_id;

	int ret;

	int ret, i;



	if (GENL_REQ_ATTR_CHECK(info, OVPN_A_KEYCONF))

		return -EINVAL;



	ret = nla_parse_nested(attrs, OVPN_A_KEYCONF_MAX,

			       info->attrs[OVPN_A_KEYCONF],

			       ovpn_keyconf_nl_policy, info->extack);

			       ovpn_keyconf_get_nl_policy, info->extack);

	if (ret)

		return ret;
@@ -988,6 +1005,24 @@ int ovpn_nl_key_get_doit(struct sk_buff *skb, struct genl_info *info) 
			      OVPN_A_KEYCONF_SLOT))

		return -EINVAL;



	/* OVPN_CMD_KEY_GET expects only the PEER_ID and the SLOT, therefore

	 * ensure that the user hasn't specified any other attribute.

	 *

	 * Unfortunately this check cannot be performed via netlink

	 * spec/policy and must be open-coded.

	 */

	for (i = 0; i < OVPN_A_KEYCONF_MAX + 1; i++) {

		if (i == OVPN_A_KEYCONF_PEER_ID ||

		    i == OVPN_A_KEYCONF_SLOT)

			continue;



		if (attrs[i]) {

			NL_SET_ERR_MSG_FMT_MOD(info->extack,

					       "unexpected attribute %u", i);

			return -EINVAL;

		}

	}



	peer_id = nla_get_u32(attrs[OVPN_A_KEYCONF_PEER_ID]);

	peer = ovpn_peer_get_by_id(ovpn, peer_id);

	if (!peer) {
@@ -1037,7 +1072,7 @@ int ovpn_nl_key_swap_doit(struct sk_buff *skb, struct genl_info *info) 


	ret = nla_parse_nested(attrs, OVPN_A_KEYCONF_MAX,

			       info->attrs[OVPN_A_KEYCONF],

			       ovpn_keyconf_nl_policy, info->extack);

			       ovpn_keyconf_swap_input_nl_policy, info->extack);

	if (ret)

		return ret;
@@ -1074,7 +1109,7 @@ int ovpn_nl_key_del_doit(struct sk_buff *skb, struct genl_info *info) 


	ret = nla_parse_nested(attrs, OVPN_A_KEYCONF_MAX,

			       info->attrs[OVPN_A_KEYCONF],

			       ovpn_keyconf_nl_policy, info->extack);

			       ovpn_keyconf_del_input_nl_policy, info->extack);

	if (ret)

		return ret;