[5/5] WPS: Fix possible memory leak in wps_er_config_token_from_cred()

From: Ben Rosenfeld <ben.rosenfeld@intel.com>

In wps_er_config_token_from_cred() data.new_pak memory is allocated in
wps_build_cred() and the function returns before the memroy is released.

Signed-off-by: Ben Rosenfeld <ben.rosenfeld@intel.com>
---
 src/wps/wps_er.c | 2 ++
 1 file changed, 2 insertions(+)

On Wed, Jun 17, 2015 at 04:16:36PM +0300, Ilan Peer wrote:
> In wps_er_config_token_from_cred() data.new_pak memory is allocated in
> wps_build_cred() and the function returns before the memroy is released.

> diff --git a/src/wps/wps_er.c b/src/wps/wps_er.c
> @@ -2039,10 +2039,12 @@ struct wpabuf * wps_er_config_token_from_cred(struct wps_context *wps,
>  	data.use_cred = cred;
>  	if (wps_build_cred(&data, ret) ||
>  	    wps_build_wfa_ext(ret, 0, NULL, 0)) {
> +		os_free(data.new_psk);
>  		wpabuf_free(ret);
>  		return NULL;
>  	}
>  
> +	os_free(data.new_psk);

Could you please clarify how data.new_psk could be allocated on this
code path? data.use_cred is used to skip new credential allocation in
wps_build_cred(), i.e., all the cases that could allocate new_psk are
skipped with "goto use_provided".

Hi Jouni,

> -----Original Message-----
> From: Jouni Malinen [mailto:j@w1.fi]
> Sent: Friday, June 19, 2015 01:22
> To: Peer, Ilan
> Cc: hostap@lists.shmoo.com; Rosenfeld, Ben
> Subject: Re: [PATCH 5/5] WPS: Fix possible memory leak in
> wps_er_config_token_from_cred()
> 
> On Wed, Jun 17, 2015 at 04:16:36PM +0300, Ilan Peer wrote:
> > In wps_er_config_token_from_cred() data.new_pak memory is allocated in
> > wps_build_cred() and the function returns before the memroy is released.
> 
> > diff --git a/src/wps/wps_er.c b/src/wps/wps_er.c @@ -2039,10 +2039,12
> > @@ struct wpabuf * wps_er_config_token_from_cred(struct wps_context
> *wps,
> >  	data.use_cred = cred;
> >  	if (wps_build_cred(&data, ret) ||
> >  	    wps_build_wfa_ext(ret, 0, NULL, 0)) {
> > +		os_free(data.new_psk);
> >  		wpabuf_free(ret);
> >  		return NULL;
> >  	}
> >
> > +	os_free(data.new_psk);
> 
> Could you please clarify how data.new_psk could be allocated on this code
> path? data.use_cred is used to skip new credential allocation in
> wps_build_cred(), i.e., all the cases that could allocate new_psk are skipped
> with "goto use_provided".
> 

This is the traceback from the tool:

wps_er.c:2040: Dynamic memory stored in 'data.new_psk' is allocated by calling function 'wps_build_cred'.
wps_registrar.c:1686: wps->auth_type& (2|32) is true
wps_registrar.c:1691: 'wps->new_psk' is allocated by function 'malloc'.
wps_registrar.c:1692: wps->new_psk== ( (void* )0) is false
wps_registrar.c:1694: random_pool_ready() !=1||random_get_bytes(wps->new_psk, wps->new_psk_len) <0 is false
wps_er.c:2040: wps_build_cred( &data, ret) ||wps_build_wfa_ext(ret, 0, ( (void* )0), 0) is true
wps_er.c:2043: Dynamic memory stored in 'data.new_psk' is lost.


Regards,

Ilan.

On Sun, Jun 21, 2015 at 01:56:45PM +0000, Peer, Ilan wrote:
> > > In wps_er_config_token_from_cred() data.new_pak memory is allocated in
> > > wps_build_cred() and the function returns before the memroy is released.
> > 
> > > diff --git a/src/wps/wps_er.c b/src/wps/wps_er.c @@ -2039,10 +2039,12
> > > @@ struct wpabuf * wps_er_config_token_from_cred(struct wps_context
> > *wps,
> > >  	data.use_cred = cred;
> > >  	if (wps_build_cred(&data, ret) ||
> > >  	    wps_build_wfa_ext(ret, 0, NULL, 0)) {
> > > +		os_free(data.new_psk);

> This is the traceback from the tool:
> 
> wps_er.c:2040: Dynamic memory stored in 'data.new_psk' is allocated by calling function 'wps_build_cred'.

wps_er.c:2039 sets data.user_cred to a non-NULL value (neither of the
two callers of wps_er_config_token_from_cred() can use NULL as the cred
argument).

> wps_registrar.c:1686: wps->auth_type& (2|32) is true

this is within wps_build_cred() after these steps:

    if (wps->use_cred) {
	os_memcpy(&wps->cred, wps->use_cred, sizeof(wps->cred));
	goto use_provided;
    }

and before the use_provided label. There is no label in the middle
either, so no way to get back to line 1686. In other words, this code
path is not possible.

> -----Original Message-----
> From: Jouni Malinen [mailto:j@w1.fi]
> Sent: Monday, June 29, 2015 20:42
> To: Peer, Ilan
> Cc: Rosenfeld, Ben; hostap@lists.shmoo.com
> Subject: Re: [PATCH 5/5] WPS: Fix possible memory leak in
> wps_er_config_token_from_cred()
> 
> On Sun, Jun 21, 2015 at 01:56:45PM +0000, Peer, Ilan wrote:
> > > > In wps_er_config_token_from_cred() data.new_pak memory is
> > > > allocated in
> > > > wps_build_cred() and the function returns before the memroy is
> released.
> > >
> > > > diff --git a/src/wps/wps_er.c b/src/wps/wps_er.c @@ -2039,10
> > > > +2039,12 @@ struct wpabuf * wps_er_config_token_from_cred(struct
> > > > wps_context
> > > *wps,
> > > >  	data.use_cred = cred;
> > > >  	if (wps_build_cred(&data, ret) ||
> > > >  	    wps_build_wfa_ext(ret, 0, NULL, 0)) {
> > > > +		os_free(data.new_psk);
> 
> > This is the traceback from the tool:
> >
> > wps_er.c:2040: Dynamic memory stored in 'data.new_psk' is allocated by
> calling function 'wps_build_cred'.
> 
> wps_er.c:2039 sets data.user_cred to a non-NULL value (neither of the two
> callers of wps_er_config_token_from_cred() can use NULL as the cred
> argument).
> 
> > wps_registrar.c:1686: wps->auth_type& (2|32) is true
> 
> this is within wps_build_cred() after these steps:
> 
>     if (wps->use_cred) {
> 	os_memcpy(&wps->cred, wps->use_cred, sizeof(wps->cred));
> 	goto use_provided;
>     }
> 
> and before the use_provided label. There is no label in the middle either, so
> no way to get back to line 1686. In other words, this code path is not possible.
> 

Thanks for the clarification. Please drop this patch.

Ilan.