By sending OPP Put request before CONNECT we were able to cause
SIGSEGV in obexd. Crash was caused by null pointer dereference.
gdb output:
Program received signal SIGSEGV, Segmentation fault.
manager_request_authorization (transfer=transfer@entry=0x0, new_folder=new_folder@entry=0x7fffffffda18, new_name=new_name@entry=0x7fffffffda20) at obexd/src/manager.c:677
677 struct obex_session *os = transfer->session;
(gdb) bt
*#0 manager_request_authorization (transfer=transfer@entry=0x0, new_folder=new_folder@entry=0x7fffffffda18, new_name=new_name@entry=0x7fffffffda20) at obexd/src/manager.c:677
*#1 0x000000000041b7a5 in opp_chkput (os=0x67de60, user_data=0x0) at obexd/plugins/opp.c:80
*#2 0x0000000000426cc5 in check_put (obex=0x678a50, req=0x679250, user_data=0x67de60) at obexd/src/obex.c:831
*#3 cmd_put (obex=0x678a50, req=0x679250, user_data=0x67de60) at obexd/src/obex.c:887
*#4 0x00000000004145e7 in handle_request (req=0x679250, obex=0x678a50) at gobex/gobex.c:1199
*#5 incoming_data (io=<optimized out>, cond=<optimized out>, user_data=0x678a50) at gobex/gobex.c:1375
*#6 0x00007ffff749204a in g_main_dispatch (context=0x674810) at /build/glib2.0-prJhLS/glib2.0-2.48.2/./glib/gmain.c:3154
*#7 g_main_context_dispatch (context=context@entry=0x674810) at /build/glib2.0-prJhLS/glib2.0-2.48.2/./glib/gmain.c:3769
*#8 0x00007ffff74923f0 in g_main_context_iterate (context=0x674810, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>)
at /build/glib2.0-prJhLS/glib2.0-2.48.2/./glib/gmain.c:3840
*#9 0x00007ffff7492712 in g_main_loop_run (loop=0x66fdf0) at /build/glib2.0-prJhLS/glib2.0-2.48.2/./glib/gmain.c:4034
*#10 0x000000000040dd0f in main (argc=1, argv=0x7fffffffde08) at obexd/src/main.c:322
Crash was found using Synopsys Defensics Obex Server test suite.
---
obexd/src/obex.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/obexd/src/obex.c b/obexd/src/obex.c
index 788bffc..91fa838 100644
--- a/obexd/src/obex.c
+++ b/obexd/src/obex.c
@@ -825,7 +825,7 @@ static gboolean check_put(GObex *obex, GObexPacket *req, void *user_data)
struct obex_session *os = user_data;
int ret;
- if (os->service->chkput == NULL)
+ if (os->service->chkput == NULL || os->service_data == NULL)
goto done;
ret = os->service->chkput(os, os->service_data);
--
2.7.4
On Thu, Jun 22, 2017 at 09:18:10AM +0300, Johan Hedberg wrote:
> Hi Matias,
>
> On Thu, Jun 22, 2017, Matias Karhumaa wrote:
> > > >> It seems to me that the bug is with opp.c passing the service_data to
> > > >> manager_request_authorization(), and service_data is expected to be the
> > > >> obex_transfer object. However, currently the code creates this object
> > > >> only upon CONNECT (in opp_connect).
> > > >>
> > > >> I think one possible way to solve this would be to trigger a call to
> > > >> os->service->connect if CONNECT hasn't been explicitly issued, however
> > > >> then the code needs to track this in some other way since service_data
> > > >> seems too unreliable.
> > >
> > > How about the following:
> > >
> > > https://gist.github.com/Vudentz/1736d6af9608b9332b93858d92a3feff
> >
> > Your fix seems solid to me and fixes the crash with OPP. I checked ftp.c
> > also and saw potentially similar problem in ftp_chkput. Should we call
> > os->service->connect also in case of OBEX_FTP?
>
> OPP is the only service where the specification allows skipping the
> connect step. This is why os->service gets pre-initialized to OPP (in
> the obex_session_start function). Also, if the OPP plugin isn't
> available os->service will be NULL before CONNECT, and that's something
> the code already seems to check for (e.g. in cmd_put). So unless I've
> misunderstood something, you shouldn't be able get to a situation where
> os->service points at FTP but CONNECT hasn't been issued.
Ok, understood. Actually I tried to do FTP PUT without CONNECT and I can
confirm that it works as you explained.
I will create a patch from the gist.
Best regards,
Matias Karhumaa
Hi Matias,
On Thu, Jun 22, 2017, Matias Karhumaa wrote:
> > >> It seems to me that the bug is with opp.c passing the service_data to
> > >> manager_request_authorization(), and service_data is expected to be the
> > >> obex_transfer object. However, currently the code creates this object
> > >> only upon CONNECT (in opp_connect).
> > >>
> > >> I think one possible way to solve this would be to trigger a call to
> > >> os->service->connect if CONNECT hasn't been explicitly issued, however
> > >> then the code needs to track this in some other way since service_data
> > >> seems too unreliable.
> >
> > How about the following:
> >
> > https://gist.github.com/Vudentz/1736d6af9608b9332b93858d92a3feff
>
> Your fix seems solid to me and fixes the crash with OPP. I checked ftp.c
> also and saw potentially similar problem in ftp_chkput. Should we call
> os->service->connect also in case of OBEX_FTP?
OPP is the only service where the specification allows skipping the
connect step. This is why os->service gets pre-initialized to OPP (in
the obex_session_start function). Also, if the OPP plugin isn't
available os->service will be NULL before CONNECT, and that's something
the code already seems to check for (e.g. in cmd_put). So unless I've
misunderstood something, you shouldn't be able get to a situation where
os->service points at FTP but CONNECT hasn't been issued.
Johan
On Wed, Jun 21, 2017 at 10:31:39PM +0300, Luiz Augusto von Dentz wrote:
> Hi Matias,
>
> On Wed, Jun 21, 2017 at 8:36 PM, Luiz Augusto von Dentz
> <[email protected]> wrote:
> > Hi Johan,
> >
> > On Wed, Jun 21, 2017 at 12:41 PM, Johan Hedberg <[email protected]> wrote:
> >> Hi Matias,
> >>
> >> On Wed, Jun 21, 2017, Matias Karhumaa wrote:
> >>> By sending OPP Put request before CONNECT we were able to cause
> >>> SIGSEGV in obexd. Crash was caused by null pointer dereference.
> >>>
> >>> gdb output:
> >>>
> >>> Program received signal SIGSEGV, Segmentation fault.
> >>> manager_request_authorization (transfer=transfer@entry=0x0, new_folder=new_folder@entry=0x7fffffffda18, new_name=new_name@entry=0x7fffffffda20) at obexd/src/manager.c:677
> >>> 677 struct obex_session *os = transfer->session;
> >>> (gdb) bt
> >>> *#0 manager_request_authorization (transfer=transfer@entry=0x0, new_folder=new_folder@entry=0x7fffffffda18, new_name=new_name@entry=0x7fffffffda20) at obexd/src/manager.c:677
> >>> *#1 0x000000000041b7a5 in opp_chkput (os=0x67de60, user_data=0x0) at obexd/plugins/opp.c:80
> >>> *#2 0x0000000000426cc5 in check_put (obex=0x678a50, req=0x679250, user_data=0x67de60) at obexd/src/obex.c:831
> >>> *#3 cmd_put (obex=0x678a50, req=0x679250, user_data=0x67de60) at obexd/src/obex.c:887
> >>> *#4 0x00000000004145e7 in handle_request (req=0x679250, obex=0x678a50) at gobex/gobex.c:1199
> >>> *#5 incoming_data (io=<optimized out>, cond=<optimized out>, user_data=0x678a50) at gobex/gobex.c:1375
> >>> *#6 0x00007ffff749204a in g_main_dispatch (context=0x674810) at /build/glib2.0-prJhLS/glib2.0-2.48.2/./glib/gmain.c:3154
> >>> *#7 g_main_context_dispatch (context=context@entry=0x674810) at /build/glib2.0-prJhLS/glib2.0-2.48.2/./glib/gmain.c:3769
> >>> *#8 0x00007ffff74923f0 in g_main_context_iterate (context=0x674810, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>)
> >>> at /build/glib2.0-prJhLS/glib2.0-2.48.2/./glib/gmain.c:3840
> >>> *#9 0x00007ffff7492712 in g_main_loop_run (loop=0x66fdf0) at /build/glib2.0-prJhLS/glib2.0-2.48.2/./glib/gmain.c:4034
> >>> *#10 0x000000000040dd0f in main (argc=1, argv=0x7fffffffde08) at obexd/src/main.c:322
> >>>
> >>> Crash was found using Synopsys Defensics Obex Server test suite.
> >>> ---
> >>> obexd/src/obex.c | 2 +-
> >>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/obexd/src/obex.c b/obexd/src/obex.c
> >>> index 788bffc..91fa838 100644
> >>> --- a/obexd/src/obex.c
> >>> +++ b/obexd/src/obex.c
> >>> @@ -825,7 +825,7 @@ static gboolean check_put(GObex *obex, GObexPacket *req, void *user_data)
> >>> struct obex_session *os = user_data;
> >>> int ret;
> >>>
> >>> - if (os->service->chkput == NULL)
> >>> + if (os->service->chkput == NULL || os->service_data == NULL)
> >>> goto done;
> >>
> >> As far as I understand, os->service_data is the OBEX service-specific
> >> context, which I think can in principle validly be NULL. Also, isn't it
> >> so that OBEX Object Push permits PUT without a preceding CONNECT (at
> >> least that's what I remember from the times I was working with OBEX)?
> >
> > Yep, OPP can start a transfer without a CONNECT so the service_data
> > will need to be instantiated directly on PUT if that wasn't created
> > yet.
> >
> >> It seems to me that the bug is with opp.c passing the service_data to
> >> manager_request_authorization(), and service_data is expected to be the
> >> obex_transfer object. However, currently the code creates this object
> >> only upon CONNECT (in opp_connect).
> >>
> >> I think one possible way to solve this would be to trigger a call to
> >> os->service->connect if CONNECT hasn't been explicitly issued, however
> >> then the code needs to track this in some other way since service_data
> >> seems too unreliable.
>
> How about the following:
>
> https://gist.github.com/Vudentz/1736d6af9608b9332b93858d92a3feff
Your fix seems solid to me and fixes the crash with OPP. I checked ftp.c
also and saw potentially similar problem in ftp_chkput. Should we call
os->service->connect also in case of OBEX_FTP?
Best Regards,
Matias Karhumaa
Hi Matias,
On Wed, Jun 21, 2017 at 8:36 PM, Luiz Augusto von Dentz
<[email protected]> wrote:
> Hi Johan,
>
> On Wed, Jun 21, 2017 at 12:41 PM, Johan Hedberg <[email protected]> wrote:
>> Hi Matias,
>>
>> On Wed, Jun 21, 2017, Matias Karhumaa wrote:
>>> By sending OPP Put request before CONNECT we were able to cause
>>> SIGSEGV in obexd. Crash was caused by null pointer dereference.
>>>
>>> gdb output:
>>>
>>> Program received signal SIGSEGV, Segmentation fault.
>>> manager_request_authorization (transfer=transfer@entry=0x0, new_folder=new_folder@entry=0x7fffffffda18, new_name=new_name@entry=0x7fffffffda20) at obexd/src/manager.c:677
>>> 677 struct obex_session *os = transfer->session;
>>> (gdb) bt
>>> *#0 manager_request_authorization (transfer=transfer@entry=0x0, new_folder=new_folder@entry=0x7fffffffda18, new_name=new_name@entry=0x7fffffffda20) at obexd/src/manager.c:677
>>> *#1 0x000000000041b7a5 in opp_chkput (os=0x67de60, user_data=0x0) at obexd/plugins/opp.c:80
>>> *#2 0x0000000000426cc5 in check_put (obex=0x678a50, req=0x679250, user_data=0x67de60) at obexd/src/obex.c:831
>>> *#3 cmd_put (obex=0x678a50, req=0x679250, user_data=0x67de60) at obexd/src/obex.c:887
>>> *#4 0x00000000004145e7 in handle_request (req=0x679250, obex=0x678a50) at gobex/gobex.c:1199
>>> *#5 incoming_data (io=<optimized out>, cond=<optimized out>, user_data=0x678a50) at gobex/gobex.c:1375
>>> *#6 0x00007ffff749204a in g_main_dispatch (context=0x674810) at /build/glib2.0-prJhLS/glib2.0-2.48.2/./glib/gmain.c:3154
>>> *#7 g_main_context_dispatch (context=context@entry=0x674810) at /build/glib2.0-prJhLS/glib2.0-2.48.2/./glib/gmain.c:3769
>>> *#8 0x00007ffff74923f0 in g_main_context_iterate (context=0x674810, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>)
>>> at /build/glib2.0-prJhLS/glib2.0-2.48.2/./glib/gmain.c:3840
>>> *#9 0x00007ffff7492712 in g_main_loop_run (loop=0x66fdf0) at /build/glib2.0-prJhLS/glib2.0-2.48.2/./glib/gmain.c:4034
>>> *#10 0x000000000040dd0f in main (argc=1, argv=0x7fffffffde08) at obexd/src/main.c:322
>>>
>>> Crash was found using Synopsys Defensics Obex Server test suite.
>>> ---
>>> obexd/src/obex.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/obexd/src/obex.c b/obexd/src/obex.c
>>> index 788bffc..91fa838 100644
>>> --- a/obexd/src/obex.c
>>> +++ b/obexd/src/obex.c
>>> @@ -825,7 +825,7 @@ static gboolean check_put(GObex *obex, GObexPacket *req, void *user_data)
>>> struct obex_session *os = user_data;
>>> int ret;
>>>
>>> - if (os->service->chkput == NULL)
>>> + if (os->service->chkput == NULL || os->service_data == NULL)
>>> goto done;
>>
>> As far as I understand, os->service_data is the OBEX service-specific
>> context, which I think can in principle validly be NULL. Also, isn't it
>> so that OBEX Object Push permits PUT without a preceding CONNECT (at
>> least that's what I remember from the times I was working with OBEX)?
>
> Yep, OPP can start a transfer without a CONNECT so the service_data
> will need to be instantiated directly on PUT if that wasn't created
> yet.
>
>> It seems to me that the bug is with opp.c passing the service_data to
>> manager_request_authorization(), and service_data is expected to be the
>> obex_transfer object. However, currently the code creates this object
>> only upon CONNECT (in opp_connect).
>>
>> I think one possible way to solve this would be to trigger a call to
>> os->service->connect if CONNECT hasn't been explicitly issued, however
>> then the code needs to track this in some other way since service_data
>> seems too unreliable.
How about the following:
https://gist.github.com/Vudentz/1736d6af9608b9332b93858d92a3feff
--
Luiz Augusto von Dentz
Hi Johan,
On Wed, Jun 21, 2017 at 12:41 PM, Johan Hedberg <[email protected]> wrote:
> Hi Matias,
>
> On Wed, Jun 21, 2017, Matias Karhumaa wrote:
>> By sending OPP Put request before CONNECT we were able to cause
>> SIGSEGV in obexd. Crash was caused by null pointer dereference.
>>
>> gdb output:
>>
>> Program received signal SIGSEGV, Segmentation fault.
>> manager_request_authorization (transfer=transfer@entry=0x0, new_folder=new_folder@entry=0x7fffffffda18, new_name=new_name@entry=0x7fffffffda20) at obexd/src/manager.c:677
>> 677 struct obex_session *os = transfer->session;
>> (gdb) bt
>> *#0 manager_request_authorization (transfer=transfer@entry=0x0, new_folder=new_folder@entry=0x7fffffffda18, new_name=new_name@entry=0x7fffffffda20) at obexd/src/manager.c:677
>> *#1 0x000000000041b7a5 in opp_chkput (os=0x67de60, user_data=0x0) at obexd/plugins/opp.c:80
>> *#2 0x0000000000426cc5 in check_put (obex=0x678a50, req=0x679250, user_data=0x67de60) at obexd/src/obex.c:831
>> *#3 cmd_put (obex=0x678a50, req=0x679250, user_data=0x67de60) at obexd/src/obex.c:887
>> *#4 0x00000000004145e7 in handle_request (req=0x679250, obex=0x678a50) at gobex/gobex.c:1199
>> *#5 incoming_data (io=<optimized out>, cond=<optimized out>, user_data=0x678a50) at gobex/gobex.c:1375
>> *#6 0x00007ffff749204a in g_main_dispatch (context=0x674810) at /build/glib2.0-prJhLS/glib2.0-2.48.2/./glib/gmain.c:3154
>> *#7 g_main_context_dispatch (context=context@entry=0x674810) at /build/glib2.0-prJhLS/glib2.0-2.48.2/./glib/gmain.c:3769
>> *#8 0x00007ffff74923f0 in g_main_context_iterate (context=0x674810, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>)
>> at /build/glib2.0-prJhLS/glib2.0-2.48.2/./glib/gmain.c:3840
>> *#9 0x00007ffff7492712 in g_main_loop_run (loop=0x66fdf0) at /build/glib2.0-prJhLS/glib2.0-2.48.2/./glib/gmain.c:4034
>> *#10 0x000000000040dd0f in main (argc=1, argv=0x7fffffffde08) at obexd/src/main.c:322
>>
>> Crash was found using Synopsys Defensics Obex Server test suite.
>> ---
>> obexd/src/obex.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/obexd/src/obex.c b/obexd/src/obex.c
>> index 788bffc..91fa838 100644
>> --- a/obexd/src/obex.c
>> +++ b/obexd/src/obex.c
>> @@ -825,7 +825,7 @@ static gboolean check_put(GObex *obex, GObexPacket *req, void *user_data)
>> struct obex_session *os = user_data;
>> int ret;
>>
>> - if (os->service->chkput == NULL)
>> + if (os->service->chkput == NULL || os->service_data == NULL)
>> goto done;
>
> As far as I understand, os->service_data is the OBEX service-specific
> context, which I think can in principle validly be NULL. Also, isn't it
> so that OBEX Object Push permits PUT without a preceding CONNECT (at
> least that's what I remember from the times I was working with OBEX)?
Yep, OPP can start a transfer without a CONNECT so the service_data
will need to be instantiated directly on PUT if that wasn't created
yet.
> It seems to me that the bug is with opp.c passing the service_data to
> manager_request_authorization(), and service_data is expected to be the
> obex_transfer object. However, currently the code creates this object
> only upon CONNECT (in opp_connect).
>
> I think one possible way to solve this would be to trigger a call to
> os->service->connect if CONNECT hasn't been explicitly issued, however
> then the code needs to track this in some other way since service_data
> seems too unreliable.
>
> Johan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Luiz Augusto von Dentz
Hi Matias,
On Wed, Jun 21, 2017, Matias Karhumaa wrote:
> By sending OPP Put request before CONNECT we were able to cause
> SIGSEGV in obexd. Crash was caused by null pointer dereference.
>
> gdb output:
>
> Program received signal SIGSEGV, Segmentation fault.
> manager_request_authorization (transfer=transfer@entry=0x0, new_folder=new_folder@entry=0x7fffffffda18, new_name=new_name@entry=0x7fffffffda20) at obexd/src/manager.c:677
> 677 struct obex_session *os = transfer->session;
> (gdb) bt
> *#0 manager_request_authorization (transfer=transfer@entry=0x0, new_folder=new_folder@entry=0x7fffffffda18, new_name=new_name@entry=0x7fffffffda20) at obexd/src/manager.c:677
> *#1 0x000000000041b7a5 in opp_chkput (os=0x67de60, user_data=0x0) at obexd/plugins/opp.c:80
> *#2 0x0000000000426cc5 in check_put (obex=0x678a50, req=0x679250, user_data=0x67de60) at obexd/src/obex.c:831
> *#3 cmd_put (obex=0x678a50, req=0x679250, user_data=0x67de60) at obexd/src/obex.c:887
> *#4 0x00000000004145e7 in handle_request (req=0x679250, obex=0x678a50) at gobex/gobex.c:1199
> *#5 incoming_data (io=<optimized out>, cond=<optimized out>, user_data=0x678a50) at gobex/gobex.c:1375
> *#6 0x00007ffff749204a in g_main_dispatch (context=0x674810) at /build/glib2.0-prJhLS/glib2.0-2.48.2/./glib/gmain.c:3154
> *#7 g_main_context_dispatch (context=context@entry=0x674810) at /build/glib2.0-prJhLS/glib2.0-2.48.2/./glib/gmain.c:3769
> *#8 0x00007ffff74923f0 in g_main_context_iterate (context=0x674810, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>)
> at /build/glib2.0-prJhLS/glib2.0-2.48.2/./glib/gmain.c:3840
> *#9 0x00007ffff7492712 in g_main_loop_run (loop=0x66fdf0) at /build/glib2.0-prJhLS/glib2.0-2.48.2/./glib/gmain.c:4034
> *#10 0x000000000040dd0f in main (argc=1, argv=0x7fffffffde08) at obexd/src/main.c:322
>
> Crash was found using Synopsys Defensics Obex Server test suite.
> ---
> obexd/src/obex.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/obexd/src/obex.c b/obexd/src/obex.c
> index 788bffc..91fa838 100644
> --- a/obexd/src/obex.c
> +++ b/obexd/src/obex.c
> @@ -825,7 +825,7 @@ static gboolean check_put(GObex *obex, GObexPacket *req, void *user_data)
> struct obex_session *os = user_data;
> int ret;
>
> - if (os->service->chkput == NULL)
> + if (os->service->chkput == NULL || os->service_data == NULL)
> goto done;
As far as I understand, os->service_data is the OBEX service-specific
context, which I think can in principle validly be NULL. Also, isn't it
so that OBEX Object Push permits PUT without a preceding CONNECT (at
least that's what I remember from the times I was working with OBEX)?
It seems to me that the bug is with opp.c passing the service_data to
manager_request_authorization(), and service_data is expected to be the
obex_transfer object. However, currently the code creates this object
only upon CONNECT (in opp_connect).
I think one possible way to solve this would be to trigger a call to
os->service->connect if CONNECT hasn't been explicitly issued, however
then the code needs to track this in some other way since service_data
seems too unreliable.
Johan