[PATCH] Fix starting security procedures when not needed

[PATCH] Fix starting security procedures when not needed

[Vinicius Costa Gomes]

The default value of sec_level when setting *any* option
using bt_io_set() was BT_SECURITY_MEDIUM. This was causing
the security procedure being started in some situations that
it should not.
---
 btio/btio.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/btio/btio.c b/btio/btio.c
index a3cf38a..df028a6 100644
--- a/btio/btio.c
+++ b/btio/btio.c
@@ -659,7 +659,6 @@ static gboolean parse_set_opts(struct set_opts *opts, GError **err,
 	/* Set defaults */
 	opts->defer = DEFAULT_DEFER_TIMEOUT;
 	opts->master = -1;
-	opts->sec_level = BT_IO_SEC_MEDIUM;
 	opts->mode = L2CAP_MODE_BASIC;
 	opts->flushable = -1;
 
-- 
1.7.4.3

Re: [PATCH] Fix starting security procedures when not needed

[Luiz Augusto von Dentz]

Hi Vinicius,

On Tue, May 10, 2011 at 2:49 AM, Vinicius Costa Gomes
<vinicius.gomes@openbossa.org> wrote:
> The default value of sec_level when setting *any* option
> using bt_io_set() was BT_SECURITY_MEDIUM. This was causing
> the security procedure being started in some situations that
> it should not.
> ---
>  btio/btio.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
>
> diff --git a/btio/btio.c b/btio/btio.c
> index a3cf38a..df028a6 100644
> --- a/btio/btio.c
> +++ b/btio/btio.c
> @@ -659,7 +659,6 @@ static gboolean parse_set_opts(struct set_opts *opts, GError **err,
>        /* Set defaults */
>        opts->defer = DEFAULT_DEFER_TIMEOUT;
>        opts->master = -1;
> -       opts->sec_level = BT_IO_SEC_MEDIUM;
>        opts->mode = L2CAP_MODE_BASIC;
>        opts->flushable = -1;

I believe this was on purpose so that if you want another security
level you need to force it when using BtIO, this could be set in the
kernel by default but since it already uses LOW that could break some
applications.


-- 
Luiz Augusto von Dentz
Computer Engineer

Re: [PATCH] Fix starting security procedures when not needed

[Vinicius Costa Gomes]

Hi Luiz,

On 13:21 Tue 10 May, Luiz Augusto von Dentz wrote:
> Hi Vinicius,
> 
> On Tue, May 10, 2011 at 2:49 AM, Vinicius Costa Gomes
> <vinicius.gomes@openbossa.org> wrote:
> > The default value of sec_level when setting *any* option
> > using bt_io_set() was BT_SECURITY_MEDIUM. This was causing
> > the security procedure being started in some situations that
> > it should not.
> > ---
> >  btio/btio.c |    1 -
> >  1 files changed, 0 insertions(+), 1 deletions(-)
> >
> > diff --git a/btio/btio.c b/btio/btio.c
> > index a3cf38a..df028a6 100644
> > --- a/btio/btio.c
> > +++ b/btio/btio.c
> > @@ -659,7 +659,6 @@ static gboolean parse_set_opts(struct set_opts *opts, GError **err,
> >        /* Set defaults */
> >        opts->defer = DEFAULT_DEFER_TIMEOUT;
> >        opts->master = -1;
> > -       opts->sec_level = BT_IO_SEC_MEDIUM;
> >        opts->mode = L2CAP_MODE_BASIC;
> >        opts->flushable = -1;
> 
> I believe this was on purpose so that if you want another security
> level you need to force it when using BtIO, this could be set in the
> kernel by default but since it already uses LOW that could break some
> applications.

If this was by design, I would gladly have my first patch applied.

It is just that it is weird that I have to pass the security level on
every call to bt_io_set() if I don't want the security level to change.

In any case, while writing this email, I realized that only checking
all bt_io_set() calls wasn't enough, so this patch may be incomplete.
All bt_io_listen() users should be safe, but there are some callers
of bt_io_connect() that may depend on BtIO setting the default
security level to MEDIUM.

> 
> 
> -- 
> Luiz Augusto von Dentz
> Computer Engineer


Cheers,
-- 
Vinicius

Re: [PATCH] Fix starting security procedures when not needed

[Luiz Augusto von Dentz]

Hi Vinicius,

On Tue, May 10, 2011 at 5:56 PM, Vinicius Costa Gomes
<vinicius.gomes@openbossa.org> wrote:
> Hi Luiz,
>
> On 13:21 Tue 10 May, Luiz Augusto von Dentz wrote:
>> Hi Vinicius,
>>
>> On Tue, May 10, 2011 at 2:49 AM, Vinicius Costa Gomes
>> <vinicius.gomes@openbossa.org> wrote:
>> > The default value of sec_level when setting *any* option
>> > using bt_io_set() was BT_SECURITY_MEDIUM. This was causing
>> > the security procedure being started in some situations that
>> > it should not.
>> > ---
>> >  btio/btio.c |    1 -
>> >  1 files changed, 0 insertions(+), 1 deletions(-)
>> >
>> > diff --git a/btio/btio.c b/btio/btio.c
>> > index a3cf38a..df028a6 100644
>> > --- a/btio/btio.c
>> > +++ b/btio/btio.c
>> > @@ -659,7 +659,6 @@ static gboolean parse_set_opts(struct set_opts *opts, GError **err,
>> >        /* Set defaults */
>> >        opts->defer = DEFAULT_DEFER_TIMEOUT;
>> >        opts->master = -1;
>> > -       opts->sec_level = BT_IO_SEC_MEDIUM;
>> >        opts->mode = L2CAP_MODE_BASIC;
>> >        opts->flushable = -1;
>>
>> I believe this was on purpose so that if you want another security
>> level you need to force it when using BtIO, this could be set in the
>> kernel by default but since it already uses LOW that could break some
>> applications.
>
> If this was by design, I would gladly have my first patch applied.

Not sure what patch are talking about besides this.

> It is just that it is weird that I have to pass the security level on
> every call to bt_io_set() if I don't want the security level to change.

Actually the other way round, most profiles so far requires security
medium that why BtIO default is medium, for historic reason the
default in kernel is low otherwise we would have set the default to
medium when 2.1 was introduced.

> In any case, while writing this email, I realized that only checking
> all bt_io_set() calls wasn't enough, so this patch may be incomplete.
> All bt_io_listen() users should be safe, but there are some callers
> of bt_io_connect() that may depend on BtIO setting the default
> security level to MEDIUM.

Exactly, so if you really want to use kernel default you will have to
change all of those, but I don't thing we use any other security level
more than medium, then it would better not to change it.


-- 
Luiz Augusto von Dentz
Computer Engineer

Re: [PATCH] Fix starting security procedures when not needed

[Johan Hedberg]

Hi Luiz & Vinicius,

> > In any case, while writing this email, I realized that only checking
> > all bt_io_set() calls wasn't enough, so this patch may be incomplete.
> > All bt_io_listen() users should be safe, but there are some callers
> > of bt_io_connect() that may depend on BtIO setting the default
> > security level to MEDIUM.
> 
> Exactly, so if you really want to use kernel default you will have to
> change all of those, but I don't thing we use any other security level
> more than medium, then it would better not to change it.

The problem is that the same function is used within btio.c for parsing
options, no matter if they're from _set, _connect or _listen. Having an
default to MEDIUM within btio.c means that we don't know when a
bt_io_set caller *doesn't* want to modify the existing security level.
(something the commit message should explain more clearly, btw). So I do
think we need to remove this default from btio.c and have all users set
it explicitly when needed (I could find 2 or 3 places missing it under
audio/ but there could be more). A simple "git grep BT_IO_OPT_" gives a
pretty good overview. Just look for places that give a PSM or RFCOMM
channel but no SEC_LEVEL.

Johan