2012-03-05 18:12:31

by Ido Yariv

Subject: [PATCH] Storage: Fix a buffer overflow in write_link_key

The temporary string allocated on the stack is not large enough in worst
case. To be on the safe side, increase it to 64 bytes.

Signed-off-by: Ido Yariv <[email protected]>
---
src/storage.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/src/storage.c b/src/storage.c
index a65cee4..7e7f081 100644
--- a/src/storage.c
+++ b/src/storage.c
@@ -533,7 +533,7 @@ int write_lastused_info(bdaddr_t *local, bdaddr_t *peer, struct tm *tm)

int write_link_key(bdaddr_t *local, bdaddr_t *peer, unsigned char *key, uint8_t type, int length)
{
- char filename[PATH_MAX + 1], addr[18], str[38];
+ char filename[PATH_MAX + 1], addr[18], str[64];
int i;

memset(str, 0, sizeof(str));
--
1.7.7.6

2012-03-06 01:34:15

by Ido Yariv

Subject: Re: [PATCH] Storage: Fix a buffer overflow in write_link_key

Hi Johan,

On Mon, Mar 05, 2012 at 12:01:29PM -0800, Johan Hedberg wrote:
> Hi Ido,
>
> On Mon, Mar 05, 2012, Ido Yariv wrote:
> > The temporary string allocated on the stack is not large enough in worst
> > case. To be on the safe side, increase it to 64 bytes.
> >
> > Signed-off-by: Ido Yariv <[email protected]>
> > ---
> > src/storage.c | 2 +-
> > 1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/src/storage.c b/src/storage.c
> > index a65cee4..7e7f081 100644
> > --- a/src/storage.c
> > +++ b/src/storage.c
> > @@ -533,7 +533,7 @@ int write_lastused_info(bdaddr_t *local, bdaddr_t *peer, struct tm *tm)
> >
> > int write_link_key(bdaddr_t *local, bdaddr_t *peer, unsigned char *key, uint8_t type, int length)
> > {
> > - char filename[PATH_MAX + 1], addr[18], str[38];
> > + char filename[PATH_MAX + 1], addr[18], str[64];
> > int i;
>
> I don't really see how the worst case you mention could ever happen in
> practice. The key type requires one byte and 16 is the maximum for the
> PIN length. Is there something I'm missing?

Thanks for looking into this.

I might have used an old/modified version when I first encountered this.
When I did, the type was actually above 128 (SMP), which ended up taking
3 bytes. As a result, the buffer was overflown.

I'm not sure this can actually happen with the current code, with LTKs
being stored in a separate file. It's probably still safer to allocate
enough space just in case, but it's not essential.

Thanks,
Ido.

2012-03-05 20:01:29

by Johan Hedberg

Subject: Re: [PATCH] Storage: Fix a buffer overflow in write_link_key

Hi Ido,

On Mon, Mar 05, 2012, Ido Yariv wrote:
> The temporary string allocated on the stack is not large enough in worst
> case. To be on the safe side, increase it to 64 bytes.
>
> Signed-off-by: Ido Yariv <[email protected]>
> ---
> src/storage.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/src/storage.c b/src/storage.c
> index a65cee4..7e7f081 100644
> --- a/src/storage.c
> +++ b/src/storage.c
> @@ -533,7 +533,7 @@ int write_lastused_info(bdaddr_t *local, bdaddr_t *peer, struct tm *tm)
>
> int write_link_key(bdaddr_t *local, bdaddr_t *peer, unsigned char *key, uint8_t type, int length)
> {
> - char filename[PATH_MAX + 1], addr[18], str[38];
> + char filename[PATH_MAX + 1], addr[18], str[64];
> int i;

I don't really see how the worst case you mention could ever happen in
practice. The key type requires one byte and 16 is the maximum for the
PIN length. Is there something I'm missing?

Johan