[PATCH 1/2] Pass length of PIN through callbacks.

[PATCH 1/2] Pass length of PIN through callbacks.

[Scott James Remnant]

In HCI, the PIN is a sequence of octets always accompanied by a length,
which means that a NULL byte is valid within a PIN. Indeed, some devices
use their BD_ADDR (or the host's) as a PIN, and these do have 0x00 bytes.

Adjust the pincode callbacks to always pass a length with the PIN, so we
use the initially calculated length from the D-Bus String rather than
calculating separately later.
---
 src/agent.c  |   10 +++++-----
 src/agent.h  |    2 +-
 src/device.c |    6 +++---
 src/event.c  |    6 +++---
 4 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/src/agent.c b/src/agent.c
index 9b942e8..4477210 100644
--- a/src/agent.c
+++ b/src/agent.c
@@ -160,7 +160,7 @@ void agent_free(struct agent *agent)
 		switch (agent->request->type) {
 		case AGENT_REQUEST_PINCODE:
 			pincode_cb = agent->request->cb;
-			pincode_cb(agent, &err, NULL, agent->request->user_data);
+			pincode_cb(agent, &err, NULL, 0, agent->request->user_data);
 			break;
 		default:
 			cb = agent->request->cb;
@@ -367,7 +367,7 @@ static void pincode_reply(DBusPendingCall *call, void *user_data)
 		error("Agent %s replied with an error: %s, %s",
 				agent->path, err.name, err.message);
 
-		cb(agent, &err, NULL, req->user_data);
+		cb(agent, &err, NULL, 0, req->user_data);
 		dbus_error_free(&err);
 		goto done;
 	}
@@ -377,7 +377,7 @@ static void pincode_reply(DBusPendingCall *call, void *user_data)
 				DBUS_TYPE_STRING, &pin,
 				DBUS_TYPE_INVALID)) {
 		error("Wrong passkey reply signature: %s", err.message);
-		cb(agent, &err, NULL, req->user_data);
+		cb(agent, &err, NULL, 0, req->user_data);
 		dbus_error_free(&err);
 		goto done;
 	}
@@ -389,12 +389,12 @@ static void pincode_reply(DBusPendingCall *call, void *user_data)
 		error("Invalid PIN length (%zu) from agent", len);
 		dbus_set_error_const(&err, "org.bluez.Error.InvalidArgs",
 					"Invalid passkey length");
-		cb(agent, &err, NULL, req->user_data);
+		cb(agent, &err, NULL, 0, req->user_data);
 		dbus_error_free(&err);
 		goto done;
 	}
 
-	cb(agent, NULL, pin, req->user_data);
+	cb(agent, NULL, pin, len, req->user_data);
 
 done:
 	if (message)
diff --git a/src/agent.h b/src/agent.h
index f62bf3b..42d90e6 100644
--- a/src/agent.h
+++ b/src/agent.h
@@ -28,7 +28,7 @@ typedef void (*agent_cb) (struct agent *agent, DBusError *err,
 				void *user_data);
 
 typedef void (*agent_pincode_cb) (struct agent *agent, DBusError *err,
-					const char *pincode, void *user_data);
+					const char *pincode, ssize_t pinlen, void *user_data);
 
 typedef void (*agent_passkey_cb) (struct agent *agent, DBusError *err,
 					uint32_t passkey, void *user_data);
