2018-05-29 08:32:17

by Grzegorz Kołodziejczyk

[permalink] [raw]
Subject: [PATCH BlueZ 1/2] client: Don't require authorization for trusted devices

This patch adds possibility to ommit authorization request from trusted
devices.
---
client/gatt.c | 21 ++++++++++++++++++---
1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/client/gatt.c b/client/gatt.c
index 3e70f365c..c7dfe42d7 100644
--- a/client/gatt.c
+++ b/client/gatt.c
@@ -1720,6 +1720,20 @@ error:
g_free(aad);
}

+static bool is_device_trusted(const char *path)
+{
+ GDBusProxy *proxy;
+ DBusMessageIter iter;
+ bool trusted;
+
+ proxy = bt_shell_get_env(path);
+
+ if (g_dbus_proxy_get_property(proxy, "Trusted", &iter))
+ dbus_message_iter_get_basic(&iter, &trusted);
+
+ return trusted;
+}
+
static DBusMessage *chrc_read_value(DBusConnection *conn, DBusMessage *msg,
void *user_data)
{
@@ -1739,7 +1753,7 @@ static DBusMessage *chrc_read_value(DBusConnection *conn, DBusMessage *msg,
bt_shell_printf("ReadValue: %s offset %u link %s\n",
path_to_address(device), offset, link);

- if (chrc->authorization_req) {
+ if (!is_device_trusted(device) && chrc->authorization_req) {
struct authorize_attribute_data *aad;

aad = g_new0(struct authorize_attribute_data, 1);
@@ -1865,6 +1879,7 @@ static DBusMessage *chrc_write_value(DBusConnection *conn, DBusMessage *msg,
struct chrc *chrc = user_data;
uint16_t offset = 0;
bool prep_authorize = false;
+ char *device = NULL;
DBusMessageIter iter;
int value_len;
uint8_t *value;
@@ -1877,11 +1892,11 @@ static DBusMessage *chrc_write_value(DBusConnection *conn, DBusMessage *msg,
"org.bluez.Error.InvalidArguments", NULL);

dbus_message_iter_next(&iter);
- if (parse_options(&iter, &offset, NULL, NULL, NULL, &prep_authorize))
+ if (parse_options(&iter, &offset, NULL, &device, NULL, &prep_authorize))
return g_dbus_create_error(msg,
"org.bluez.Error.InvalidArguments", NULL);

- if (chrc->authorization_req) {
+ if (!is_device_trusted(device) && chrc->authorization_req) {
struct authorize_attribute_data *aad;

aad = g_new0(struct authorize_attribute_data, 1);
--
2.13.6



2018-05-29 11:39:37

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH BlueZ 1/2] client: Don't require authorization for trusted devices

Hi Grzegorz,

On Tue, May 29, 2018 at 11:32 AM, Grzegorz Kolodziejczyk
<[email protected]> wrote:
> This patch adds possibility to ommit authorization request from trusted
> devices.
> ---
> client/gatt.c | 21 ++++++++++++++++++---
> 1 file changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/client/gatt.c b/client/gatt.c
> index 3e70f365c..c7dfe42d7 100644
> --- a/client/gatt.c
> +++ b/client/gatt.c
> @@ -1720,6 +1720,20 @@ error:
> g_free(aad);
> }
>
> +static bool is_device_trusted(const char *path)
> +{
> + GDBusProxy *proxy;
> + DBusMessageIter iter;
> + bool trusted;
> +
> + proxy = bt_shell_get_env(path);
> +
> + if (g_dbus_proxy_get_property(proxy, "Trusted", &iter))
> + dbus_message_iter_get_basic(&iter, &trusted);
> +
> + return trusted;
> +}
> +
> static DBusMessage *chrc_read_value(DBusConnection *conn, DBusMessage *msg,
> void *user_data)
> {
> @@ -1739,7 +1753,7 @@ static DBusMessage *chrc_read_value(DBusConnection *conn, DBusMessage *msg,
> bt_shell_printf("ReadValue: %s offset %u link %s\n",
> path_to_address(device), offset, link);
>
> - if (chrc->authorization_req) {
> + if (!is_device_trusted(device) && chrc->authorization_req) {
> struct authorize_attribute_data *aad;
>
> aad = g_new0(struct authorize_attribute_data, 1);
> @@ -1865,6 +1879,7 @@ static DBusMessage *chrc_write_value(DBusConnection *conn, DBusMessage *msg,
> struct chrc *chrc = user_data;
> uint16_t offset = 0;
> bool prep_authorize = false;
> + char *device = NULL;
> DBusMessageIter iter;
> int value_len;
> uint8_t *value;
> @@ -1877,11 +1892,11 @@ static DBusMessage *chrc_write_value(DBusConnection *conn, DBusMessage *msg,
> "org.bluez.Error.InvalidArguments", NULL);
>
> dbus_message_iter_next(&iter);
> - if (parse_options(&iter, &offset, NULL, NULL, NULL, &prep_authorize))
> + if (parse_options(&iter, &offset, NULL, &device, NULL, &prep_authorize))
> return g_dbus_create_error(msg,
> "org.bluez.Error.InvalidArguments", NULL);
>
> - if (chrc->authorization_req) {
> + if (!is_device_trusted(device) && chrc->authorization_req) {
> struct authorize_attribute_data *aad;
>
> aad = g_new0(struct authorize_attribute_data, 1);
> --
> 2.13.6

Applied, thanks.

--
Luiz Augusto von Dentz

2018-05-29 10:14:01

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH BlueZ 2/2] gatt: Don't require prepare write authorization for trusted devices

Hi Grzegorz,

On Tue, May 29, 2018 at 1:05 PM, Grzegorz Ko=C5=82odziejczyk
<[email protected]> wrote:
> Hi Luiz,
>
>
> wt., 29 maj 2018 o 11:39 Luiz Augusto von Dentz <[email protected]>
> napisa=C5=82(a):
>
>> Hi Grzegorz,
>
>> On Tue, May 29, 2018 at 11:32 AM, Grzegorz Kolodziejczyk
>> <[email protected]> wrote:
>> > This patch adds possibility to ommit prepare write authorization
>> > request from trusted devices.
>> > ---
>> > src/gatt-database.c | 6 ++++--
>> > 1 file changed, 4 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/src/gatt-database.c b/src/gatt-database.c
>> > index 22c78e840..99c8ea231 100644
>> > --- a/src/gatt-database.c
>> > +++ b/src/gatt-database.c
>> > @@ -2569,7 +2569,8 @@ static void desc_write_cb(struct
> gatt_db_attribute *attrib,
>> > }
>> >
>> > if (opcode =3D=3D BT_ATT_OP_PREP_WRITE_REQ) {
>> > - if (!desc->prep_authorized &&
> desc->req_prep_authorization)
>> > + if (!device_is_trusted(device) &&
> !desc->prep_authorized &&
>> > +
> desc->req_prep_authorization)
>> > send_write(device, attrib, desc->proxy,
>> > desc->pending_writes, id,
> value, len,
>> > offset,
> bt_att_get_link_type(att),
>> > @@ -2672,7 +2673,8 @@ static void chrc_write_cb(struct
> gatt_db_attribute *attrib,
>> > queue =3D NULL;
>> >
>> > if (opcode =3D=3D BT_ATT_OP_PREP_WRITE_REQ) {
>> > - if (!chrc->prep_authorized &&
> chrc->req_prep_authorization)
>> > + if (!device_is_trusted(device) &&
> !chrc->prep_authorized &&
>> > +
> chrc->req_prep_authorization)
>> > send_write(device, attrib, chrc->proxy, queue,
>> > id, value, len, offset,
>> > bt_att_get_link_type(att),
> true, true);
>> > --
>> > 2.13.6
>
>> I guess I can ignore the first patch and just apply this one, or do
>> you still have any use for checking the trusted flag on the client
>> side?
>
> Yes, I have use for checking the trusted flag on client side.
>
> For example:
> - Read/write,
> - Long read, long write,
> - Execute write

Right, so those are to prevent the client to show any prompt, got it now.

> In all those cases user will be bothered with authorization request if
> trusted won't be checked.
>
> This patch 2/2 reduce redundant authorization request for prepare writes
> only since we know that device is trusted on daemon side. In other cases
> operation is authorized on client side, so I think there is no other plac=
e
> where we can check if device is trusted.
>
>> --
>> Luiz Augusto von Dentz
>
> Regards,
> Grzegorz



--=20
Luiz Augusto von Dentz

2018-05-29 10:05:43

by Grzegorz Kołodziejczyk

[permalink] [raw]
Subject: Re: [PATCH BlueZ 2/2] gatt: Don't require prepare write authorization for trusted devices

Hi Luiz,


wt., 29 maj 2018 o 11:39 Luiz Augusto von Dentz <[email protected]>
napisa=C5=82(a):

> Hi Grzegorz,

> On Tue, May 29, 2018 at 11:32 AM, Grzegorz Kolodziejczyk
> <[email protected]> wrote:
> > This patch adds possibility to ommit prepare write authorization
> > request from trusted devices.
> > ---
> > src/gatt-database.c | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/gatt-database.c b/src/gatt-database.c
> > index 22c78e840..99c8ea231 100644
> > --- a/src/gatt-database.c
> > +++ b/src/gatt-database.c
> > @@ -2569,7 +2569,8 @@ static void desc_write_cb(struct
gatt_db_attribute *attrib,
> > }
> >
> > if (opcode =3D=3D BT_ATT_OP_PREP_WRITE_REQ) {
> > - if (!desc->prep_authorized &&
desc->req_prep_authorization)
> > + if (!device_is_trusted(device) &&
!desc->prep_authorized &&
> > +
desc->req_prep_authorization)
> > send_write(device, attrib, desc->proxy,
> > desc->pending_writes, id,
value, len,
> > offset,
bt_att_get_link_type(att),
> > @@ -2672,7 +2673,8 @@ static void chrc_write_cb(struct
gatt_db_attribute *attrib,
> > queue =3D NULL;
> >
> > if (opcode =3D=3D BT_ATT_OP_PREP_WRITE_REQ) {
> > - if (!chrc->prep_authorized &&
chrc->req_prep_authorization)
> > + if (!device_is_trusted(device) &&
!chrc->prep_authorized &&
> > +
chrc->req_prep_authorization)
> > send_write(device, attrib, chrc->proxy, queue,
> > id, value, len, offset,
> > bt_att_get_link_type(att),
true, true);
> > --
> > 2.13.6

> I guess I can ignore the first patch and just apply this one, or do
> you still have any use for checking the trusted flag on the client
> side?

Yes, I have use for checking the trusted flag on client side.

For example:
- Read/write,
- Long read, long write,
- Execute write

In all those cases user will be bothered with authorization request if
trusted won't be checked.

This patch 2/2 reduce redundant authorization request for prepare writes
only since we know that device is trusted on daemon side. In other cases
operation is authorized on client side, so I think there is no other place
where we can check if device is trusted.

> --
> Luiz Augusto von Dentz

Regards,
Grzegorz

2018-05-29 09:39:57

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH BlueZ 2/2] gatt: Don't require prepare write authorization for trusted devices

Hi Grzegorz,

On Tue, May 29, 2018 at 11:32 AM, Grzegorz Kolodziejczyk
<[email protected]> wrote:
> This patch adds possibility to ommit prepare write authorization
> request from trusted devices.
> ---
> src/gatt-database.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/src/gatt-database.c b/src/gatt-database.c
> index 22c78e840..99c8ea231 100644
> --- a/src/gatt-database.c
> +++ b/src/gatt-database.c
> @@ -2569,7 +2569,8 @@ static void desc_write_cb(struct gatt_db_attribute *attrib,
> }
>
> if (opcode == BT_ATT_OP_PREP_WRITE_REQ) {
> - if (!desc->prep_authorized && desc->req_prep_authorization)
> + if (!device_is_trusted(device) && !desc->prep_authorized &&
> + desc->req_prep_authorization)
> send_write(device, attrib, desc->proxy,
> desc->pending_writes, id, value, len,
> offset, bt_att_get_link_type(att),
> @@ -2672,7 +2673,8 @@ static void chrc_write_cb(struct gatt_db_attribute *attrib,
> queue = NULL;
>
> if (opcode == BT_ATT_OP_PREP_WRITE_REQ) {
> - if (!chrc->prep_authorized && chrc->req_prep_authorization)
> + if (!device_is_trusted(device) && !chrc->prep_authorized &&
> + chrc->req_prep_authorization)
> send_write(device, attrib, chrc->proxy, queue,
> id, value, len, offset,
> bt_att_get_link_type(att), true, true);
> --
> 2.13.6

I guess I can ignore the first patch and just apply this one, or do
you still have any use for checking the trusted flag on the client
side?

--
Luiz Augusto von Dentz

2018-05-29 08:32:18

by Grzegorz Kołodziejczyk

[permalink] [raw]
Subject: [PATCH BlueZ 2/2] gatt: Don't require prepare write authorization for trusted devices

This patch adds possibility to ommit prepare write authorization
request from trusted devices.
---
src/gatt-database.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/gatt-database.c b/src/gatt-database.c
index 22c78e840..99c8ea231 100644
--- a/src/gatt-database.c
+++ b/src/gatt-database.c
@@ -2569,7 +2569,8 @@ static void desc_write_cb(struct gatt_db_attribute *attrib,
}

if (opcode == BT_ATT_OP_PREP_WRITE_REQ) {
- if (!desc->prep_authorized && desc->req_prep_authorization)
+ if (!device_is_trusted(device) && !desc->prep_authorized &&
+ desc->req_prep_authorization)
send_write(device, attrib, desc->proxy,
desc->pending_writes, id, value, len,
offset, bt_att_get_link_type(att),
@@ -2672,7 +2673,8 @@ static void chrc_write_cb(struct gatt_db_attribute *attrib,
queue = NULL;

if (opcode == BT_ATT_OP_PREP_WRITE_REQ) {
- if (!chrc->prep_authorized && chrc->req_prep_authorization)
+ if (!device_is_trusted(device) && !chrc->prep_authorized &&
+ chrc->req_prep_authorization)
send_write(device, attrib, chrc->proxy, queue,
id, value, len, offset,
bt_att_get_link_type(att), true, true);
--
2.13.6