[PATCH] Adding a new option to specify security level for gatttool

[PATCH] Adding a new option to specify security level for gatttool

[Sheldon Demario]

---
 TODO              |    6 ------
 attrib/gatttool.c |   15 +++++++++++++--
 2 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/TODO b/TODO
index c0a25f1..49a9e76 100644
--- a/TODO
+++ b/TODO
@@ -123,12 +123,6 @@ ATT/GATT
   Priority: Low
   Complexity: C1
 
-- Add command line support to use medium instead of (default) low
-  security level with gatttool (--sec-level)
-
-  Priority: Low
-  Complexity: C1
-
 - Implement Server characteristic Configuration support in the attribute
   server to manage characteristic value broadcasting. There is a single
   instance of the Server Characteristic Configuration for all clients.
diff --git a/attrib/gatttool.c b/attrib/gatttool.c
index b9f5138..bb4fb80 100644
--- a/attrib/gatttool.c
+++ b/attrib/gatttool.c
@@ -49,6 +49,7 @@
 static gchar *opt_src = NULL;
 static gchar *opt_dst = NULL;
 static gchar *opt_value = NULL;
+static gchar *opt_sec_level = "low";
 static int opt_start = 0x0001;
 static int opt_end = 0xffff;
 static int opt_handle = -1;
@@ -84,6 +85,7 @@ static GIOChannel *do_connect(gboolean le)
 	GIOChannel *chan;
 	bdaddr_t sba, dba;
 	GError *err = NULL;
+	BtIOSecLevel sec_level;
 
 	/* This check is required because currently setsockopt() returns no
 	 * errors for MTU values smaller than the allowed minimum. */
@@ -109,13 +111,20 @@ static GIOChannel *do_connect(gboolean le)
 	} else
 		bacpy(&sba, BDADDR_ANY);
 
+	if (strncmp(opt_sec_level, "medium", 6) == 0)
+		sec_level = BT_IO_SEC_MEDIUM;
+	else if (strncmp(opt_sec_level, "high", 4) == 0)
+		sec_level = BT_IO_SEC_HIGH;
+	else
+		sec_level = BT_IO_SEC_LOW;
+
 	if (le)
 		chan = bt_io_connect(BT_IO_L2CAP, connect_cb, NULL, NULL, &err,
 				BT_IO_OPT_SOURCE_BDADDR, &sba,
 				BT_IO_OPT_DEST_BDADDR, &dba,
 				BT_IO_OPT_CID, GATT_CID,
 				BT_IO_OPT_OMTU, opt_mtu,
-				BT_IO_OPT_SEC_LEVEL, BT_IO_SEC_LOW,
+				BT_IO_OPT_SEC_LEVEL, sec_level,
 				BT_IO_OPT_INVALID);
 	else
 		chan = bt_io_connect(BT_IO_L2CAP, connect_cb, NULL, NULL, &err,
@@ -123,7 +132,7 @@ static GIOChannel *do_connect(gboolean le)
 				BT_IO_OPT_DEST_BDADDR, &dba,
 				BT_IO_OPT_PSM, opt_psm,
 				BT_IO_OPT_OMTU, opt_mtu,
-				BT_IO_OPT_SEC_LEVEL, BT_IO_SEC_LOW,
+				BT_IO_OPT_SEC_LEVEL, sec_level,
 				BT_IO_OPT_INVALID);
 
 	if (err) {
@@ -519,6 +528,8 @@ static GOptionEntry options[] = {
 		"Specify the MTU size", "MTU" },
 	{ "psm", 'p', 0, G_OPTION_ARG_INT, &opt_psm,
 		"Specify the PSM for GATT/ATT over BR/EDR", "PSM" },
+	{ "sec-level", 'l', 0, G_OPTION_ARG_STRING, &opt_sec_level,
+		"Set security level. Default: low", "[low | medium | high]"},
 	{ NULL },
 };
 
-- 
1.6.2.rc2

Re: [PATCH] Adding a new option to specify security level for gatttool

[Johan Hedberg]

Hi Sheldon,

On Tue, Nov 16, 2010, Sheldon Demario wrote:
> ---
>  TODO              |    6 ------
>  attrib/gatttool.c |   15 +++++++++++++--
>  2 files changed, 13 insertions(+), 8 deletions(-)

In general the patch seems fine, but:

> +	if (strncmp(opt_sec_level, "medium", 6) == 0)
> +		sec_level = BT_IO_SEC_MEDIUM;
> +	else if (strncmp(opt_sec_level, "high", 4) == 0)
> +		sec_level = BT_IO_SEC_HIGH;
> +	else
> +		sec_level = BT_IO_SEC_LOW;