diff --git a/src/device.c b/src/device.c
index 16855b1..3745119 100644
--- a/src/device.c
+++ b/src/device.c
@@ -2455,7 +2455,7 @@ void device_cancel_bonding(struct btd_device *device, uint8_t status)
 }
 
 static void pincode_cb(struct agent *agent, DBusError *err,
-					const char *pincode, void *data)
+				const char *pincode, ssize_t pinlen, void *data)
 {
 	struct authentication_req *auth = data;
 	struct btd_device *device = auth->device;
@@ -2481,7 +2481,7 @@ done:
 	if (auth->cb == NULL)
 		return;
 
-	((agent_pincode_cb) auth->cb)(agent, err, pincode, device);
+	((agent_pincode_cb) auth->cb)(agent, err, pincode, pinlen, device);
 
 	device->authr->cb = NULL;
 	device->authr->agent = NULL;
@@ -2629,7 +2629,7 @@ static void cancel_authentication(struct authentication_req *auth)
 
 	switch (auth->type) {
 	case AUTH_TYPE_PINCODE:
-		((agent_pincode_cb) auth->cb)(agent, &err, NULL, device);
+		((agent_pincode_cb) auth->cb)(agent, &err, NULL, 0, device);
 		break;
 	case AUTH_TYPE_CONFIRM:
 		((agent_cb) auth->cb)(agent, &err, device);
diff --git a/src/event.c b/src/event.c
index 6854990..906b1c5 100644
--- a/src/event.c
+++ b/src/event.c
@@ -87,7 +87,8 @@ static gboolean get_adapter_and_device(bdaddr_t *src, bdaddr_t *dst,
  *****************************************************************/
 
 static void pincode_cb(struct agent *agent, DBusError *derr,
-				const char *pincode, struct btd_device *device)
+			const char *pincode, ssize_t pinlen,
+			struct btd_device *device)
 {
 	struct btd_adapter *adapter = device_get_adapter(device);
 	bdaddr_t dba;
@@ -102,8 +103,7 @@ static void pincode_cb(struct agent *agent, DBusError *derr,
 		return;
 	}
 
-	err = btd_adapter_pincode_reply(adapter, &dba, pincode,
-						pincode ? strlen(pincode) : 0);
+	err = btd_adapter_pincode_reply(adapter, &dba, pincode, pinlen);
 	if (err < 0)
 		goto fail;
 
-- 
1.7.7.3

[PATCH 2/2] agent: allow agent to reply to RequestPinCode with bytes

[Scott James Remnant]

Since the specification allows the PIN to be a sequence of arbitrary
octets, and not a UTF-8 string, allow the userspace agent to reply
with such a sequence (as a D-Bus Array of Bytes).

This means a userspace agent can deal with devices that require their
BD_ADDR (or the host's) as a PIN, and take care of trying multiple of
such requirements.
---
 src/agent.c |   16 ++++++++++++----
 1 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/src/agent.c b/src/agent.c
index 4477210..3b9e849 100644
--- a/src/agent.c
+++ b/src/agent.c
@@ -353,7 +353,8 @@ static void pincode_reply(DBusPendingCall *call, void *user_data)
 	DBusMessage *message;
 	DBusError err;
 	bdaddr_t sba;
-	size_t len;
+	int32_t array_len;
+	ssize_t len;
 	char *pin;
 
 	adapter_get_address(adapter, &sba);
@@ -373,17 +374,24 @@ static void pincode_reply(DBusPendingCall *call, void *user_data)
 	}
 
 	dbus_error_init(&err);
-	if (!dbus_message_get_args(message, &err,
+	if (dbus_message_get_args(message, &err,
 				DBUS_TYPE_STRING, &pin,
 				DBUS_TYPE_INVALID)) {
+		len = strlen(pin);
+
+	} else if (dbus_message_get_args(message, &err,
+				DBUS_TYPE_ARRAY, DBUS_TYPE_BYTE, &pin, &array_len,
+				DBUS_TYPE_INVALID)) {
+		/* convert int32_t to ssize_t (probably the same length) */
+		len = array_len;
+
+	} else {
 		error("Wrong passkey reply signature: %s", err.message);
 		cb(agent, &err, NULL, 0, req->user_data);
 		dbus_error_free(&err);
 		goto done;
 	}
 
-	len = strlen(pin);
-
 	dbus_error_init(&err);
 	if (len > 16 || len < 1) {
 		error("Invalid PIN length (%zu) from agent", len);
-- 
1.7.7.3

Re: [PATCH 2/2] agent: allow agent to reply to RequestPinCode with bytes

[Marcel Holtmann]

Hi Scott,

> Since the specification allows the PIN to be a sequence of arbitrary
> octets, and not a UTF-8 string, allow the userspace agent to reply
> with such a sequence (as a D-Bus Array of Bytes).
> 
> This means a userspace agent can deal with devices that require their
> BD_ADDR (or the host's) as a PIN, and take care of trying multiple of
> such requirements.

this has been discussed before and the answer is a clear no to this.

You can however write a plugin that can handle binary PIN codes.

Regards

Marcel

Re: [PATCH 2/2] agent: allow agent to reply to RequestPinCode with bytes

[Scott James Remnant]

[-- Attachment #1: Type: text/plain, Size: 776 bytes --]

On Sat, Jan 14, 2012 at 8:41 AM, Marcel Holtmann <marcel@holtmann.org>wrote:


> > Since the specification allows the PIN to be a sequence of arbitrary
> > octets, and not a UTF-8 string, allow the userspace agent to reply
> > with such a sequence (as a D-Bus Array of Bytes).
> >
> > This means a userspace agent can deal with devices that require their
> > BD_ADDR (or the host's) as a PIN, and take care of trying multiple of
> > such requirements.
>
> this has been discussed before and the answer is a clear no to this.
>
>
Why a no? I couldn't find the discussion in the archives.


> You can however write a plugin that can handle binary PIN codes.
>
>
The problem there is that the plugin can only try one, and can't retry the
authentication if that PIN fails.

Scott

[-- Attachment #2: Type: text/html, Size: 1297 bytes --]

Re: [PATCH 2/2] agent: allow agent to reply to RequestPinCode with bytes

[Scott James Remnant]

On Sat, Jan 14, 2012 at 8:41 AM, Marcel Holtmann <marcel@holtmann.org> wrote:

> > Since the specification allows the PIN to be a sequence of arbitrary
> > octets, and not a UTF-8 string, allow the userspace agent to reply
> > with such a sequence (as a D-Bus Array of Bytes).
> >
> > This means a userspace agent can deal with devices that require their
> > BD_ADDR (or the host's) as a PIN, and take care of trying multiple of
> > such requirements.
>
> this has been discussed before and the answer is a clear no to this.
>

Why a no? I couldn't find the discussion in the archives.

> You can however write a plugin that can handle binary PIN codes.
>

The problem there is that the plugin can only try one, and can't retry
the authentication if that PIN fails.

Scott

Re: [PATCH 2/2] agent: allow agent to reply to RequestPinCode with bytes

[David Herrmann]

Hi Scott

On Sat, Jan 14, 2012 at 9:34 PM, Scott James Remnant
<keybuk@chromium.org> wrote:
> On Sat, Jan 14, 2012 at 8:41 AM, Marcel Holtmann <marcel@holtmann.org> wr=
ote:
>
>> > Since the specification allows the PIN to be a sequence of arbitrary
>> > octets, and not a UTF-8 string, allow the userspace agent to reply
>> > with such a sequence (as a D-Bus Array of Bytes).
>> >
>> > This means a userspace agent can deal with devices that require their
>> > BD_ADDR (or the host's) as a PIN, and take care of trying multiple of
>> > such requirements.
>>
>> this has been discussed before and the answer is a clear no to this.
>>
>
> Why a no? I couldn't find the discussion in the archives.

See here:
http://thread.gmane.org/gmane.linux.bluez.kernel/13380

>> You can however write a plugin that can handle binary PIN codes.
>>
>
> The problem there is that the plugin can only try one, and can't retry
> the authentication if that PIN fails.

Does your device provide proper PID/VID information? Then I see no
need to try several different PINs. Could you provide more information
with what kind of devices you are working here?

> Scott
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth=
" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at =A0http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] agent: allow agent to reply to RequestPinCode with bytes

[Scott James Remnant]

On Mon, Jan 16, 2012 at 7:44 AM, David Herrmann
<dh.herrmann@googlemail.com> wrote:

> On Sat, Jan 14, 2012 at 9:34 PM, Scott James Remnant
> <keybuk@chromium.org> wrote:
>> On Sat, Jan 14, 2012 at 8:41 AM, Marcel Holtmann <marcel@holtmann.org> wrote:
>>
>>> > Since the specification allows the PIN to be a sequence of arbitrary
>>> > octets, and not a UTF-8 string, allow the userspace agent to reply
>>> > with such a sequence (as a D-Bus Array of Bytes).
>>> >
>>> > This means a userspace agent can deal with devices that require their
>>> > BD_ADDR (or the host's) as a PIN, and take care of trying multiple of
>>> > such requirements.
>>>
>>> this has been discussed before and the answer is a clear no to this.
>>>
>>
>> Why a no? I couldn't find the discussion in the archives.
>
> See here:
> http://thread.gmane.org/gmane.linux.bluez.kernel/13380
>

This thread doesn't give a rationale for why support for Binary PINs
couldn't be added to BlueZ, just an alternate implementation in the
case of the WiiMote.

>>> You can however write a plugin that can handle binary PIN codes.
>>>
>>
>> The problem there is that the plugin can only try one, and can't retry
>> the authentication if that PIN fails.
>
> Does your device provide proper PID/VID information? Then I see no
> need to try several different PINs. Could you provide more information
> with what kind of devices you are working here?
>

No, but the name of the device can be strcmp()d to identify it

But that's not the problem - the problem is that the PIN varies on the
device depending on the exact mode being paired - and that's not
something we can tell from the host.

We simply need to try one of three PINs, two of which are annoyingly
binary (they're the Bluetooth address of the device and the host
respectively)

It's kinda difficult to do this in a plugin since each authentication
request only gets one shot - it's easier (and far cleaner) in the
agent, which can outlive each connection attempt and be used for both,
keeping track of the PINs it's tried.

(Likewise the agent is already trying "0000" on devices, and dealing
with not presenting a PIN for keyboards, etc.)

Scott

Re: [PATCH 2/2] agent: allow agent to reply to RequestPinCode with bytes

[Marcel Holtmann]

Hi Scott,

> >> On Sat, Jan 14, 2012 at 8:41 AM, Marcel Holtmann <marcel@holtmann.org> wrote:
> >>
> >>> > Since the specification allows the PIN to be a sequence of arbitrary
> >>> > octets, and not a UTF-8 string, allow the userspace agent to reply
> >>> > with such a sequence (as a D-Bus Array of Bytes).
> >>> >
> >>> > This means a userspace agent can deal with devices that require their
> >>> > BD_ADDR (or the host's) as a PIN, and take care of trying multiple of
> >>> > such requirements.
> >>>
> >>> this has been discussed before and the answer is a clear no to this.
> >>>
> >>
> >> Why a no? I couldn't find the discussion in the archives.
> >
> > See here:
> > http://thread.gmane.org/gmane.linux.bluez.kernel/13380
> >
> 
> This thread doesn't give a rationale for why support for Binary PINs
> couldn't be added to BlueZ, just an alternate implementation in the
> case of the WiiMote.

the BlueZ agent request is a direct question to the user. And the
question is meant to give something they can understand and something
they will also be entering on the other side.

It is not for trying to shoehorn Legacy Pairing into Simple Pairing.
Never was and never will be. Hence we are doing UTF-8 only strings.

> >>> You can however write a plugin that can handle binary PIN codes.
> >>>
> >>
> >> The problem there is that the plugin can only try one, and can't retry
> >> the authentication if that PIN fails.
> >
> > Does your device provide proper PID/VID information? Then I see no
> > need to try several different PINs. Could you provide more information
> > with what kind of devices you are working here?
> >
> 
> No, but the name of the device can be strcmp()d to identify it
> 
> But that's not the problem - the problem is that the PIN varies on the
> device depending on the exact mode being paired - and that's not
> something we can tell from the host.
> 
> We simply need to try one of three PINs, two of which are annoyingly
> binary (they're the Bluetooth address of the device and the host
> respectively)
> 
> It's kinda difficult to do this in a plugin since each authentication
> request only gets one shot - it's easier (and far cleaner) in the
> agent, which can outlive each connection attempt and be used for both,
> keeping track of the PINs it's tried.

I have to send you back to reading the Bluetooth specification for
Legacy Pairing when trying multiple pairing attempts. There is a
built-in security concept that will make this concept really
complicated.

Nothing you should handle inside the UI anyway. The agent is not meant
for this. Built a plugin that can handle this for you. We support that
for exactly these weird devices that wanna emulate Simple Pairing, but
are too lazy to do just use a Bluetooth 2.1 stack + device.

And now I have to ask the question, why are we bothering this heavy with
trying to do auto pairing with Legacy Pairing where almost everything
has moved on the Simple Pairing and the users have been trained to enter
0000 for their HID and headset devices if they ever get asked?

Regards

Marcel

Re: [PATCH 2/2] agent: allow agent to reply to RequestPinCode with bytes

[Scott James Remnant]

On Mon, Jan 16, 2012 at 11:47 PM, Marcel Holtmann <marcel@holtmann.org> wrote:

> > This thread doesn't give a rationale for why support for Binary PINs
> > couldn't be added to BlueZ, just an alternate implementation in the
> > case of the WiiMote.
>
> the BlueZ agent request is a direct question to the user. And the
> question is meant to give something they can understand and something
> they will also be entering on the other side.
>

Ok, so it'd be appropriate to write a plugin that handled, for
example, sending 0000 to devices within a certain class?

And for keyboard devices, would it be appropriate for the plugin to
generate the PIN itself and send the DisplayPasskey method to the
agent rather than the RequestPinCode? (This is a HID spec
recommendation)

> > It's kinda difficult to do this in a plugin since each authentication
> > request only gets one shot - it's easier (and far cleaner) in the
> > agent, which can outlive each connection attempt and be used for both,
> > keeping track of the PINs it's tried.
>
> I have to send you back to reading the Bluetooth specification for
> Legacy Pairing when trying multiple pairing attempts. There is a
> built-in security concept that will make this concept really
> complicated.
>

I've read it through recently, and I don't recall anything that would
prohibit retrying the link request again.

> And now I have to ask the question, why are we bothering this heavy with
> trying to do auto pairing with Legacy Pairing where almost everything
> has moved on the Simple Pairing and the users have been trained to enter
> 0000 for their HID and headset devices if they ever get asked?
>

Well, "almost everything" unfortunately seems to exclude almost every
device we purchased from Amazon and Fry's, which are all 2.0 at best.

And I don't think users really have been adequately trained - take
keyboards for example, does anyone really know that not only do you
have to try a pin (e.g. 0000) but then you have to type that same
exact pin on the keyboard? No, because a good OS hides all that.


Anyway, I take your point that the Agent is intended to be a direct
representation of the user, so will go down the plugin route and deal
with the lazy (and complicated) devices that way.

Would you like the lazy plugin contributed upstream?

Scott

Re: [PATCH 2/2] agent: allow agent to reply to RequestPinCode with bytes

[Marcel Holtmann]

Hi Scott,

> > > This thread doesn't give a rationale for why support for Binary PINs
> > > couldn't be added to BlueZ, just an alternate implementation in the
> > > case of the WiiMote.
> >
> > the BlueZ agent request is a direct question to the user. And the
> > question is meant to give something they can understand and something
> > they will also be entering on the other side.
> >
> 
> Ok, so it'd be appropriate to write a plugin that handled, for
> example, sending 0000 to devices within a certain class?

as long as you know that it is correct PIN code. If it is not, then you
might end up in the funky case that you have to try again. Meaning
actually pair again. If the remote device lets you do this again without
pressing the magic buttons again.

> And for keyboard devices, would it be appropriate for the plugin to
> generate the PIN itself and send the DisplayPasskey method to the
> agent rather than the RequestPinCode? (This is a HID spec
> recommendation)

Something like that could be done, but you do wanna solve that part
inside the bluetoothd core somehow. Play a little bit with the timing
details on this idea. There might be a problem. You can easily run into
a LMP timeout if you supply the PIN code too fast.

> > > It's kinda difficult to do this in a plugin since each authentication
> > > request only gets one shot - it's easier (and far cleaner) in the
> > > agent, which can outlive each connection attempt and be used for both,
> > > keeping track of the PINs it's tried.
> >
> > I have to send you back to reading the Bluetooth specification for
> > Legacy Pairing when trying multiple pairing attempts. There is a
> > built-in security concept that will make this concept really
> > complicated.
> >
> 
> I've read it through recently, and I don't recall anything that would
> prohibit retrying the link request again.

The is a backoff algorithm inside the core LMP on your device that will
effectively prevent these kind of "attacks".

> > And now I have to ask the question, why are we bothering this heavy with
> > trying to do auto pairing with Legacy Pairing where almost everything
> > has moved on the Simple Pairing and the users have been trained to enter
> > 0000 for their HID and headset devices if they ever get asked?
> >
> 
> Well, "almost everything" unfortunately seems to exclude almost every
> device we purchased from Amazon and Fry's, which are all 2.0 at best.

That is seriously funny. I have not bought a single 2.0 device in a long
time. Only exceptions are keyboards and mice. That device class seems to
be waiting for HID over Low Energy.

> And I don't think users really have been adequately trained - take
> keyboards for example, does anyone really know that not only do you
> have to try a pin (e.g. 0000) but then you have to type that same
> exact pin on the keyboard? No, because a good OS hides all that.
> 
> 
> Anyway, I take your point that the Agent is intended to be a direct
> representation of the user, so will go down the plugin route and deal
> with the lazy (and complicated) devices that way.
> 
> Would you like the lazy plugin contributed upstream?

Sure. Send them all upstream. You will need to touch the core anyway at
some point. Some of your ideas can not be solved with a pure plugin
right now. Especially if you wanna change the part that interacts with
the agent as well.

Regards

Marcel

Re: [PATCH 2/2] agent: allow agent to reply to RequestPinCode with bytes

[Scott James Remnant]

Thanks for the feedback!

On Tue, Jan 17, 2012 at 2:23 AM, Marcel Holtmann <marcel@holtmann.org> wrote:

>> Ok, so it'd be appropriate to write a plugin that handled, for
>> example, sending 0000 to devices within a certain class?
>
> as long as you know that it is correct PIN code. If it is not, then you
> might end up in the funky case that you have to try again. Meaning
> actually pair again. If the remote device lets you do this again without
> pressing the magic buttons again.
>

Yeah, that's a concern we'd have to test - on the other hand, OS X
does seem to just send 0000 for a whole class of devices without ever
prompting the user, so there's a chance this is "safe enough"

>> And for keyboard devices, would it be appropriate for the plugin to
>> generate the PIN itself and send the DisplayPasskey method to the
>> agent rather than the RequestPinCode? (This is a HID spec
>> recommendation)
>
> Something like that could be done, but you do wanna solve that part
> inside the bluetoothd core somehow. Play a little bit with the timing
> details on this idea. There might be a problem. You can easily run into
> a LMP timeout if you supply the PIN code too fast.
>

No problem

>> > I have to send you back to reading the Bluetooth specification for
>> > Legacy Pairing when trying multiple pairing attempts. There is a
>> > built-in security concept that will make this concept really
>> > complicated.
>> >
>>
>> I've read it through recently, and I don't recall anything that would
>> prohibit retrying the link request again.
>
> The is a backoff algorithm inside the core LMP on your device that will
> effectively prevent these kind of "attacks".
>

Fair point, I'll try rejecting OS X's 0000 PIN and see whether there
are interesting timings going on there that they've solved.

>> Well, "almost everything" unfortunately seems to exclude almost every
>> device we purchased from Amazon and Fry's, which are all 2.0 at best.
>
> That is seriously funny. I have not bought a single 2.0 device in a long
> time. Only exceptions are keyboards and mice. That device class seems to
> be waiting for HID over Low Energy.
>

That may be true, K+M are my focus for now, other than insane silly devices ...

> Sure. Send them all upstream. You will need to touch the core anyway at
> some point. Some of your ideas can not be solved with a pure plugin
> right now. Especially if you wanna change the part that interacts with
> the agent as well.
>
Sweet, thanks for your advice!

Scott