Wireshark · Wireshark-dev: Re: [Wireshark-dev] Wi-Fi Protected Setup: Patch for EAP and 802.11 dissectors

[Jeff Morriss <jeff.morriss@xxxxxxxxxxx>]

Hi,

Sorry for the the delay in reviewing your patch.

Could you change all of these _add_text()s:

> +                 wpsOpCode = tvb_get_guint8(tvb, offset);
> +                 switch(wpsOpCode)
> +                 {
> +                 case WFA_WSC_START:
> +                         proto_tree_add_text(eap_tree, tvb, offset,1,"OP Code: WSC_START");
> +                         break;
> + 

to use, say, add_item() instead?  That makes the fields filterable _and_ 
it'll save you a lot of code.  You just need a value_string for "wps Op 
Codes", an hf_ entry, and a single call to _add_item(). (In general, 
using _add_text() is a bad thing.  The fact that your patch doesn't add 
a single hf_ is proof of that.)

The 80211.c patch has a lot of hard coded values in it:

> +               case 0x1001:

that could/should be pulled out into #defines.

All of these bit fields:

> +                       i = g_snprintf(out_buff, SHORT_STR, "Authentication Type Flags");
> +                       if (data_length != 2)
> +                               i = g_snprintf(out_buff + i, SHORT_STR - i, ": wrong size of field!");
> +                       else {
> +                               u16 = tvb_get_ntohs(tag_tvb, tag_off);
> +                               i = g_snprintf(out_buff + i, SHORT_STR - i, " = 0x%04x (%s%s%s%s%s%s/)",
> +                                       u16,
> +                                       u16 & 0x0001 ? "/ Open " : "",
> +                                       u16 & 0x0002 ? "/ WPAPSK " : "",
> +                                       u16 & 0x0004 ? "/ Shared " : "",
> +                                       u16 & 0x0008 ? "/ WPA " : "",
> +                                       u16 & 0x0010 ? "/ WPA2 " : "",
> +                                       u16 & 0x0020 ? "/ WPA2PSK " : "");

could be pulled out into hf entries, too.  Of course you might want to 
put them in a sub-tree (so the individual lines for each bit are 
normally hidden).

Regards,
-Jeff

Philippe Teuwen wrote:
> Hello,
>
> Here is a proposal of patch to analyse Wi-Fi Protected Setup
> related messages, both in IEEE802.11 management frames and in EAP msgs.
>
> Wi-Fi Protected Setup?
> cf http://www.wi-fi.org/pressroom_overview.php?newsid=263
>
> The code was done by Jianping Jiang and myself prior to the public
> announcement of Wi-Fi Protected Setup and now I ported it to the
> snapshot version (20070131, I hope nothing changed drastically this week-end)
>
> I generated the diffs with svn.
> Prior to this patch I also removed some warnings when compiling
> packet-ieee80211.c, these changes are also visible alone in
> packet-ieee80211_nowarn.patch.gz (very small) but note that
> packet-ieee80211-eap_WPS.patch.gz is a cumulative patch as
> svn diff is always against the repository.
>
> I ran the fuzzer on a sample capture of a pairing in progress
> for a (long) while without getting any error.
>
> Thanks for your comments.
>
> Note that I'm not member of the wireshark-dev list so take care
> when you reply to this post, thanks!
>
> Philippe Teuwen
>
>
> ------------------------------------------------------------------------
>
> _______________________________________________
> Wireshark-dev mailing list
> Wireshark-dev@xxxxxxxxxxxxx
> http://www.wireshark.org/mailman/listinfo/wireshark-dev