diff mbox

P2P: Add error message for invalid PIN

Message ID CAFk-A4mWbDwLUW_wovRrpKcXENNoCTiYjsMrjGDgwnY2wUPn9Q@mail.gmail.com
State Accepted
Headers show

Commit Message

Masashi Honma March 6, 2012, 11:49 a.m. UTC 
2012/3/5 Jouni Malinen <j@w1.fi>:
> On Sun, Mar 04, 2012 at 12:09:37PM +0900, Masashi Honma wrote:
>> The second argument of p2p_connect command is "pin", "pbc" or PIN number.
>>
>> But PIN number is not checked. So the user typed "pbd" can not
>> recognize he mistyped.
>> Because the command returns "OK".
>>
>> So I made a patch adds a error message.
>
> This may be a bit too restrictive conditions. While 4-digit PINs are not
> very commonly used, I think that they would be allowed for P2P use
> cases. As such, I don't think I can apply this patch in its current
> form. In addition, user-generated 8-digit PINs are not required to
> include a checksum digit, so the use of wps_pin_valid() may not actually
> be valid for some use cases.
>
> I would be fine with a change that verifies that the PIN value is either
> a 4-digit or 8-digit number, but I'm not sure whether we can make this
> command any more restrictive than that. Please note that the separate
> wps_check_pin command was added for this same reason instead of making
> wps_pin command validate the PIN values.

Thanks for your advice. I relaxed the restriction of new patch.

>
>
> PS.
>
> Please read the CONTRIBUTIONS file in the top of the tree and use the
> Signed-hostap: line in future contributions to the project.


Signed-hostap: Masashi Honma <masashi.honma@gmail.com>

 	int ret;
@@ -2704,6 +2705,7 @@ static int p2p_ctrl_connect(struct
wpa_supplicant *wpa_s, char *cmd,
 	int auth;
 	int go_intent = -1;
 	int freq = 0;
+	long int val;

 	/* <addr> <"pbc" | "pin" | PIN> [label|display|keypad] [persistent]
 	 * [join] [auth] [go_intent=<0..15>] [freq=<in MHz>] */
@@ -2750,6 +2752,13 @@ static int p2p_ctrl_connect(struct
wpa_supplicant *wpa_s, char *cmd,
 			if (os_strncmp(pos, "display", 7) == 0)
 				wps_method = WPS_PIN_DISPLAY;
 		}
+		val = strtol(pin, &end, 10);
+		val = val; // workaround for a compiler warning
+		if ((os_strlen(pin) != 4 && os_strlen(pin) != 8) ||
+		    *end != '\0') {
+			os_memcpy(buf, "FAIL-INVALID-PIN\n", 17);
+			return 17;
+		}
 	}

 	new_pin = wpas_p2p_connect(wpa_s, addr, pin, wps_method,


Regards,
Masashi Honma.

Comments

Jouni Malinen June 30, 2012, 6:31 p.m. UTC | #1 
On Tue, Mar 06, 2012 at 08:49:58PM +0900, Masashi Honma wrote:
> Thanks for your advice. I relaxed the restriction of new patch.

Thanks, applied.

> diff --git a/wpa_supplicant/ctrl_iface.c b/wpa_supplicant/ctrl_iface.c
> @@ -2750,6 +2752,13 @@ static int p2p_ctrl_connect(struct
> +		val = strtol(pin, &end, 10);
> +		val = val; // workaround for a compiler warning
> +		if ((os_strlen(pin) != 4 && os_strlen(pin) != 8) ||
> +		    *end != '\0') {

Though, I replaced that val = val; workaround with val < 0 check to
avoid accepting values like "-123" as a valid PIN. This would still
accept "+123".. I ended up moving this to a common WPS function and
implementing the validation with a loop that verifies that each
character is a digit to avoid that type of corner cases.
diff mbox

Patch

diff --git a/wpa_supplicant/ctrl_iface.c b/wpa_supplicant/ctrl_iface.c
index 11f4674..f98d403 100644
--- a/wpa_supplicant/ctrl_iface.c
+++ b/wpa_supplicant/ctrl_iface.c
@@ -2696,6 +2696,7 @@  static int p2p_ctrl_connect(struct
wpa_supplicant *wpa_s, char *cmd,
 	u8 addr[ETH_ALEN];
 	char *pos, *pos2;
 	char *pin = NULL;
+	char *end;
 	enum p2p_wps_method wps_method;
 	int new_pin;