Commit 61c14937 authored by Daniel Verkamp's avatar Daniel Verkamp Committed by Jim Harris
Browse files

iscsi: disallow netmask prefix of 0 bits 



Netmasks with 0 bits of prefix do not make sense.  For example,
192.168.1.2/0 would allow hosts from any address, but this is
indicated with a special "ANY" value rather than a normal netmask.

Netmask prefixes of the full address size (e.g. 32 for IPv4 and 128 for
IPv6) are still allowed, since this represents a valid configuration
that matches a single specific address.

This also allows the IPv4 netmask math to be done entirely in uint32_t
instead of promoting to unsigned long long to avoid undefined behavior
when bits == 0 (shift count would have been 32 in that case).

Change-Id: I021b718e6a46f628c96a358edae816de81cd8929
Signed-off-by: default avatarDaniel Verkamp <daniel.verkamp@intel.com>
Reviewed-on: https://review.gerrithub.io/392969


Tested-by: default avatarSPDK Automated Test System <sys_sgsw@intel.com>
Reviewed-by: default avatar <shuhei.matsumoto.xt@hitachi.com>
Reviewed-by: default avatarBen Walker <benjamin.walker@intel.com>
Reviewed-by: default avatarJim Harris <james.r.harris@intel.com>
parent c315d8e8

lib/iscsi/tgt_node.c

+3 −3
Original line number Diff line number Diff line
@@ -79,7 +79,7 @@ spdk_iscsi_ipv6_netmask_allow_addr(const char *netmask, const char *addr) 


	if (p[0] == '/') {

		bits = (int) strtol(p + 1, NULL, 10);

		if (bits < 0 || bits > 128) {

		if (bits <= 0 || bits > 128) {

			return false;

		}

	} else {
@@ -139,7 +139,7 @@ spdk_iscsi_ipv4_netmask_allow_addr(const char *netmask, const char *addr) 


	if (p[0] == '/') {

		bits = (int) strtol(p + 1, NULL, 10);

		if (bits < 0 || bits > 32) {

		if (bits <= 0 || bits > 32) {

			return false;

		}

	} else {
@@ -153,7 +153,7 @@ spdk_iscsi_ipv4_netmask_allow_addr(const char *netmask, const char *addr) 
	}



	/* check 32bits */

	bmask = (0xffffffffULL << (32 - bits)) & 0xffffffffU;

	bmask = (0xffffffffU << (32 - bits)) & 0xffffffffU;

	if ((ntohl(in4_mask.s_addr) & bmask) != (ntohl(in4_addr.s_addr) & bmask)) {

		return false;

	}

test/unit/lib/iscsi/tgt_node.c/tgt_node_ut.c

+78 −0
Original line number Diff line number Diff line
@@ -157,6 +157,12 @@ allow_ipv6_allowed(void) 


	result = spdk_iscsi_netmask_allow_addr(netmask, addr);

	CU_ASSERT(result == true);



	/* Netmask prefix bits == 128 (all bits must match) */

	netmask = "[2001:ad6:1234:5678:9abc::1]/128";

	addr = "2001:ad6:1234:5678:9abc::1";

	result = spdk_iscsi_ipv6_netmask_allow_addr(netmask, addr);

	CU_ASSERT(result == true);

}



static void
@@ -174,6 +180,38 @@ allow_ipv6_denied(void) 


	result = spdk_iscsi_netmask_allow_addr(netmask, addr);

	CU_ASSERT(result == false);



	/* Netmask prefix bits == 128 (all bits must match) */

	netmask = "[2001:ad6:1234:5678:9abc::1]/128";

	addr = "2001:ad6:1234:5678:9abc::2";

	result = spdk_iscsi_ipv6_netmask_allow_addr(netmask, addr);

	CU_ASSERT(result == false);

}



static void

allow_ipv6_invalid(void)

{

	bool result;

	char *netmask;

	char *addr;



	/* Netmask prefix bits > 128 */

	netmask = "[2001:ad6:1234::]/129";

	addr = "2001:ad6:1234:5678:9abc::";

	result = spdk_iscsi_ipv6_netmask_allow_addr(netmask, addr);

	CU_ASSERT(result == false);



	/* Netmask prefix bits == 0 */

	netmask = "[2001:ad6:1234::]/0";

	addr = "2001:ad6:1234:5678:9abc::";

	result = spdk_iscsi_ipv6_netmask_allow_addr(netmask, addr);

	CU_ASSERT(result == false);



	/* Netmask prefix bits < 0 */

	netmask = "[2001:ad6:1234::]/-1";

	addr = "2001:ad6:1234:5678:9abc::";

	result = spdk_iscsi_ipv6_netmask_allow_addr(netmask, addr);

	CU_ASSERT(result == false);

}



static void
@@ -191,6 +229,12 @@ allow_ipv4_allowed(void) 


	result = spdk_iscsi_netmask_allow_addr(netmask, addr);

	CU_ASSERT(result == true);



	/* Netmask prefix == 32 (all bits must match) */

	netmask = "192.168.2.1/32";

	addr = "192.168.2.1";

	result = spdk_iscsi_ipv4_netmask_allow_addr(netmask, addr);

	CU_ASSERT(result == true);

}



static void
@@ -208,6 +252,38 @@ allow_ipv4_denied(void) 


	result = spdk_iscsi_netmask_allow_addr(netmask, addr);

	CU_ASSERT(result == false);



	/* Netmask prefix == 32 (all bits must match) */

	netmask = "192.168.2.1/32";

	addr = "192.168.2.2";

	result = spdk_iscsi_ipv4_netmask_allow_addr(netmask, addr);

	CU_ASSERT(result == false);

}



static void

allow_ipv4_invalid(void)

{

	bool result;

	char *netmask;

	char *addr;



	/* Netmask prefix bits > 32 */

	netmask = "192.168.2.0/33";

	addr = "192.168.2.1";

	result = spdk_iscsi_ipv4_netmask_allow_addr(netmask, addr);

	CU_ASSERT(result == false);



	/* Netmask prefix bits == 0 */

	netmask = "192.168.2.0/0";

	addr = "192.168.2.1";

	result = spdk_iscsi_ipv4_netmask_allow_addr(netmask, addr);

	CU_ASSERT(result == false);



	/* Netmask prefix bits < 0 */

	netmask = "192.168.2.0/-1";

	addr = "192.168.2.1";

	result = spdk_iscsi_ipv4_netmask_allow_addr(netmask, addr);

	CU_ASSERT(result == false);

}



static void
@@ -683,8 +759,10 @@ main(int argc, char **argv) 
		|| CU_add_test(suite, "allow any allowed case", allow_any_allowed) == NULL

		|| CU_add_test(suite, "allow ipv6 allowed case", allow_ipv6_allowed) == NULL

		|| CU_add_test(suite, "allow ipv6 denied case", allow_ipv6_denied) == NULL

		|| CU_add_test(suite, "allow ipv6 invalid case", allow_ipv6_invalid) == NULL

		|| CU_add_test(suite, "allow ipv4 allowed case", allow_ipv4_allowed) == NULL

		|| CU_add_test(suite, "allow ipv4 denied case", allow_ipv4_denied) == NULL

		|| CU_add_test(suite, "allow ipv4 invalid case", allow_ipv4_invalid) == NULL

		|| CU_add_test(suite, "node access allowed case", node_access_allowed) == NULL

		|| CU_add_test(suite, "node access denied case (empty netmask)",

			       node_access_denied_by_empty_netmask) == NULL