diff mbox

[10/23] P2PS: Add validation for P2PS PD request

Message ID 1443116293-9323-11-git-send-email-ilan.peer@intel.com
State Accepted
Headers show

Commit Message

Peer, Ilan Sept. 24, 2015, 5:38 p.m. UTC
Validate that all the required attributes appear in a P2PS PD request,
and in addition in case of follow-on PD request, check that the given
values match those of the original PD request.

Signed-off-by: Ilan Peer <ilan.peer@intel.com>
---
 src/p2p/p2p_pd.c | 176 ++++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 149 insertions(+), 27 deletions(-)

Comments

Jouni Malinen Oct. 5, 2015, 4:40 p.m. UTC | #1
On Thu, Sep 24, 2015 at 08:38:00PM +0300, Ilan Peer wrote:
> Validate that all the required attributes appear in a P2PS PD request,
> and in addition in case of follow-on PD request, check that the given
> values match those of the original PD request.

This seems to be losing couple of checks and potentially allowing DoS
attacks due to NULL pointer dereferences..


> +static int p2ps_validate_pd_req(struct p2p_data *p2p,

> +	P2PS_PD_REQ_CHECK(1, adv_id);
> +	P2PS_PD_REQ_CHECK(1, session_id);
> +	P2PS_PD_REQ_CHECK(1, capability);
> +	P2PS_PD_REQ_CHECK(1, p2p_device_info);
> +	P2PS_PD_REQ_CHECK(1, feature_cap);

session_mac and adv_mac missing here..


> @@ -538,21 +645,21 @@ void p2p_process_prov_disc_req(struct p2p_data *p2p, const u8 *sa,
> -	if (!msg.adv_id || !msg.session_id || !msg.session_mac ||
> -	    !msg.adv_mac || !msg.feature_cap ||
> -	    msg.feature_cap_len < sizeof(*req_fcap) ||
> -	    !(msg.status || msg.conn_cap))

While they were checked here..

>  	req_fcap = (struct p2ps_feature_capab *) msg.feature_cap;

And are being dereferenced unconditionally after that line..

Am I missing something here? I added these to my work branch to avoid
NULL pointer dereference:

+       P2PS_PD_REQ_CHECK(1, session_mac);
+       P2PS_PD_REQ_CHECK(1, adv_mac);
Peer, Ilan Oct. 6, 2015, 6:56 p.m. UTC | #2
> -----Original Message-----
> From: Jouni Malinen [mailto:j@w1.fi]
> Sent: Monday, October 05, 2015 19:40
> To: Peer, Ilan
> Cc: hostap@lists.shmoo.com
> Subject: Re: [PATCH 10/23] P2PS: Add validation for P2PS PD request
> 
> On Thu, Sep 24, 2015 at 08:38:00PM +0300, Ilan Peer wrote:
> > Validate that all the required attributes appear in a P2PS PD request,
> > and in addition in case of follow-on PD request, check that the given
> > values match those of the original PD request.
> 
> This seems to be losing couple of checks and potentially allowing DoS attacks
> due to NULL pointer dereferences..
> 
> 
> > +static int p2ps_validate_pd_req(struct p2p_data *p2p,
> 
> > +	P2PS_PD_REQ_CHECK(1, adv_id);
> > +	P2PS_PD_REQ_CHECK(1, session_id);
> > +	P2PS_PD_REQ_CHECK(1, capability);
> > +	P2PS_PD_REQ_CHECK(1, p2p_device_info);
> > +	P2PS_PD_REQ_CHECK(1, feature_cap);
> 
> session_mac and adv_mac missing here..

These are unconditionally set when session_id and adv_mac are set in p2p_parse_attribute(), so I assumed it is ok to skip these checks.

Regards,

Ilan.
Jouni Malinen Oct. 7, 2015, 1:20 p.m. UTC | #3
On Tue, Oct 06, 2015 at 06:56:41PM +0000, Peer, Ilan wrote:
> > From: Jouni Malinen [mailto:j@w1.fi]
> > > +static int p2ps_validate_pd_req(struct p2p_data *p2p,
> > 
> > > +	P2PS_PD_REQ_CHECK(1, adv_id);
> > > +	P2PS_PD_REQ_CHECK(1, session_id);
> > > +	P2PS_PD_REQ_CHECK(1, capability);
> > > +	P2PS_PD_REQ_CHECK(1, p2p_device_info);
> > > +	P2PS_PD_REQ_CHECK(1, feature_cap);
> > 
> > session_mac and adv_mac missing here..

> These are unconditionally set when session_id and adv_mac are set in p2p_parse_attribute(), so I assumed it is ok to skip these checks.

Ah, yes, you're correct. That said, I think I'm going to leave the
explicit checks in place since I'm not confident that static analyzers
would be able to follow the logic here and could end up reporting false
issues related to possible NULL dereference.. In any case, the extra
checks do not cause any harm here apart from making the implement a tiny
bit larger and slower.
diff mbox

Patch

diff --git a/src/p2p/p2p_pd.c b/src/p2p/p2p_pd.c
index 723e85c..1dffc89 100644
--- a/src/p2p/p2p_pd.c
+++ b/src/p2p/p2p_pd.c
@@ -426,6 +426,113 @@  static u8 p2ps_own_preferred_cpt(const u8 *cpt_priority, u8 req_cpt_mask)
 	return 0;
 }
 
+/* Check if the message contains a valid P2PS PD request */
+static int p2ps_validate_pd_req(struct p2p_data *p2p,
+				struct p2p_message *msg,
+				const u8 *addr)
+{
+	u8 group_id = 0;
+	u8 intended_addr = 0;
+	u8 operating_channel = 0;
+	u8 channel_list = 0;
+	u8 config_timeout = 0;
+	u8 listen_channel = 0;
+
+#define P2PS_PD_REQ_CHECK(_val, _attr) \
+do { \
+	if ((_val) && !msg->_attr) { \
+		p2p_dbg(p2p, "Not P2PS PD req. Missing %s", #_attr); \
+		return -1; \
+	} \
+} while (0)
+
+	P2PS_PD_REQ_CHECK(1, adv_id);
+	P2PS_PD_REQ_CHECK(1, session_id);
+	P2PS_PD_REQ_CHECK(1, capability);
+	P2PS_PD_REQ_CHECK(1, p2p_device_info);
+	P2PS_PD_REQ_CHECK(1, feature_cap);
+
+	/* We don't need to check Connection Capability, Persistent Group
+	 * and related attributes for follow-on PD request with a status
+	 * other than SUCCESS_DEFERRED.
+	 */
+	if (msg->status && *msg->status != P2P_SC_SUCCESS_DEFERRED)
+		return 0;
+
+	P2PS_PD_REQ_CHECK(1, conn_cap);
+
+	/* Note 1: A feature capability attribute structure can be changed
+	 * in the future. The assumption is that such modifications are
+	 * backward compatible, therefore we allow processing of msg.feature_cap
+	 * exceeding a size of p2ps_feature_capab structure.
+	 * Note 2: A verification of msg.feature_cap_len below has to be
+	 * changed to allow 2 byte feature capability processing
+	 * if struct p2ps_feature_capab is extended to include
+	 * additional fields and it affects the structure size.
+	 */
+	if (msg->feature_cap_len < sizeof(struct p2ps_feature_capab)) {
+		p2p_dbg(p2p, "P2PS: invalid feature capability len");
+		return -1;
+	}
+
+	switch (*msg->conn_cap) {
+	case P2PS_SETUP_NEW:
+		group_id = 1;
+		intended_addr = 1;
+		operating_channel = 1;
+		channel_list = 1;
+		config_timeout = 1;
+		listen_channel = 1;
+		break;
+	case P2PS_SETUP_CLIENT:
+		channel_list = 1;
+		listen_channel = 1;
+		break;
+	case P2PS_SETUP_GROUP_OWNER:
+		group_id = 1;
+		intended_addr = 1;
+		operating_channel = 1;
+		break;
+	case P2PS_SETUP_NEW | P2PS_SETUP_GROUP_OWNER:
+		group_id = 1;
+		operating_channel = 1;
+		intended_addr = 1;
+		channel_list = 1;
+		config_timeout = 1;
+		break;
+	case P2PS_SETUP_CLIENT | P2PS_SETUP_GROUP_OWNER:
+		group_id = 1;
+		intended_addr = 1;
+		operating_channel = 1;
+		channel_list = 1;
+		config_timeout = 1;
+		break;
+	default:
+		p2p_dbg(p2p, "Invalid P2PS PD connection capability");
+		return -1;
+	}
+
+	if (msg->persistent_dev) {
+		channel_list = 1;
+		config_timeout = 1;
+		if (os_memcmp(msg->persistent_dev, addr, ETH_ALEN) == 0) {
+			intended_addr = 1;
+			operating_channel = 1;
+		}
+	}
+
+	P2PS_PD_REQ_CHECK(group_id, group_id);
+	P2PS_PD_REQ_CHECK(intended_addr, intended_addr);
+	P2PS_PD_REQ_CHECK(operating_channel, operating_channel);
+	P2PS_PD_REQ_CHECK(channel_list, channel_list);
+	P2PS_PD_REQ_CHECK(config_timeout, config_timeout);
+	P2PS_PD_REQ_CHECK(listen_channel, listen_channel);
+
+#undef P2PS_PD_REQ_CHECK
+
+	return 0;
+}
+
 
 void p2p_process_prov_disc_req(struct p2p_data *p2p, const u8 *sa,
 			       const u8 *data, size_t len, int rx_freq)
@@ -538,21 +645,21 @@  void p2p_process_prov_disc_req(struct p2p_data *p2p, const u8 *sa,
 	os_memset(session_mac, 0, ETH_ALEN);
 	os_memset(adv_mac, 0, ETH_ALEN);
 
-	/* Note 1: A feature capability attribute structure can be changed
-	 * in the future. The assumption is that such modifications are
-	 * backwards compatible, therefore we allow processing of
-	 * msg.feature_cap exceeding the size of the p2ps_feature_capab
-	 * structure.
-	 * Note 2: Vverification of msg.feature_cap_len below has to be changed
-	 * to allow 2 byte feature capability processing if struct
-	 * p2ps_feature_capab is extended to include additional fields and it
-	 * affects the structure size.
+	/* Validate a P2PS PD request + skip P2PS processing if not a valid P2PS
+	 * PD.
 	 */
-	if (!msg.adv_id || !msg.session_id || !msg.session_mac ||
-	    !msg.adv_mac || !msg.feature_cap ||
-	    msg.feature_cap_len < sizeof(*req_fcap) ||
-	    !(msg.status || msg.conn_cap))
+	if (msg.adv_id) {
+		/* set adv_id here, so in case of an error, a P2PS PD response
+		 * would be sent
+		 */
+		adv_id = WPA_GET_LE32(msg.adv_id);
+		if (p2ps_validate_pd_req(p2p, &msg, sa) < 0) {
+			reject = P2P_SC_FAIL_INVALID_PARAMS;
+			goto out;
+		}
+	} else {
 		goto out;
+	}
 
 	req_fcap = (struct p2ps_feature_capab *) msg.feature_cap;
 
@@ -563,17 +670,28 @@  void p2p_process_prov_disc_req(struct p2p_data *p2p, const u8 *sa,
 	os_memcpy(adv_mac, msg.adv_mac, ETH_ALEN);
 
 	session_id = WPA_GET_LE32(msg.session_id);
-	adv_id = WPA_GET_LE32(msg.adv_id);
-
-	if (!msg.status)
-		p2ps_adv = p2p_service_p2ps_id(p2p, adv_id);
-
-	p2p_dbg(p2p, "adv_id: %x - p2ps_adv - %p", adv_id, p2ps_adv);
 
 	if (msg.conn_cap)
 		conncap = *msg.conn_cap;
 	remote_conncap = conncap;
 
+	if (!msg.status) {
+		if (os_memcmp(p2p->cfg->dev_addr, msg.adv_mac, ETH_ALEN)) {
+			p2p_dbg(p2p,
+				"P2PS PD adv mac does not match the local one");
+			reject = P2P_SC_FAIL_INCOMPATIBLE_PARAMS;
+			goto out;
+		}
+
+		p2ps_adv = p2p_service_p2ps_id(p2p, adv_id);
+		if (!p2ps_adv) {
+			p2p_dbg(p2p, "P2PS PD invalid adv_id=0x%X", adv_id);
+			reject = P2P_SC_FAIL_INCOMPATIBLE_PARAMS;
+			goto out;
+		}
+		p2p_dbg(p2p, "adv_id: 0x%X, p2ps_adv: %p", adv_id, p2ps_adv);
+	}
+
 	if (p2ps_adv) {
 		auto_accept = p2ps_adv->auto_accept;
 		conncap = p2p->cfg->p2ps_group_capability(
@@ -619,11 +737,6 @@  void p2p_process_prov_disc_req(struct p2p_data *p2p, const u8 *sa,
 		if (auto_accept || reject != P2P_SC_SUCCESS) {
 			struct p2ps_provision *tmp;
 
-			if (reject == P2P_SC_SUCCESS && !conncap) {
-				reject =
-					P2P_SC_FAIL_INCOMPATIBLE_PARAMS;
-			}
-
 			if (p2ps_setup_p2ps_prov(
 				    p2p, adv_id, session_id,
 				    msg.wps_config_methods,
@@ -644,9 +757,6 @@  void p2p_process_prov_disc_req(struct p2p_data *p2p, const u8 *sa,
 			if (reject != P2P_SC_SUCCESS)
 				goto out;
 		}
-	} else if (!msg.status) {
-		reject = P2P_SC_FAIL_INCOMPATIBLE_PARAMS;
-		goto out;
 	}
 
 	if (!msg.status && !auto_accept &&
@@ -682,6 +792,18 @@  void p2p_process_prov_disc_req(struct p2p_data *p2p, const u8 *sa,
 	    !p2p->p2ps_prov)
 		goto out;
 
+	if (p2p->p2ps_prov->adv_id != adv_id ||
+	    os_memcmp(p2p->p2ps_prov->adv_mac, msg.adv_mac, ETH_ALEN)) {
+		p2p_dbg(p2p, "P2PS Follow-on PD with mismatch Advertisement ID");
+		goto out;
+	}
+
+	if (p2p->p2ps_prov->session_id != session_id ||
+	    os_memcmp(p2p->p2ps_prov->session_mac, msg.session_mac, ETH_ALEN)) {
+		p2p_dbg(p2p, "P2PS Follow-on PD with mismatch Session ID");
+		goto out;
+	}
+
 	method = p2p->p2ps_prov->method;
 
 	conncap = p2p->cfg->p2ps_group_capability(