Why do you use strncmp instead of strcmp (or even g_str_equal)? I don't
think there's any need to map e.g. "mediumfoobar" to BT_IO_SEC_MEDIUM ;)

Johan

RE: [PATCH] Adding a new option to specify security level for gatttool

[Mike Tsai]

Hi,

-----Original Message-----
From: linux-bluetooth-owner@vger.kernel.org [mailto:linux-bluetooth-owner@vger.kernel.org] On Behalf Of Johan Hedberg
Sent: Tuesday, November 16, 2010 7:37 AM
To: Sheldon Demario
Cc: linux-bluetooth@vger.kernel.org
Subject: Re: [PATCH] Adding a new option to specify security level for gatttool

Hi Sheldon,

On Tue, Nov 16, 2010, Sheldon Demario wrote:
> ---
>  TODO              |    6 ------
>  attrib/gatttool.c |   15 +++++++++++++--
>  2 files changed, 13 insertions(+), 8 deletions(-)

In general the patch seems fine, but:

> +	if (strncmp(opt_sec_level, "medium", 6) == 0)
> +		sec_level = BT_IO_SEC_MEDIUM;
> +	else if (strncmp(opt_sec_level, "high", 4) == 0)
> +		sec_level = BT_IO_SEC_HIGH;
> +	else
> +		sec_level = BT_IO_SEC_LOW;

Why do you use strncmp instead of strcmp (or even g_str_equal)? I don't
think there's any need to map e.g. "mediumfoobar" to BT_IO_SEC_MEDIUM ;)

[Mtsai] I am not sure what are the definition of "low", "medium" or "high". By the spec of Core 4.0, LE has 2 security modes and different security levels based on the method of pairing (or bonding). It may be appeal to end user with "low", "medium" and "high" definition, but it can't be reference with LE spec. I would suggest, instead, following terms,

	"No security",
	"unauthenticated encryption",
	"authenticated encryption",
	"unauthenticated data signing",
	"authenticated data signing,

Mike


Johan
--
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  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] Adding a new option to specify security level for gatttool

[tim.howes]

Hi Mike,

-----Original Message-----
> From: linux-bluetooth-owner@vger.kernel.org [mailto:linux-bluetooth-
> owner@vger.kernel.org] On Behalf Of Johan Hedberg
> Sent: Tuesday, November 16, 2010 7:37 AM
> To: Sheldon Demario
> Cc: linux-bluetooth@vger.kernel.org
> Subject: Re: [PATCH] Adding a new option to specify security level for
> gatttool
> 
> Hi Sheldon,
> 
> On Tue, Nov 16, 2010, Sheldon Demario wrote:
> > ---
> >  TODO              |    6 ------
> >  attrib/gatttool.c |   15 +++++++++++++--
> >  2 files changed, 13 insertions(+), 8 deletions(-)
> 
> In general the patch seems fine, but:
> 
> > +	if (strncmp(opt_sec_level, "medium", 6) == 0)
> > +		sec_level = BT_IO_SEC_MEDIUM;
> > +	else if (strncmp(opt_sec_level, "high", 4) == 0)
> > +		sec_level = BT_IO_SEC_HIGH;
> > +	else
> > +		sec_level = BT_IO_SEC_LOW;
> 
> Why do you use strncmp instead of strcmp (or even g_str_equal)? I don't
> think there's any need to map e.g. "mediumfoobar" to BT_IO_SEC_MEDIUM
> ;)
> 
> [Mtsai] I am not sure what are the definition of "low", "medium" or
> "high". By the spec of Core 4.0, LE has 2 security modes and different
> security levels based on the method of pairing (or bonding). It may be
> appeal to end user with "low", "medium" and "high" definition, but it
> can't be reference with LE spec. I would suggest, instead, following
> terms,
> 
> 	"No security",
> 	"unauthenticated encryption",
> 	"authenticated encryption",
> 	"unauthenticated data signing",
> 	"authenticated data signing,
> 
> Mike

To some extent I agree; however, the semantics of such an API would have to be careful.  A particular profile should not "force" data signing because if the link is already encrypted there is little point using data signing.  So from that point of view exposing a more abstract API (a bit like "high") is better.  However, it is hard to map "high" onto any of the ones you listed (which I agree is a good list).  So perhaps it is better to have the API semantics as "advisory" or "requests" which can be fulfilled by the underlying stack in other ways (eg encryption for data-signing).

Cheers

Tim





This message is for the designated recipient only and may contain privileged, proprietary, or otherwise private information.  If you have received it in error, please notify the sender immediately and delete the original.  Any other use of the email by you is prohibited.

RE: [PATCH] Adding a new option to specify security level for gatttool

[Mike Tsai]

Hi Tim,

