[PATCH] Storage: Fix a buffer overflow in write_link_key

[PATCH] Storage: Fix a buffer overflow in write_link_key

[Ido Yariv]

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 <ido@wizery.com>
---
 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

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

[Johan Hedberg]

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 <ido@wizery.com>
> ---
>  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

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

[Ido Yariv]

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 <ido@wizery.com>
> > ---
> >  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.