* [PATCH] BT_SECURITY_HIGH requires 16 digit pin code
@ 2010-08-27 11:45 Waldemar Rymarkiewicz
2010-08-27 12:12 ` Bastien Nocera
0 siblings, 1 reply; 11+ messages in thread
From: Waldemar Rymarkiewicz @ 2010-08-27 11:45 UTC (permalink / raw)
To: linux-bluetooth
Cc: par-gunnar.p.hjalmdahl, joakim.xj.ceder, johan.hedberg,
Waldemar Rymarkiewicz
The security level BT_SECURITY_HIGH expects secure connection
and a minimum 16 digit pin code used for bonding. It's requitred by the
Sim Access Profile.
Patch on behalf of ST-Ericsson SA.
Signed-off-by: Waldemar Rymarkiewicz <waldemar.rymarkiewicz@tieto.com>
---
lib/hci.h | 8 +++++++-
src/dbus-hci.c | 33 ++++++++++++++++++++++++++++-----
src/security.c | 38 ++++++++++++++++++++++++++++++++++----
3 files changed, 69 insertions(+), 10 deletions(-)
mode change 100644 => 100755 lib/bluetooth.h
mode change 100644 => 100755 lib/hci.h
mode change 100644 => 100755 lib/l2cap.h
mode change 100644 => 100755 lib/rfcomm.h
mode change 100644 => 100755 src/btio.c
mode change 100644 => 100755 src/btio.h
mode change 100644 => 100755 src/dbus-hci.c
mode change 100644 => 100755 src/security.c
diff --git a/lib/bluetooth.h b/lib/bluetooth.h
old mode 100644
new mode 100755
diff --git a/lib/hci.h b/lib/hci.h
old mode 100644
new mode 100755
index 512dab9..a313929
--- a/lib/hci.h
+++ b/lib/hci.h
@@ -96,7 +96,7 @@ enum {
#define HCISETLINKMODE _IOW('H', 226, int)
#define HCISETACLMTU _IOW('H', 227, int)
#define HCISETSCOMTU _IOW('H', 228, int)
-
+#define HCISETCONNINFO _IOW('H', 229, int)
#define HCIBLOCKADDR _IOW('H', 230, int)
#define HCIUNBLOCKADDR _IOW('H', 231, int)
@@ -2326,9 +2326,15 @@ struct hci_conn_info_req {
struct hci_conn_info conn_info[0];
};
+struct hci_set_conn_info_req {
+ bdaddr_t bdaddr;
+ uint8_t pin_len;
+ uint8_t key_type;
+};
struct hci_auth_info_req {
bdaddr_t bdaddr;
uint8_t type;
+ uint8_t level;
};
struct hci_inquiry_req {
diff --git a/lib/l2cap.h b/lib/l2cap.h
old mode 100644
new mode 100755
diff --git a/lib/rfcomm.h b/lib/rfcomm.h
old mode 100644
new mode 100755
diff --git a/src/btio.c b/src/btio.c
old mode 100644
new mode 100755
diff --git a/src/btio.h b/src/btio.h
old mode 100644
new mode 100755
diff --git a/src/dbus-hci.c b/src/dbus-hci.c
old mode 100644
new mode 100755
index 9055ffe..177f536
--- a/src/dbus-hci.c
+++ b/src/dbus-hci.c
@@ -165,6 +165,8 @@ static void pincode_cb(struct agent *agent, DBusError *err,
{
struct btd_adapter *adapter = device_get_adapter(device);
pin_code_reply_cp pr;
+ struct hci_auth_info_req ar;
+ struct hci_set_conn_info_req cr;
bdaddr_t sba, dba;
size_t len;
int dev;
@@ -180,13 +182,30 @@ static void pincode_cb(struct agent *agent, DBusError *err,
adapter_get_address(adapter, &sba);
device_get_address(device, &dba);
- if (err) {
- hci_send_cmd(dev, OGF_LINK_CTL,
- OCF_PIN_CODE_NEG_REPLY, 6, &dba);
- goto done;
- }
+ if (err)
+ goto reject;
len = strlen(pincode);
+ memset(&ar, 0, sizeof(ar));
+ bacpy(&ar.bdaddr, &dba);
+ if (ioctl(dev, HCIGETAUTHINFO, (unsigned long) &ar) < 0) {
+ error("Can't get auth info: %s (%d)", strerror(errno), errno);
+ goto reject;
+ }
+
+ if (ar.level == BT_SECURITY_HIGH && len < 16) {
+ error("Not enough secure PIN (expected 16 digit PIN, but got \
+ %d digit).", len);
+ goto reject;
+ }
+
+ bacpy(&cr.bdaddr, &dba);
+ cr.pin_len = len;
+ cr.key_type = 0xff;
+ if (ioctl(dev, HCISETCONNINFO, (unsigned long) &cr) < 0) {
+ error("Can't set conn info: %s (%d)", strerror(errno), errno);
+ goto reject;
+ }
set_pin_length(&sba, len);
@@ -196,7 +215,11 @@ static void pincode_cb(struct agent *agent, DBusError *err,
pr.pin_len = len;
hci_send_cmd(dev, OGF_LINK_CTL, OCF_PIN_CODE_REPLY,
PIN_CODE_REPLY_CP_SIZE, &pr);
+ goto done;
+reject:
+ hci_send_cmd(dev, OGF_LINK_CTL,
+ OCF_PIN_CODE_NEG_REPLY, 6, &dba);
done:
hci_close_dev(dev);
}
diff --git a/src/security.c b/src/security.c
old mode 100644
new mode 100755
index 667f1f1..b25d1e4
--- a/src/security.c
+++ b/src/security.c
@@ -309,6 +309,7 @@ static void link_key_request(int dev, bdaddr_t *sba, bdaddr_t *dba)
char sa[18], da[18];
uint8_t type;
int err;
+ int pinlen;
if (!get_adapter_and_device(sba, dba, &adapter, &device, FALSE))
device = NULL;
@@ -325,9 +326,11 @@ static void link_key_request(int dev, bdaddr_t *sba, bdaddr_t *dba)
DBG("HCIGETAUTHINFO failed %s (%d)",
strerror(errno), errno);
req.type = 0x00;
+ req.level = BT_SECURITY_LOW;
}
- DBG("kernel auth requirements = 0x%02x", req.type);
+ DBG("kernel auth requirements = 0x%02x and security level = 0x%02x", \
+ req.type, req.level);
if (main_opts.debug_keys && device && device_get_debug_key(device, key))
type = 0x03;
@@ -341,18 +344,34 @@ static void link_key_request(int dev, bdaddr_t *sba, bdaddr_t *dba)
DBG("link key type = 0x%02x", type);
+ pinlen = read_pin_length(sba, dba);
+ DBG("stored link key type = 0x%02x pin_len = %d", type, pinlen);
+
/* Don't use unauthenticated combination keys if MITM is
- * required */
- if (type == 0x04 && req.type != 0xff && (req.type & 0x01))
+ * required and also don't use combination link keys authenticated with
+ * an PIN code len < 16 if security level BT_SECURITY_HIGH is required*/
+ if ((type == 0x04 && req.type != 0xff && (req.type & 0x01)) ||
+ (type == 0x00 && req.level == BT_SECURITY_HIGH && pinlen < 16))
hci_send_cmd(dev, OGF_LINK_CTL, OCF_LINK_KEY_NEG_REPLY,
6, dba);
else {
link_key_reply_cp lr;
+ struct hci_set_conn_info_req cr;
memcpy(lr.link_key, key, 16);
bacpy(&lr.bdaddr, dba);
- hci_send_cmd(dev, OGF_LINK_CTL, OCF_LINK_KEY_REPLY,
+ bacpy(&cr.bdaddr, dba);
+ cr.pin_len = pinlen;
+ cr.key_type = type;
+
+ if (ioctl(dev, HCISETCONNINFO, (unsigned long) &cr) < 0) {
+ error("Can't set conn info: %s (%d)", strerror(errno),
+ errno);
+ hci_send_cmd(dev, OGF_LINK_CTL, OCF_LINK_KEY_NEG_REPLY,
+ 6, dba);
+ } else
+ hci_send_cmd(dev, OGF_LINK_CTL, OCF_LINK_KEY_REPLY,
LINK_KEY_REPLY_CP_SIZE, &lr);
}
}
@@ -523,6 +542,7 @@ static void pin_code_request(int dev, bdaddr_t *sba, bdaddr_t *dba)
pin_code_reply_cp pr;
struct hci_conn_info_req *cr;
struct hci_conn_info *ci;
+ struct hci_auth_info_req ar;
char sa[18], da[18], pin[17];
int pinlen;
@@ -542,10 +562,20 @@ static void pin_code_request(int dev, bdaddr_t *sba, bdaddr_t *dba)
}
ci = cr->conn_info;
+ memset(&ar, 0, sizeof(ar));
+ bacpy(&ar.bdaddr, dba);
+ if (ioctl(dev, HCIGETAUTHINFO, (unsigned long) &ar) < 0) {
+ error("Can't get auth info: %s (%d)", strerror(errno), errno);
+ goto reject;
+ }
memset(pin, 0, sizeof(pin));
pinlen = read_pin_code(sba, dba, pin);
if (pinlen > 0) {
+ if (ar.level == BT_SECURITY_HIGH && pinlen < 16) {
+ error("Not 16 digit pin code.");
+ goto reject;
+ }
set_pin_length(sba, pinlen);
memcpy(pr.pin_code, pin, pinlen);
pr.pin_len = pinlen;
--
1.7.0.4
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] BT_SECURITY_HIGH requires 16 digit pin code
2010-08-27 11:45 [PATCH] BT_SECURITY_HIGH requires 16 digit pin code Waldemar Rymarkiewicz
@ 2010-08-27 12:12 ` Bastien Nocera
2010-08-27 12:26 ` Waldemar.Rymarkiewicz
0 siblings, 1 reply; 11+ messages in thread
From: Bastien Nocera @ 2010-08-27 12:12 UTC (permalink / raw)
To: Waldemar Rymarkiewicz
Cc: linux-bluetooth, par-gunnar.p.hjalmdahl, joakim.xj.ceder,
johan.hedberg
On Fri, 2010-08-27 at 13:45 +0200, Waldemar Rymarkiewicz wrote:
> The security level BT_SECURITY_HIGH expects secure connection
> and a minimum 16 digit pin code used for bonding. It's requitred by the
> Sim Access Profile.
How is user-space (meaning the pairing agent) supposed to handle that?
I'd need to make changes to gnome-bluetooth to use longer PIN codes for
the maximum security.
Cheers
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH] BT_SECURITY_HIGH requires 16 digit pin code
2010-08-27 12:12 ` Bastien Nocera
@ 2010-08-27 12:26 ` Waldemar.Rymarkiewicz
2010-08-27 12:32 ` Bastien Nocera
2010-08-27 12:45 ` Johan Hedberg
0 siblings, 2 replies; 11+ messages in thread
From: Waldemar.Rymarkiewicz @ 2010-08-27 12:26 UTC (permalink / raw)
To: hadess
Cc: linux-bluetooth, par-gunnar.p.hjalmdahl, joakim.xj.ceder,
johan.hedberg, arunkr.singh
Hi,
>-----Original Message-----
>From: Bastien Nocera [mailto:hadess@hadess.net]
>Sent: Friday, August 27, 2010 2:12 PM
>To: Rymarkiewicz Waldemar
>Cc: linux-bluetooth@vger.kernel.org;
>par-gunnar.p.hjalmdahl@stericsson.com;
>joakim.xj.ceder@stericsson.com; johan.hedberg@gmail.com
>Subject: Re: [PATCH] BT_SECURITY_HIGH requires 16 digit pin code
>
>On Fri, 2010-08-27 at 13:45 +0200, Waldemar Rymarkiewicz wrote:
>> The security level BT_SECURITY_HIGH expects secure connection and a
>> minimum 16 digit pin code used for bonding. It's requitred
>by the Sim
>> Access Profile.
>
>How is user-space (meaning the pairing agent) supposed to handle that?
>I'd need to make changes to gnome-bluetooth to use longer PIN
>codes for the maximum security.
>
>Cheers
>
I assume that user will know that the 16 digit pin is requred, so should be enough to let the user type 16 digit in an agent I guess.
Usually a service that requires high security will generate right pin code.
Originaly the high security level was planned to require max pin code lenght as I know.
Thanks,
/Waldek
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH] BT_SECURITY_HIGH requires 16 digit pin code
2010-08-27 12:26 ` Waldemar.Rymarkiewicz
@ 2010-08-27 12:32 ` Bastien Nocera
2010-08-27 12:45 ` Johan Hedberg
1 sibling, 0 replies; 11+ messages in thread
From: Bastien Nocera @ 2010-08-27 12:32 UTC (permalink / raw)
To: Waldemar.Rymarkiewicz
Cc: linux-bluetooth, par-gunnar.p.hjalmdahl, joakim.xj.ceder,
johan.hedberg, arunkr.singh
On Fri, 2010-08-27 at 15:26 +0300, Waldemar.Rymarkiewicz@tieto.com
wrote:
> Hi,
>
> >-----Original Message-----
> >From: Bastien Nocera [mailto:hadess@hadess.net]
> >Sent: Friday, August 27, 2010 2:12 PM
> >To: Rymarkiewicz Waldemar
> >Cc: linux-bluetooth@vger.kernel.org;
> >par-gunnar.p.hjalmdahl@stericsson.com;
> >joakim.xj.ceder@stericsson.com; johan.hedberg@gmail.com
> >Subject: Re: [PATCH] BT_SECURITY_HIGH requires 16 digit pin code
> >
> >On Fri, 2010-08-27 at 13:45 +0200, Waldemar Rymarkiewicz wrote:
> >> The security level BT_SECURITY_HIGH expects secure connection and a
> >> minimum 16 digit pin code used for bonding. It's requitred
> >by the Sim
> >> Access Profile.
> >
> >How is user-space (meaning the pairing agent) supposed to handle that?
> >I'd need to make changes to gnome-bluetooth to use longer PIN
> >codes for the maximum security.
> >
> >Cheers
> >
>
> I assume that user will know that the 16 digit pin is requred, so
> should be enough to let the user type 16 digit in an agent I guess.
> Usually a service that requires high security will generate right pin
> code.
How would they know? Pairing the device isn't connecting to the
service... Furthermore, gnome-bluetooth's wizard takes a lot of care
generating pin codes for the user by default, so we'd need to know that
a 16 digit pin code is required.
Supporting 16 digits pin code would probably require interface changes.
And I was under the impression that the PIN code's length didn't come
into account for the creation of the encryption, just for the initial
challenge-response needed to verify the device is who it says it is.
> Originaly the high security level was planned to require max pin code
> lenght as I know.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] BT_SECURITY_HIGH requires 16 digit pin code
2010-08-27 12:26 ` Waldemar.Rymarkiewicz
2010-08-27 12:32 ` Bastien Nocera
@ 2010-08-27 12:45 ` Johan Hedberg
2010-08-27 13:32 ` Waldemar.Rymarkiewicz
2010-09-02 13:27 ` Waldemar.Rymarkiewicz
1 sibling, 2 replies; 11+ messages in thread
From: Johan Hedberg @ 2010-08-27 12:45 UTC (permalink / raw)
To: Waldemar.Rymarkiewicz
Cc: hadess, linux-bluetooth, par-gunnar.p.hjalmdahl, joakim.xj.ceder,
arunkr.singh
Hi Waldemar,
On Fri, Aug 27, 2010, Waldemar.Rymarkiewicz@tieto.com wrote:
> I assume that user will know that the 16 digit pin is requred, so
> should be enough to let the user type 16 digit in an agent I guess.
> Usually a service that requires high security will generate right pin
> code.
I don't think "the user should know" is enough here, so we may need to
think about ways that we could relay this info to the agent (maybe a
special adapter property or an extra parameter in the PIN request agent
callback).
> Originaly the high security level was planned to require max pin code
> lenght as I know.
That's correct, however the UI side hasn't really been considered so
far.
In general about the patches, I suspect it'll take a bit longer than
usual to get them in since the code you're touching is one of the most
sensitive ones with respect to breakage. We've finetuned it multiple
times over the last few years to get rid of IOP issues, security
vulnerabilities and to make sure all qualification tests pass. Another
reason for an expected delay is that you're introducing changes to the
kernel interface and that always requires some extra reviewing.
So how well have you tested the patches? I.e. how confident are you that
you're not introducing any regressions? Scenarios that would need to be
tested before an upstream merge are (and I'm probably forgetting several
of them):
- legacy pairing acceptor & initiator
- security mode 3 acceptor & initiator
- ssp acceptor & initiator
- renewed link key handling for both debug and normal keys
- security level upgrading (i.e. connect first to a low security socket
and then over the same ACL to a higher security socket)
- complete and partial failure scenarios for all of the above
Additionally all these test should be done against several different
controllers due to differences in HCI interface behavior (event
ordering, error codes, etc). In that list I'd include at least one CSR,
and one Broadcom adapter and any other adapters from other manufacturers
that you can get hold of.
So how many of these tests do you already have covered? I'm not very
comfortable with pushing the patches upstream before most of the above
scenarios have been tested and verified not to introduce any
regressions.
Johan
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH] BT_SECURITY_HIGH requires 16 digit pin code
2010-08-27 12:45 ` Johan Hedberg
@ 2010-08-27 13:32 ` Waldemar.Rymarkiewicz
2010-08-27 20:11 ` Johan Hedberg
2010-09-02 13:27 ` Waldemar.Rymarkiewicz
1 sibling, 1 reply; 11+ messages in thread
From: Waldemar.Rymarkiewicz @ 2010-08-27 13:32 UTC (permalink / raw)
To: johan.hedberg
Cc: hadess, linux-bluetooth, par-gunnar.p.hjalmdahl, joakim.xj.ceder,
arunkr.singh
Johan,
>-----Original Message-----
>From: Johan Hedberg [mailto:johan.hedberg@gmail.com]
>Sent: Friday, August 27, 2010 2:45 PM
>Hi Waldemar,
>
>On Fri, Aug 27, 2010, Waldemar.Rymarkiewicz@tieto.com wrote:
>> I assume that user will know that the 16 digit pin is requred, so
>> should be enough to let the user type 16 digit in an agent I guess.
>> Usually a service that requires high security will generate
>right pin
>> code.
>
>I don't think "the user should know" is enough here, so we may
>need to think about ways that we could relay this info to the
>agent (maybe a special adapter property or an extra parameter
>in the PIN request agent callback).
I agree and I guess adding a new property is more convenient then braking existing agen api.
>> Originaly the high security level was planned to require max
>pin code
>> lenght as I know.
>
>That's correct, however the UI side hasn't really been
>considered so far.
>
>In general about the patches, I suspect it'll take a bit
>longer than usual to get them in since the code you're
>touching is one of the most sensitive ones with respect to
>breakage. We've finetuned it multiple times over the last few
>years to get rid of IOP issues, security vulnerabilities and
>to make sure all qualification tests pass. Another reason for
>an expected delay is that you're introducing changes to the
>kernel interface and that always requires some extra reviewing.
I'm aware of that and I actually expected delay. Sending those patches I have been rather more interesting in starting some disscution on this topic, then get it up streamed now.
>So how well have you tested the patches? I.e. how confident
>are you that you're not introducing any regressions? Scenarios
>that would need to be tested before an upstream merge are (and
>I'm probably forgetting several of them):
>
>- legacy pairing acceptor & initiator
>- security mode 3 acceptor & initiator
>- ssp acceptor & initiator
>- renewed link key handling for both debug and normal keys
>- security level upgrading (i.e. connect first to a low security socket
> and then over the same ACL to a higher security socket)
>- complete and partial failure scenarios for all of the above
>
>Additionally all these test should be done against several
>different controllers due to differences in HCI interface
>behavior (event ordering, error codes, etc). In that list I'd
>include at least one CSR, and one Broadcom adapter and any
>other adapters from other manufacturers that you can get hold of.
>
>So how many of these tests do you already have covered? I'm
>not very comfortable with pushing the patches upstream before
>most of the above scenarios have been tested and verified not
>to introduce any regressions.
>
>Johan
>
To be honest, I did not check all mentioned use cases. I did tests against lagacy and ssp parring and security level upgrading.
So, I will put more effort into testing to be sure.
However, I would appreciate any comments to the changes as I'm not sure what was originally assumed for the high security level. What's more, I'am not happy that the link key request and the pin code request are handled in bluez. It makes that bluez need to pass it to the kernel as it's needed there. I used ioctl for that. Some comments on this?
Thanks,
/Waldek
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] BT_SECURITY_HIGH requires 16 digit pin code
2010-08-27 13:32 ` Waldemar.Rymarkiewicz
@ 2010-08-27 20:11 ` Johan Hedberg
0 siblings, 0 replies; 11+ messages in thread
From: Johan Hedberg @ 2010-08-27 20:11 UTC (permalink / raw)
To: Waldemar.Rymarkiewicz
Cc: hadess, linux-bluetooth, par-gunnar.p.hjalmdahl, joakim.xj.ceder,
arunkr.singh
Hi Waldek,
On Fri, Aug 27, 2010, Waldemar.Rymarkiewicz@tieto.com wrote:
> However, I would appreciate any comments to the changes as I'm not
> sure what was originally assumed for the high security level.
Your assumptions were more or less correct. Additionally there should be
the check for encryption key size (to fulfill SAP requirements), though
that'd require at least a 3.0 capable controller if for the standardized
HCI command.
> What's more, I'am not happy that the link key request and the pin code
> request are handled in bluez. It makes that bluez need to pass it to
> the kernel as it's needed there. I used ioctl for that. Some comments
> on this?
I'm not happy with it either. In fact, the arbitrary split of security
logic between kernel and userspace has caused us lots of grief,
especially with SSP. Because of this, the plan for BlueZ 5.x is to move
most security related logic to the kernel side. The only thing remaining
in userspace is security policy requiring user interaction (e.g.
answering user confirmation requests) and persistent link key storage
(runtime storage will be added to the kernel side). The promiscuous HCI
socket will go away from bluetoothd so the daemon doesn't always wake up
when there's some HCI traffic.
To make all this possible we're designing a new protocol between kernel
and userspace. Hopefully we'll get the first bits, at least a
preliminary specification, upstream within the next month or so. Because
of this I'm wondering if the new ioctl that you're adding is really
worth it. Maybe we should just wait for BlueZ 5.x with this. That'd also
give us the possibility of adding a new parameter to
Agent.RequestPinCode since we need to break the agent API anyway (e.g.
add an incoming just-works pairing callback).
Johan
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH] BT_SECURITY_HIGH requires 16 digit pin code
2010-08-27 12:45 ` Johan Hedberg
2010-08-27 13:32 ` Waldemar.Rymarkiewicz
@ 2010-09-02 13:27 ` Waldemar.Rymarkiewicz
2010-09-02 13:51 ` Johan Hedberg
1 sibling, 1 reply; 11+ messages in thread
From: Waldemar.Rymarkiewicz @ 2010-09-02 13:27 UTC (permalink / raw)
To: johan.hedberg
Cc: hadess, linux-bluetooth, par-gunnar.p.hjalmdahl, joakim.xj.ceder,
arunkr.singh
[-- Attachment #1: Type: text/plain, Size: 2150 bytes --]
Johan,
>From: Johan Hedberg [mailto:johan.hedberg@gmail.com]
>Sent: Friday, August 27, 2010 2:45 PM
>So how well have you tested the patches? I.e. how confident
>are you that you're not introducing any regressions? Scenarios
>that would need to be tested before an upstream merge are (and
>I'm probably forgetting several of them):
>
>- legacy pairing acceptor & initiator
>- security mode 3 acceptor & initiator
>- ssp acceptor & initiator
>- renewed link key handling for both debug and normal keys
>- security level upgrading (i.e. connect first to a low security socket
> and then over the same ACL to a higher security socket)
>- complete and partial failure scenarios for all of the above
I've completed more tests on the patches and didn't faced any problems do far.
Legacy paring, ssp, sec mode 3, refresh existing keys and security upgrading have finished with success. I did the tests for bluez as initiator and again when bluez was an acceptor. All tests were done against different controllers CSR (1.1, 2.0, 2.1), Broadcom (2.0, 2.1), ST-Ericsson (2.1). I also tried different combinations of the controllers in the same use case.
So, I'm pretty sure that it will not introduce any regression.
>Additionally all these test should be done against several
>different controllers due to differences in HCI interface
>behavior (event ordering, error codes, etc). In that list I'd
>include at least one CSR, and one Broadcom adapter and any
>other adapters from other manufacturers that you can get hold of.
>
>So how many of these tests do you already have covered? I'm
>not very comfortable with pushing the patches upstream before
>most of the above scenarios have been tested and verified not
>to introduce any regressions.
>
Aditionally, we plan to bring this to the UPF and it would be appreciated if also other would have that possibility for regression testing.
If it comes to interaction with the agent I would do this in a seperate patch which will contain a new property when 16 digit pin code is required.
I attached slightly updated patches.
Regards,
/Waldek
[-- Attachment #2: 0001-BT_SECURITY_HIGH-requires-16-digit-pin-code.patch --]
[-- Type: application/octet-stream, Size: 6043 bytes --]
From 7c1eeda7c1aa5e19c310e965be192f124df7c6fd Mon Sep 17 00:00:00 2001
From: Waldemar Rymarkiewicz <waldemar.rymarkiewicz@tieto.com>
Date: Mon, 21 Jun 2010 18:53:51 +0200
Subject: [PATCH] BT_SECURITY_HIGH requires 16 digit pin code
The security level BT_SECURITY_HIGH expects secure connection
and a minimum 16 digit pin code used for bonding. It's required by the
Sim Access Profile.
Patch on behalf of ST-Ericsson SA.
Signed-off-by: Waldemar Rymarkiewicz <waldemar.rymarkiewicz@tieto.com>
---
lib/hci.h | 8 +++++++-
src/dbus-hci.c | 32 +++++++++++++++++++++++++++-----
src/security.c | 39 +++++++++++++++++++++++++++++++++++----
3 files changed, 69 insertions(+), 10 deletions(-)
diff --git a/lib/hci.h b/lib/hci.h
index 512dab9..a313929 100644
--- a/lib/hci.h
+++ b/lib/hci.h
@@ -96,7 +96,7 @@ enum {
#define HCISETLINKMODE _IOW('H', 226, int)
#define HCISETACLMTU _IOW('H', 227, int)
#define HCISETSCOMTU _IOW('H', 228, int)
-
+#define HCISETCONNINFO _IOW('H', 229, int)
#define HCIBLOCKADDR _IOW('H', 230, int)
#define HCIUNBLOCKADDR _IOW('H', 231, int)
@@ -2326,9 +2326,15 @@ struct hci_conn_info_req {
struct hci_conn_info conn_info[0];
};
+struct hci_set_conn_info_req {
+ bdaddr_t bdaddr;
+ uint8_t pin_len;
+ uint8_t key_type;
+};
struct hci_auth_info_req {
bdaddr_t bdaddr;
uint8_t type;
+ uint8_t level;
};
struct hci_inquiry_req {
diff --git a/src/dbus-hci.c b/src/dbus-hci.c
index 9055ffe..fe38e45 100644
--- a/src/dbus-hci.c
+++ b/src/dbus-hci.c
@@ -165,6 +165,8 @@ static void pincode_cb(struct agent *agent, DBusError *err,
{
struct btd_adapter *adapter = device_get_adapter(device);
pin_code_reply_cp pr;
+ struct hci_auth_info_req ar;
+ struct hci_set_conn_info_req cr;
bdaddr_t sba, dba;
size_t len;
int dev;
@@ -180,13 +182,29 @@ static void pincode_cb(struct agent *agent, DBusError *err,
adapter_get_address(adapter, &sba);
device_get_address(device, &dba);
- if (err) {
- hci_send_cmd(dev, OGF_LINK_CTL,
- OCF_PIN_CODE_NEG_REPLY, 6, &dba);
- goto done;
- }
+ if (err)
+ goto reject;
len = strlen(pincode);
+ memset(&ar, 0, sizeof(ar));
+ bacpy(&ar.bdaddr, &dba);
+ if (ioctl(dev, HCIGETAUTHINFO, (unsigned long) &ar) < 0) {
+ error("Can't get auth info: %s (%d)", strerror(errno), errno);
+ goto reject;
+ }
+
+ if (ar.level == BT_SECURITY_HIGH && len < 16) {
+ error("PIN code is not a 16 digit (%d).", len);
+ goto reject;
+ }
+
+ bacpy(&cr.bdaddr, &dba);
+ cr.pin_len = len;
+ cr.key_type = 0xff;
+ if (ioctl(dev, HCISETCONNINFO, (unsigned long) &cr) < 0) {
+ error("Can't set conn info: %s (%d)", strerror(errno), errno);
+ goto reject;
+ }
set_pin_length(&sba, len);
@@ -196,7 +214,11 @@ static void pincode_cb(struct agent *agent, DBusError *err,
pr.pin_len = len;
hci_send_cmd(dev, OGF_LINK_CTL, OCF_PIN_CODE_REPLY,
PIN_CODE_REPLY_CP_SIZE, &pr);
+ goto done;
+reject:
+ hci_send_cmd(dev, OGF_LINK_CTL,
+ OCF_PIN_CODE_NEG_REPLY, 6, &dba);
done:
hci_close_dev(dev);
}
diff --git a/src/security.c b/src/security.c
index 667f1f1..a4eedc2 100644
--- a/src/security.c
+++ b/src/security.c
@@ -309,6 +309,7 @@ static void link_key_request(int dev, bdaddr_t *sba, bdaddr_t *dba)
char sa[18], da[18];
uint8_t type;
int err;
+ int pinlen;
if (!get_adapter_and_device(sba, dba, &adapter, &device, FALSE))
device = NULL;
@@ -325,9 +326,11 @@ static void link_key_request(int dev, bdaddr_t *sba, bdaddr_t *dba)
DBG("HCIGETAUTHINFO failed %s (%d)",
strerror(errno), errno);
req.type = 0x00;
+ req.level = BT_SECURITY_LOW;
}
- DBG("kernel auth requirements = 0x%02x", req.type);
+ DBG("kernel auth requirements = 0x%02x and security level = 0x%02x", \
+ req.type, req.level);
if (main_opts.debug_keys && device && device_get_debug_key(device, key))
type = 0x03;
@@ -341,18 +344,35 @@ static void link_key_request(int dev, bdaddr_t *sba, bdaddr_t *dba)
DBG("link key type = 0x%02x", type);
+ pinlen = read_pin_length(sba, dba);
+ DBG("stored link key type = 0x%02x pin_len = %d", type, pinlen);
+
/* Don't use unauthenticated combination keys if MITM is
- * required */
- if (type == 0x04 && req.type != 0xff && (req.type & 0x01))
+ * required and also don't use combination link keys authenticated
+ * with the PIN code len < 16 if security level BT_SECURITY_HIGH
+ * is required */
+ if ((type == 0x04 && req.type != 0xff && (req.type & 0x01)) ||
+ (type == 0x00 && req.level == BT_SECURITY_HIGH && pinlen < 16))
hci_send_cmd(dev, OGF_LINK_CTL, OCF_LINK_KEY_NEG_REPLY,
6, dba);
else {
link_key_reply_cp lr;
+ struct hci_set_conn_info_req cr;
memcpy(lr.link_key, key, 16);
bacpy(&lr.bdaddr, dba);
- hci_send_cmd(dev, OGF_LINK_CTL, OCF_LINK_KEY_REPLY,
+ bacpy(&cr.bdaddr, dba);
+ cr.pin_len = pinlen;
+ cr.key_type = type;
+
+ if (ioctl(dev, HCISETCONNINFO, (unsigned long) &cr) < 0) {
+ error("Can't set conn info: %s (%d)", strerror(errno),
+ errno);
+ hci_send_cmd(dev, OGF_LINK_CTL, OCF_LINK_KEY_NEG_REPLY,
+ 6, dba);
+ } else
+ hci_send_cmd(dev, OGF_LINK_CTL, OCF_LINK_KEY_REPLY,
LINK_KEY_REPLY_CP_SIZE, &lr);
}
}
@@ -523,6 +543,7 @@ static void pin_code_request(int dev, bdaddr_t *sba, bdaddr_t *dba)
pin_code_reply_cp pr;
struct hci_conn_info_req *cr;
struct hci_conn_info *ci;
+ struct hci_auth_info_req ar;
char sa[18], da[18], pin[17];
int pinlen;
@@ -542,10 +563,20 @@ static void pin_code_request(int dev, bdaddr_t *sba, bdaddr_t *dba)
}
ci = cr->conn_info;
+ memset(&ar, 0, sizeof(ar));
+ bacpy(&ar.bdaddr, dba);
+ if (ioctl(dev, HCIGETAUTHINFO, (unsigned long) &ar) < 0) {
+ error("Can't get auth info: %s (%d)", strerror(errno), errno);
+ goto reject;
+ }
memset(pin, 0, sizeof(pin));
pinlen = read_pin_code(sba, dba, pin);
if (pinlen > 0) {
+ if (ar.level == BT_SECURITY_HIGH && pinlen < 16) {
+ error("Not 16 digit pin code.");
+ goto reject;
+ }
set_pin_length(sba, pinlen);
memcpy(pr.pin_code, pin, pinlen);
pr.pin_len = pinlen;
--
1.7.0.4
[-- Attachment #3: 0001-Bluetooth-BT_SECURITY_HIGH-requires-16-digit-pin-cod.patch --]
[-- Type: application/octet-stream, Size: 9554 bytes --]
From acc1dd35f7fd6f9d24c328aef244365ef6716382 Mon Sep 17 00:00:00 2001
From: Waldemar Rymarkiewicz <waldemar.rymarkiewicz@tieto.com>
Date: Wed, 25 Aug 2010 17:48:32 +0200
Subject: [PATCH] Bluetooth: BT_SECURITY_HIGH requires 16 digit pin code
The security level BT_SECURITY_HIGH expects secure connection
and a minimum 16 digit pin code used for bonding. It's required by the
Sim Access Profile.
Patch on behalf of ST-Ericsson SA.
Signed-off-by: Waldemar Rymarkiewicz <waldemar.rymarkiewicz@tieto.com>
---
include/net/bluetooth/hci.h | 17 +++++++
include/net/bluetooth/hci_core.h | 4 ++
net/bluetooth/hci_conn.c | 97 +++++++++++++++++++++++++++++++-------
net/bluetooth/hci_event.c | 4 ++
net/bluetooth/hci_sock.c | 3 +
net/bluetooth/rfcomm/core.c | 10 ++++-
6 files changed, 116 insertions(+), 19 deletions(-)
diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index bcbdd6d..32a33c0 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -99,6 +99,7 @@ enum {
#define HCISETLINKMODE _IOW('H', 226, int)
#define HCISETACLMTU _IOW('H', 227, int)
#define HCISETSCOMTU _IOW('H', 228, int)
+#define HCISETCONNINFO _IOW('H', 229, int)
#define HCIBLOCKADDR _IOW('H', 230, int)
#define HCIUNBLOCKADDR _IOW('H', 231, int)
@@ -224,6 +225,15 @@ enum {
#define HCI_AT_GENERAL_BONDING 0x04
#define HCI_AT_GENERAL_BONDING_MITM 0x05
+/* Link Key types */
+#define HCI_LK_COMBINATION 0x00
+#define HCI_LK_LOCAL_UNIT 0x01
+#define HCI_LK_REMOTE_UNIT 0x02
+#define HCI_LK_DEBUG_COMBINATION 0x03
+#define HCI_LK_UNAUTHENTICATED_COMBINATION 0x04
+#define HCI_LK_AUTHENTICATED_COMBINATION 0x05
+#define HCI_LK_CHANGEED_COMBINATION_KEY 0x06
+
/* ----- HCI Commands ---- */
#define HCI_OP_INQUIRY 0x0401
struct hci_cp_inquiry {
@@ -1022,9 +1032,16 @@ struct hci_conn_info_req {
struct hci_conn_info conn_info[0];
};
+struct hci_set_conn_info_req {
+ bdaddr_t bdaddr;
+ __u8 pin_len;
+ __u8 key_type;
+};
+
struct hci_auth_info_req {
bdaddr_t bdaddr;
__u8 type;
+ __u8 level;
};
struct hci_inquiry_req {
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 4568b93..9eb2da3 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -183,6 +183,8 @@ struct hci_conn {
__u32 link_mode;
__u8 auth_type;
__u8 sec_level;
+ __u8 key_type;
+ __u8 pin_len;
__u8 power_save;
__u16 disc_timeout;
unsigned long pend;
@@ -430,6 +432,8 @@ int hci_get_dev_info(void __user *arg);
int hci_get_conn_list(void __user *arg);
int hci_get_conn_info(struct hci_dev *hdev, void __user *arg);
int hci_get_auth_info(struct hci_dev *hdev, void __user *arg);
+int hci_set_conn_info(struct hci_dev *hdev, void __user *arg);
+
int hci_inquiry(void __user *arg);
struct bdaddr_list *hci_blacklist_lookup(struct hci_dev *hdev, bdaddr_t *bdaddr);
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 0b1e460..54709fa 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -233,6 +233,8 @@ struct hci_conn *hci_conn_add(struct hci_dev *hdev, int type, bdaddr_t *dst)
conn->mode = HCI_CM_ACTIVE;
conn->state = BT_OPEN;
conn->auth_type = HCI_AT_GENERAL_BONDING;
+ conn->key_type = 0xff;
+ conn->pin_len = 0;
conn->power_save = 1;
conn->disc_timeout = HCI_DISCONN_TIMEOUT;
@@ -433,15 +435,11 @@ int hci_conn_check_link_mode(struct hci_conn *conn)
EXPORT_SYMBOL(hci_conn_check_link_mode);
/* Authenticate remote device */
-static int hci_conn_auth(struct hci_conn *conn, __u8 sec_level, __u8 auth_type)
+static void hci_conn_auth(struct hci_conn *conn, __u8 sec_level, __u8 auth_type)
{
BT_DBG("conn %p", conn);
- if (sec_level > conn->sec_level)
- conn->sec_level = sec_level;
- else if (conn->link_mode & HCI_LM_AUTH)
- return 1;
-
+ conn->sec_level = sec_level;
conn->auth_type = auth_type;
if (!test_and_set_bit(HCI_CONN_AUTH_PEND, &conn->pend)) {
@@ -450,8 +448,20 @@ static int hci_conn_auth(struct hci_conn *conn, __u8 sec_level, __u8 auth_type)
hci_send_cmd(conn->hdev, HCI_OP_AUTH_REQUESTED,
sizeof(cp), &cp);
}
+}
- return 0;
+/* Encrypt the the link */
+static void hci_conn_encrypt(struct hci_conn *conn)
+{
+ BT_DBG("conn %p", conn);
+
+ if (!test_and_set_bit(HCI_CONN_ENCRYPT_PEND, &conn->pend)) {
+ struct hci_cp_set_conn_encrypt cp;
+ cp.handle = cpu_to_le16(conn->handle);
+ cp.encrypt = 1;
+ hci_send_cmd(conn->hdev, HCI_OP_SET_CONN_ENCRYPT,
+ sizeof(cp), &cp);
+ }
}
/* Enable security */
@@ -459,28 +469,54 @@ int hci_conn_security(struct hci_conn *conn, __u8 sec_level, __u8 auth_type)
{
BT_DBG("conn %p", conn);
+ /* For sdp we do not need the link key. */
if (sec_level == BT_SECURITY_SDP)
return 1;
+ /* For non 2.1 devices and low security level we do not need the
+ link key. */
if (sec_level == BT_SECURITY_LOW &&
(!conn->ssp_mode || !conn->hdev->ssp_mode))
return 1;
- if (conn->link_mode & HCI_LM_ENCRYPT)
- return hci_conn_auth(conn, sec_level, auth_type);
-
+ /* For other security levels we need link key. */
+ if (!(conn->link_mode & HCI_LM_AUTH))
+ goto do_auth;
+
+ /* An authenticated combination key has sufficient security for any
+ security level. */
+ if (conn->key_type == HCI_LK_AUTHENTICATED_COMBINATION)
+ goto do_encrypt;
+
+ /* An unauthenticated combination key has sufficient security for
+ security level 1 and 2. */
+ if (conn->key_type == HCI_LK_UNAUTHENTICATED_COMBINATION
+ && (sec_level == BT_SECURITY_MEDIUM
+ || sec_level == BT_SECURITY_LOW))
+ goto do_encrypt;
+
+ /* A combination key has always sufficient security for the security
+ levels 1 or 2. High security level requires that the combination key
+ was generated using the maximum PIN code length (16).
+ For pre 2.1 units. */
+ if ((conn->key_type == HCI_LK_COMBINATION))
+ if ((sec_level != BT_SECURITY_HIGH) || (conn->pin_len >= 16))
+ goto do_encrypt;
+
+do_auth:
if (test_and_set_bit(HCI_CONN_ENCRYPT_PEND, &conn->pend))
return 0;
- if (hci_conn_auth(conn, sec_level, auth_type)) {
- struct hci_cp_set_conn_encrypt cp;
- cp.handle = cpu_to_le16(conn->handle);
- cp.encrypt = 1;
- hci_send_cmd(conn->hdev, HCI_OP_SET_CONN_ENCRYPT,
- sizeof(cp), &cp);
- }
-
+ hci_conn_auth(conn, sec_level, auth_type);
return 0;
+
+do_encrypt:
+ if (conn->link_mode & HCI_LM_ENCRYPT)
+ return 1; /* sufficient link key */
+ else{
+ hci_conn_encrypt(conn);
+ return 0; /* auth pending */
+ }
}
EXPORT_SYMBOL(hci_conn_security);
@@ -713,6 +749,30 @@ int hci_get_conn_info(struct hci_dev *hdev, void __user *arg)
return copy_to_user(ptr, &ci, sizeof(ci)) ? -EFAULT : 0;
}
+int hci_set_conn_info(struct hci_dev *hdev, void __user *arg)
+{
+ struct hci_set_conn_info_req req;
+ struct hci_conn *conn;
+
+ if (copy_from_user(&req, arg, sizeof(req))) {
+ BT_DBG("copy from user failed");
+ return -EFAULT;
+ }
+
+ hci_dev_lock_bh(hdev);
+ conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, &req.bdaddr);
+ if (conn) {
+ conn->pin_len = req.pin_len;
+ conn->key_type = req.key_type;
+ }
+ hci_dev_unlock_bh(hdev);
+
+ if (!conn)
+ return -ENOENT;
+
+ return 0;
+}
+
int hci_get_auth_info(struct hci_dev *hdev, void __user *arg)
{
struct hci_auth_info_req req;
@@ -725,6 +785,7 @@ int hci_get_auth_info(struct hci_dev *hdev, void __user *arg)
conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, &req.bdaddr);
if (conn)
req.type = conn->auth_type;
+ req.level = conn->sec_level;
hci_dev_unlock_bh(hdev);
if (!conn)
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index bfef5ba..6d6b04c 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1521,6 +1521,10 @@ static inline void hci_link_key_notify_evt(struct hci_dev *hdev, struct sk_buff
conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, &ev->bdaddr);
if (conn) {
hci_conn_hold(conn);
+ /* For Changed Combination Link Key the only link key has
+ * been changed, not link key type. */
+ if (conn->key_type != HCI_LK_CHANGEED_COMBINATION_KEY)
+ conn->key_type = ev->key_type;
conn->disc_timeout = HCI_DISCONN_TIMEOUT;
hci_conn_put(conn);
}
diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
index 83acd16..502a7b0 100644
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c
@@ -269,6 +269,9 @@ static inline int hci_sock_bound_ioctl(struct sock *sk, unsigned int cmd, unsign
case HCIGETCONNINFO:
return hci_get_conn_info(hdev, (void __user *) arg);
+ case HCISETCONNINFO:
+ return hci_set_conn_info(hdev, (void __user *) arg);
+
case HCIGETAUTHINFO:
return hci_get_auth_info(hdev, (void __user *) arg);
diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c
index 7dca91b..2e248d5 100644
--- a/net/bluetooth/rfcomm/core.c
+++ b/net/bluetooth/rfcomm/core.c
@@ -2086,7 +2086,15 @@ static void rfcomm_security_cfm(struct hci_conn *conn, u8 status, u8 encrypt)
continue;
if (!status)
- set_bit(RFCOMM_AUTH_ACCEPT, &d->flags);
+ if (d->sec_level != BT_SECURITY_HIGH)
+ set_bit(RFCOMM_AUTH_ACCEPT, &d->flags);
+ else
+ if ((conn->key_type == HCI_LK_AUTHENTICATED_COMBINATION)
+ || (conn->key_type == HCI_LK_COMBINATION
+ && conn->pin_len >= 16))
+ set_bit(RFCOMM_AUTH_ACCEPT, &d->flags);
+ else
+ set_bit(RFCOMM_AUTH_REJECT, &d->flags);
else
set_bit(RFCOMM_AUTH_REJECT, &d->flags);
}
--
1.7.0.4
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] BT_SECURITY_HIGH requires 16 digit pin code
2010-09-02 13:27 ` Waldemar.Rymarkiewicz
@ 2010-09-02 13:51 ` Johan Hedberg
2010-09-02 15:30 ` Waldemar.Rymarkiewicz
2010-09-07 8:57 ` Waldemar.Rymarkiewicz
0 siblings, 2 replies; 11+ messages in thread
From: Johan Hedberg @ 2010-09-02 13:51 UTC (permalink / raw)
To: Waldemar.Rymarkiewicz
Cc: hadess, linux-bluetooth, par-gunnar.p.hjalmdahl, joakim.xj.ceder,
arunkr.singh
Hi Waldek,
On Thu, Sep 02, 2010, Waldemar.Rymarkiewicz@tieto.com wrote:
> I've completed more tests on the patches and didn't faced any problems
> do far. Legacy paring, ssp, sec mode 3, refresh existing keys and
> security upgrading have finished with success. I did the tests for
> bluez as initiator and again when bluez was an acceptor. All tests
> were done against different controllers CSR (1.1, 2.0, 2.1), Broadcom
> (2.0, 2.1), ST-Ericsson (2.1). I also tried different combinations of
> the controllers in the same use case.
>
> So, I'm pretty sure that it will not introduce any regression.
Ok, that's good to hear.
> Aditionally, we plan to bring this to the UPF and it would be
> appreciated if also other would have that possibility for regression
> testing.
I'll be at the UPF too, so this might be possible.
> If it comes to interaction with the agent I would do this in a
> seperate patch which will contain a new property when 16 digit pin
> code is required.
That's fine.
> I attached slightly updated patches.
Thanks. However, the kernel patch and new ioctl will need comments at
least from Marcel. Once we add an ioctl we're stuck with it for quite
some time and have to maintain it, no matter what kind of newer/better
kernel-userspace interfaces we come up with. So the choice of accepting
a new ioctl isn't so easy.
One thing that you'd definitely need to fix in your patches is to keep
at least the same level of support that the current BlueZ has with
kernels that don't have the new ioctl. Right now your patch would make
legacy pairing fail in such cases which is not acceptable. Only with a
major version change (5.x) would it be possible to consider requiring a
newer kernel version in order to have essential functionality in place.
With all this in mind I'd still prefer it if we postpone the feature
addition until the point where we have a more flexible kernel-userspace
API in place and most of the security logic and information on the
kernel side.
Johan
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH] BT_SECURITY_HIGH requires 16 digit pin code
2010-09-02 13:51 ` Johan Hedberg
@ 2010-09-02 15:30 ` Waldemar.Rymarkiewicz
2010-09-07 8:57 ` Waldemar.Rymarkiewicz
1 sibling, 0 replies; 11+ messages in thread
From: Waldemar.Rymarkiewicz @ 2010-09-02 15:30 UTC (permalink / raw)
To: johan.hedberg
Cc: hadess, linux-bluetooth, par-gunnar.p.hjalmdahl, joakim.xj.ceder,
arunkr.singh
Hi Johan,
>From: Johan Hedberg [mailto:johan.hedberg@gmail.com]
>Sent: Thursday, September 02, 2010 3:51 PM
>
>
>Thanks. However, the kernel patch and new ioctl will need
>comments at least from Marcel. Once we add an ioctl we're
>stuck with it for quite some time and have to maintain it, no
>matter what kind of newer/better kernel-userspace interfaces
>we come up with. So the choice of accepting a new ioctl isn't so easy.
Ok. Let's wait for Marcel's comment.
>One thing that you'd definitely need to fix in your patches is
>to keep at least the same level of support that the current
>BlueZ has with kernels that don't have the new ioctl. Right
>now your patch would make legacy pairing fail in such cases
>which is not acceptable. Only with a major version change
>(5.x) would it be possible to consider requiring a newer
>kernel version in order to have essential functionality in place.
Right. Will fix this.
/Waldek
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH] BT_SECURITY_HIGH requires 16 digit pin code
2010-09-02 13:51 ` Johan Hedberg
2010-09-02 15:30 ` Waldemar.Rymarkiewicz
@ 2010-09-07 8:57 ` Waldemar.Rymarkiewicz
1 sibling, 0 replies; 11+ messages in thread
From: Waldemar.Rymarkiewicz @ 2010-09-07 8:57 UTC (permalink / raw)
To: johan.hedberg, marcel
Cc: hadess, linux-bluetooth, par-gunnar.p.hjalmdahl, joakim.xj.ceder,
arunkr.singh
[-- Attachment #1: Type: text/plain, Size: 1104 bytes --]
Hi Johan,
>-----Original Message-----
>From: Johan Hedberg [mailto:johan.hedberg@gmail.com]
>Sent: Thursday, September 02, 2010 3:51 PM
>> I attached slightly updated patches.
>Thanks. However, the kernel patch and new ioctl will need
>comments at least from Marcel. Once we add an ioctl we're
>stuck with it for quite some time and have to maintain it, no
>matter what kind of newer/better kernel-userspace interfaces
>we come up with. So the choice of accepting a new ioctl isn't so easy.
Then, wait for Marcel's comment.
>One thing that you'd definitely need to fix in your patches is
>to keep at least the same level of support that the current
>BlueZ has with kernels that don't have the new ioctl. Right
>now your patch would make legacy pairing fail in such cases
>which is not acceptable. Only with a major version change
>(5.x) would it be possible to consider requiring a newer
>kernel version in order to have essential functionality in place.
I updated the bluez patch. Now paring will not fail due to not compatible kernel api.
Regards,
/Waldek
[-- Attachment #2: 0001-BT_SECURITY_HIGH-requires-16-digit-pin-code.patch --]
[-- Type: application/octet-stream, Size: 6664 bytes --]
From 2433903023f6e99cc5b1d800f71a7431048a23ee Mon Sep 17 00:00:00 2001
From: Waldemar Rymarkiewicz <waldemar.rymarkiewicz@tieto.com>
Date: Mon, 21 Jun 2010 18:53:51 +0200
Subject: [PATCH] BT_SECURITY_HIGH requires 16 digit pin code
The security level BT_SECURITY_HIGH expects secure connection
and a minimum 16 digit pin code used for bonding. It's required by the
Sim Access Profile.
Patch on behalf of ST-Ericsson SA.
---
lib/hci.h | 8 +++++++-
src/dbus-hci.c | 38 +++++++++++++++++++++++++++++++++-----
src/security.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++--------
3 files changed, 86 insertions(+), 14 deletions(-)
diff --git a/lib/hci.h b/lib/hci.h
index 512dab9..a313929 100644
--- a/lib/hci.h
+++ b/lib/hci.h
@@ -96,7 +96,7 @@ enum {
#define HCISETLINKMODE _IOW('H', 226, int)
#define HCISETACLMTU _IOW('H', 227, int)
#define HCISETSCOMTU _IOW('H', 228, int)
-
+#define HCISETCONNINFO _IOW('H', 229, int)
#define HCIBLOCKADDR _IOW('H', 230, int)
#define HCIUNBLOCKADDR _IOW('H', 231, int)
@@ -2326,9 +2326,15 @@ struct hci_conn_info_req {
struct hci_conn_info conn_info[0];
};
+struct hci_set_conn_info_req {
+ bdaddr_t bdaddr;
+ uint8_t pin_len;
+ uint8_t key_type;
+};
struct hci_auth_info_req {
bdaddr_t bdaddr;
uint8_t type;
+ uint8_t level;
};
struct hci_inquiry_req {
diff --git a/src/dbus-hci.c b/src/dbus-hci.c
index 9055ffe..fe90337 100644
--- a/src/dbus-hci.c
+++ b/src/dbus-hci.c
@@ -165,9 +165,12 @@ static void pincode_cb(struct agent *agent, DBusError *err,
{
struct btd_adapter *adapter = device_get_adapter(device);
pin_code_reply_cp pr;
+ struct hci_auth_info_req ar;
+ struct hci_set_conn_info_req cr;
bdaddr_t sba, dba;
size_t len;
int dev;
+ int ret;
uint16_t dev_id = adapter_get_dev_id(adapter);
dev = hci_open_dev(dev_id);
@@ -180,13 +183,34 @@ static void pincode_cb(struct agent *agent, DBusError *err,
adapter_get_address(adapter, &sba);
device_get_address(device, &dba);
- if (err) {
- hci_send_cmd(dev, OGF_LINK_CTL,
- OCF_PIN_CODE_NEG_REPLY, 6, &dba);
- goto done;
- }
+ if (err)
+ goto reject;
len = strlen(pincode);
+ memset(&ar, 0, sizeof(ar));
+ bacpy(&ar.bdaddr, &dba);
+ ar.level = BT_SECURITY_LOW;
+
+ ret = ioctl(dev, HCIGETAUTHINFO, (unsigned long) &ar);
+ if (ret < 0 && errno != EINVAL) {
+ error("Can't get auth info: %s (%d)", strerror(errno), errno);
+ goto reject;
+ }
+
+ if (ar.level == BT_SECURITY_HIGH && len < 16) {
+ error("PIN code is not a 16 digit (%d)", len);
+ goto reject;
+ }
+
+ bacpy(&cr.bdaddr, &dba);
+ cr.pin_len = len;
+ cr.key_type = 0xff;
+
+ ret = ioctl(dev, HCISETCONNINFO, (unsigned long) &cr);
+ if (ret < 0 && errno != EINVAL) {
+ error("Can't set conn info: %s (%d)", strerror(errno), errno);
+ goto reject;
+ }
set_pin_length(&sba, len);
@@ -196,7 +220,11 @@ static void pincode_cb(struct agent *agent, DBusError *err,
pr.pin_len = len;
hci_send_cmd(dev, OGF_LINK_CTL, OCF_PIN_CODE_REPLY,
PIN_CODE_REPLY_CP_SIZE, &pr);
+ goto done;
+reject:
+ hci_send_cmd(dev, OGF_LINK_CTL,
+ OCF_PIN_CODE_NEG_REPLY, 6, &dba);
done:
hci_close_dev(dev);
}
diff --git a/src/security.c b/src/security.c
index 667f1f1..5ede6f6 100644
--- a/src/security.c
+++ b/src/security.c
@@ -309,6 +309,7 @@ static void link_key_request(int dev, bdaddr_t *sba, bdaddr_t *dba)
char sa[18], da[18];
uint8_t type;
int err;
+ int pinlen;
if (!get_adapter_and_device(sba, dba, &adapter, &device, FALSE))
device = NULL;
@@ -318,16 +319,18 @@ static void link_key_request(int dev, bdaddr_t *sba, bdaddr_t *dba)
memset(&req, 0, sizeof(req));
bacpy(&req.bdaddr, dba);
+ req.type = 0x00;
+ /* Set low security in case when kernel does not support 16 digit
+ pin code in the high security level */
+ req.level = BT_SECURITY_LOW;
err = ioctl(dev, HCIGETAUTHINFO, (unsigned long) &req);
- if (err < 0) {
- if (errno != EINVAL)
- DBG("HCIGETAUTHINFO failed %s (%d)",
+ if (err < 0 && errno != EINVAL)
+ DBG("HCIGETAUTHINFO failed %s (%d)",
strerror(errno), errno);
- req.type = 0x00;
- }
- DBG("kernel auth requirements = 0x%02x", req.type);
+ DBG("kernel auth requirements = 0x%02x and security level = 0x%02x", \
+ req.type, req.level);
if (main_opts.debug_keys && device && device_get_debug_key(device, key))
type = 0x03;
@@ -341,17 +344,37 @@ static void link_key_request(int dev, bdaddr_t *sba, bdaddr_t *dba)
DBG("link key type = 0x%02x", type);
+ pinlen = read_pin_length(sba, dba);
+ DBG("stored link key type = 0x%02x pin_len = %d", type, pinlen);
+
/* Don't use unauthenticated combination keys if MITM is
- * required */
- if (type == 0x04 && req.type != 0xff && (req.type & 0x01))
+ * required and also don't use combination link keys authenticated
+ * with the PIN code len < 16 if security level BT_SECURITY_HIGH
+ * is required */
+ if ((type == 0x04 && req.type != 0xff && (req.type & 0x01)) ||
+ (type == 0x00 && req.level == BT_SECURITY_HIGH && pinlen < 16))
hci_send_cmd(dev, OGF_LINK_CTL, OCF_LINK_KEY_NEG_REPLY,
6, dba);
else {
link_key_reply_cp lr;
+ struct hci_set_conn_info_req cr;
memcpy(lr.link_key, key, 16);
bacpy(&lr.bdaddr, dba);
+ bacpy(&cr.bdaddr, dba);
+ cr.pin_len = pinlen;
+ cr.key_type = type;
+
+ err = ioctl(dev, HCISETCONNINFO, (unsigned long) &cr);
+ if (err < 0 && errno != EINVAL) {
+ error("Can't set conn info: %s (%d)", strerror(errno),
+ errno);
+ hci_send_cmd(dev, OGF_LINK_CTL, OCF_LINK_KEY_NEG_REPLY,
+ 6, dba);
+ return;
+ }
+
hci_send_cmd(dev, OGF_LINK_CTL, OCF_LINK_KEY_REPLY,
LINK_KEY_REPLY_CP_SIZE, &lr);
}
@@ -523,8 +546,10 @@ static void pin_code_request(int dev, bdaddr_t *sba, bdaddr_t *dba)
pin_code_reply_cp pr;
struct hci_conn_info_req *cr;
struct hci_conn_info *ci;
+ struct hci_auth_info_req ar;
char sa[18], da[18], pin[17];
int pinlen;
+ int err;
memset(&pr, 0, sizeof(pr));
bacpy(&pr.bdaddr, dba);
@@ -542,10 +567,23 @@ static void pin_code_request(int dev, bdaddr_t *sba, bdaddr_t *dba)
}
ci = cr->conn_info;
+ memset(&ar, 0, sizeof(ar));
+ bacpy(&ar.bdaddr, dba);
+ ar.level = BT_SECURITY_LOW;
+
+ err = ioctl(dev, HCIGETAUTHINFO, (unsigned long) &ar);
+ if (err < 0 && errno != EINVAL) {
+ error("Can't get auth info: %s (%d)", strerror(errno), errno);
+ goto reject;
+ }
memset(pin, 0, sizeof(pin));
pinlen = read_pin_code(sba, dba, pin);
if (pinlen > 0) {
+ if (ar.level == BT_SECURITY_HIGH && pinlen < 16) {
+ error("Not 16 digit pin code.");
+ goto reject;
+ }
set_pin_length(sba, pinlen);
memcpy(pr.pin_code, pin, pinlen);
pr.pin_len = pinlen;
--
1.7.0.4
^ permalink raw reply related [flat|nested] 11+ messages in thread
end of thread, other threads:[~2010-09-07 8:57 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-08-27 11:45 [PATCH] BT_SECURITY_HIGH requires 16 digit pin code Waldemar Rymarkiewicz
2010-08-27 12:12 ` Bastien Nocera
2010-08-27 12:26 ` Waldemar.Rymarkiewicz
2010-08-27 12:32 ` Bastien Nocera
2010-08-27 12:45 ` Johan Hedberg
2010-08-27 13:32 ` Waldemar.Rymarkiewicz
2010-08-27 20:11 ` Johan Hedberg
2010-09-02 13:27 ` Waldemar.Rymarkiewicz
2010-09-02 13:51 ` Johan Hedberg
2010-09-02 15:30 ` Waldemar.Rymarkiewicz
2010-09-07 8:57 ` Waldemar.Rymarkiewicz
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox