Patchwork Reset the ssid in wps before being replaced with the one in credential.

login
register
mail settings
Submitter jungwalk@gmail.com
Date Dec. 9, 2011, 5:10 a.m.
Message ID <1323407410-25072-1-git-send-email-jungwalk@gmail.com>
Download mbox | patch
Permalink /patch/130281/
State Rejected
Headers show

Comments

jungwalk@gmail.com - Dec. 9, 2011, 5:10 a.m.
From: Spencer Chang <jungwalk@gmail.com>

It is better to reset the ssid in WPS first before it is replaced with the one in
credential in case the length of ssid in credential is longer than wps->ssid.

Signed-off-by: Spencer Chang <jungwalk@gmail.com>
---
 src/ap/wps_hostapd.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
Johannes Berg - Dec. 9, 2011, 11:34 a.m.
On Fri, 2011-12-09 at 13:10 +0800, jungwalk@gmail.com wrote:
> From: Spencer Chang <jungwalk@gmail.com>
> 
> It is better to reset the ssid in WPS first before it is replaced with the one in
> credential in case the length of ssid in credential is longer than wps->ssid.

> +	os_memset(hapd->wps->ssid, 0, HOSTAPD_MAX_SSID_LEN);
>  	os_memcpy(hapd->wps->ssid, cred->ssid, cred->ssid_len);
>  	hapd->wps->ssid_len = cred->ssid_len;

That seems completely useless, in particular when it's longer since then
it will partially overwrite it anyway. They are binary data, not
strings, after all.

johannes
jungwalk@gmail.com - Dec. 9, 2011, 1:31 p.m.
Oops, typo.

What I wanted to say is "in case the length of ssid in wps->ssid is longer
than the one in credential".
Then, does this make more sense?
If it is, I will resend the patch.

2011/12/9 Johannes Berg <johannes@sipsolutions.net>

> On Fri, 2011-12-09 at 13:10 +0800, jungwalk@gmail.com wrote:
> > From: Spencer Chang <jungwalk@gmail.com>
> >
> > It is better to reset the ssid in WPS first before it is replaced with
> the one in
> > credential in case the length of ssid in credential is longer than
> wps->ssid.
>
> > +     os_memset(hapd->wps->ssid, 0, HOSTAPD_MAX_SSID_LEN);
> >       os_memcpy(hapd->wps->ssid, cred->ssid, cred->ssid_len);
> >       hapd->wps->ssid_len = cred->ssid_len;
>
> That seems completely useless, in particular when it's longer since then
> it will partially overwrite it anyway. They are binary data, not
> strings, after all.
>
> johannes
>
>
Johannes Berg - Dec. 9, 2011, 1:34 p.m.
On Fri, 2011-12-09 at 21:31 +0800, 張晏榕 wrote:
> Oops, typo.
> 
> What I wanted to say is "in case the length of ssid in wps->ssid is
> longer than the one in credential".
> Then, does this make more sense?
> If it is, I will resend the patch.

Even then, it doesn't seem to make a lot of sense, since not setting it
to 0 will only cause some old data to be left in an unused portion of
the memory ...

johannes
Jouni Malinen - Dec. 9, 2011, 4 p.m.
On Fri, Dec 09, 2011 at 01:10:10PM +0800, jungwalk@gmail.com wrote:
> It is better to reset the ssid in WPS first before it is replaced with the one in
> credential in case the length of ssid in credential is longer than wps->ssid.

Are you trying to fix a bug that addresses some noticeable issue?

> @@ -318,6 +318,7 @@ static int hapd_wps_cred_cb(struct hostapd_data *hapd, void *ctx)
> +	os_memset(hapd->wps->ssid, 0, HOSTAPD_MAX_SSID_LEN);
>  	os_memcpy(hapd->wps->ssid, cred->ssid, cred->ssid_len);
>  	hapd->wps->ssid_len = cred->ssid_len;

Like Johannes pointed out, this should not really result in any real
change in actual behavior. If it does, there is likely a real bug
somewhere else and that should be fixed. The SSID buffer is used as an
arbitrary binary array of ssid_len octets and as such, there is no need
to null-terminate it - the extra octets in the buffer are ignored.
jungwalk@gmail.com - Dec. 9, 2011, 9:50 p.m.
Hi Jouni and Johannes,

Yes, I am trying to fix a bug related to generating random WPS ssid for
each band, and thought this kind of change maybe necessary even without
that random WPS ssid modification.
However, I seems to misunderstand how to use that SSID buffer. I will check
it more.
Thanks for your responses. :)



2011/12/10 Jouni Malinen <j@w1.fi>

> On Fri, Dec 09, 2011 at 01:10:10PM +0800, jungwalk@gmail.com wrote:
> > It is better to reset the ssid in WPS first before it is replaced with
> the one in
> > credential in case the length of ssid in credential is longer than
> wps->ssid.
>
> Are you trying to fix a bug that addresses some noticeable issue?
>
> > @@ -318,6 +318,7 @@ static int hapd_wps_cred_cb(struct hostapd_data
> *hapd, void *ctx)
> > +     os_memset(hapd->wps->ssid, 0, HOSTAPD_MAX_SSID_LEN);
> >       os_memcpy(hapd->wps->ssid, cred->ssid, cred->ssid_len);
> >       hapd->wps->ssid_len = cred->ssid_len;
>
> Like Johannes pointed out, this should not really result in any real
> change in actual behavior. If it does, there is likely a real bug
> somewhere else and that should be fixed. The SSID buffer is used as an
> arbitrary binary array of ssid_len octets and as such, there is no need
> to null-terminate it - the extra octets in the buffer are ignored.
>
> --
> Jouni Malinen                                            PGP id EFC895FA
> _______________________________________________
> HostAP mailing list
> HostAP@lists.shmoo.com
> http://lists.shmoo.com/mailman/listinfo/hostap
>
Jouni Malinen - Dec. 9, 2011, 11:49 p.m.
On Sat, Dec 10, 2011 at 05:50:46AM +0800, 張晏榕 wrote:
> Yes, I am trying to fix a bug related to generating random WPS ssid for
> each band, and thought this kind of change maybe necessary even without
> that random WPS ssid modification.

Could you please describe that in more detail? That sounds more like a
misunderstanding of how hostapd is supposed to work since hostapd does
not generate a random SSID. It does generate a random passphrase when
moving from unconfigured to configured state, but the initial SSID is
assumed to be generated by something external.
jungwalk@gmail.com - Dec. 10, 2011, 2 a.m.
Hi Jouni,

Sorry for misleading.
That code of generating random SSID is added by myself for customer's
request, and I misunderstood how the SSID buffer should be used. Therefore,
I created an issue of wrong SSID on dual band product and that issue could
be fixed with that patch of resetting SSID buffer.
And then I thought may be the SSID buffer should be reset even in original
hostapd code (without random SSID generation). But I am wrong.
After you and Johannes described, I know the root cause of that wrong SSID
issue I created and it should be fixed by following the right way of using
SSID buffer, and it is not necessary to reset the SSID buffer.

Thanks for your responses and sorry for misleading again.

2011/12/10 Jouni Malinen <j@w1.fi>

> On Sat, Dec 10, 2011 at 05:50:46AM +0800, 張晏榕 wrote:
> > Yes, I am trying to fix a bug related to generating random WPS ssid for
> > each band, and thought this kind of change maybe necessary even without
> > that random WPS ssid modification.
>
> Could you please describe that in more detail? That sounds more like a
> misunderstanding of how hostapd is supposed to work since hostapd does
> not generate a random SSID. It does generate a random passphrase when
> moving from unconfigured to configured state, but the initial SSID is
> assumed to be generated by something external.
>
> --
> Jouni Malinen                                            PGP id EFC895FA
> _______________________________________________
> HostAP mailing list
> HostAP@lists.shmoo.com
> http://lists.shmoo.com/mailman/listinfo/hostap
>

Patch

diff --git a/src/ap/wps_hostapd.c b/src/ap/wps_hostapd.c
index 817012e..42b22a2 100644
--- a/src/ap/wps_hostapd.c
+++ b/src/ap/wps_hostapd.c
@@ -318,6 +318,7 @@  static int hapd_wps_cred_cb(struct hostapd_data *hapd, void *ctx)
 	if (hapd->conf->wps_cred_processing == 1)
 		return 0;
 
+	os_memset(hapd->wps->ssid, 0, HOSTAPD_MAX_SSID_LEN);
 	os_memcpy(hapd->wps->ssid, cred->ssid, cred->ssid_len);
 	hapd->wps->ssid_len = cred->ssid_len;
 	hapd->wps->encr_types = cred->encr_type;