-----Original Message-----
From: tim.howes@accenture.com [mailto:tim.howes@accenture.com] 
Sent: Wednesday, November 17, 2010 7:04 AM
To: Mike Tsai
Cc: linux-bluetooth@vger.kernel.org
Subject: RE: [PATCH] Adding a new option to specify security level for gatttool

Hi Mike,

-----Original Message-----
> From: linux-bluetooth-owner@vger.kernel.org [mailto:linux-bluetooth-
> owner@vger.kernel.org] On Behalf Of Johan Hedberg
> Sent: Tuesday, November 16, 2010 7:37 AM
> To: Sheldon Demario
> Cc: linux-bluetooth@vger.kernel.org
> Subject: Re: [PATCH] Adding a new option to specify security level for
> gatttool
> 
> Hi Sheldon,
> 
> On Tue, Nov 16, 2010, Sheldon Demario wrote:
> > ---
> >  TODO              |    6 ------
> >  attrib/gatttool.c |   15 +++++++++++++--
> >  2 files changed, 13 insertions(+), 8 deletions(-)
> 
> In general the patch seems fine, but:
> 
> > +	if (strncmp(opt_sec_level, "medium", 6) == 0)
> > +		sec_level = BT_IO_SEC_MEDIUM;
> > +	else if (strncmp(opt_sec_level, "high", 4) == 0)
> > +		sec_level = BT_IO_SEC_HIGH;
> > +	else
> > +		sec_level = BT_IO_SEC_LOW;
> 
> Why do you use strncmp instead of strcmp (or even g_str_equal)? I don't
> think there's any need to map e.g. "mediumfoobar" to BT_IO_SEC_MEDIUM
> ;)
> 
> [Mtsai] I am not sure what are the definition of "low", "medium" or
> "high". By the spec of Core 4.0, LE has 2 security modes and different
> security levels based on the method of pairing (or bonding). It may be
> appeal to end user with "low", "medium" and "high" definition, but it
> can't be reference with LE spec. I would suggest, instead, following
> terms,
> 
> 	"No security",
> 	"unauthenticated encryption",
> 	"authenticated encryption",
> 	"unauthenticated data signing",
> 	"authenticated data signing,
> 
> Mike

To some extent I agree; however, the semantics of such an API would have to be careful.  A particular profile should not "force" data signing because if the link is already encrypted there is little point using data signing.  So from that point of view exposing a more abstract API (a bit like "high") is better.  However, it is hard to map "high" onto any of the ones you listed (which I agree is a good list).  So perhaps it is better to have the API semantics as "advisory" or "requests" which can be fulfilled by the underlying stack in other ways (eg encryption for data-signing).

Cheers

Tim

[Mtsai] I fully embrace the "abstract" API concept. However, all new LE profiles have clearly defined the security request per each attribute now. As a generic attribute profile (as GATT), without clear indication from Application what level of security is requested, GATT/GAP can handle the security request incorrectly and cause IOP issues. For example, what does "low" mean when GAP/SM gets this request from Application? It does not tell GAP/SM what kind of pairing shall be performed and if encryption or data signing for accessing the attributes. 

Cheers,

Mike





This message is for the designated recipient only and may contain privileged, proprietary, or otherwise private information.  If you have received it in error, please notify the sender immediately and delete the original.  Any other use of the email by you is prohibited.

Re: [PATCH] Adding a new option to specify security level for gatttool

[Johan Hedberg]

Hi Tim,

Nice to see you on this list! :)

On Wed, Nov 17, 2010, tim.howes@accenture.com wrote:
> > [Mtsai] I am not sure what are the definition of "low", "medium" or
> > "high". By the spec of Core 4.0, LE has 2 security modes and different
> > security levels based on the method of pairing (or bonding). It may be
> > appeal to end user with "low", "medium" and "high" definition, but it
> > can't be reference with LE spec. I would suggest, instead, following
> > terms,
> > 
> > 	"No security",
> > 	"unauthenticated encryption",
> > 	"authenticated encryption",
> > 	"unauthenticated data signing",
> > 	"authenticated data signing,
> 
> To some extent I agree; however, the semantics of such an API would
> have to be careful.  A particular profile should not "force" data
> signing because if the link is already encrypted there is little point
> using data signing.  So from that point of view exposing a more
> abstract API (a bit like "high") is better.  However, it is hard to
> map "high" onto any of the ones you listed (which I agree is a good
> list).  So perhaps it is better to have the API semantics as
> "advisory" or "requests" which can be fulfilled by the underlying
> stack in other ways (eg encryption for data-signing).

Something like that will probably be needed, yes. However the idea of
the current command line switch to gatttool is to simply map to the
existing kernel API, and that API only has low, medium and high. So at
least in the short term the patch is fine.

Johan