[Pablo Martin-Gomez <pmartin-gomez@xxxxxxxxxx>]

Hello, 

My comments are in-lined in your mail/patch. (Open my original mail in text mode, it will be easier to find them.)

Best regards,

Pablo MG

Le ven. 11 juil. 2025, 21:52, Alex Gavin <a_gavin@xxxxxxxxxx> a écrit :

On 7/11/25 11:34, Pablo MARTIN-GOMEZ wrote:
> Hi,
>
> There is two big functional and a 802.11 issues in your patch. Fix those and you should having a
> functioning patch.

I understand that the contribution process requires submission through merge request on GitLab.
Possibly it is not clear, but I posted this WIP patch to seek review for an issue I'm running into
while implementing the nl80211 cipher suites dissector.

Details are in the commit message below in addition to some 'TODOs' that I intend to resolve. Aside
from the functional issue I pointed in the commit message and inline comments, it's not clear what
issues you're referencing.

One additional item I'm seeking feedback on, aside from patch functionality, is 'AKMS_*' definitions
location. They are presently in the 'packet-ieee80211.c' dissector. I'd like to move them to the
'packet-ieee80211.h' file which is already included by the nl80211 dissector.
> On 11/07/2025 00:56, Alex Gavin via Wireshark-dev wrote:
>> Presently this dissection only properly dissects one
>> cipher suite of the several that are in the message.
>>
>> Implementation inspired by dissection in 'ieee80211_tag_ext_supp_rates()'
>> in epan/dissectors/packet-ieee80211.c
>>
>> For context, each cipher suite is a u32 in a byte array. When
>> running 'iw phy0 info' in my system, the byte array is 44 bytes
>> total (the full NL80211_ATTR_CIPHER_SUITES is 48 bytes
>> including length and attribute, both 2 bytes each).
>>
>> For this attribute, expected parsing should grab u32 (4 byte)
>> chunks of the byte array, 4 bytes at a time. However, with this
>> patch, the first cipher suite successfully printed 'WPA (0x000fac01)',
>> but I get the following error:
>>
>> [Expert Info (Warning/Malformed): Trying to fetch an unsigned integer with length 44]
>>
>> Seemingly, this implementation is grabbing the full length of the
>> attribute, rather than the 4 bytes as configured in both the call to
>> 'proto_tree_add_item()' and associated header field definition.
>>
>> Steps to reproduce:
>> 1. sudo ip link add nlmon0 type nlmon
>> 2. sudo ip link set up dev nlmon0
>> 3. Run Wireshark w/ this patch
>> 4. iw phy0 info
>> 5. Filter for 'nl80211.attr_type == 57' (NL80211_ATTR_CIPHER_SUITES)
>> ---
>>   epan/dissectors/packet-netlink-nl80211.c | 78 ++++++++++++++++++++++++
>>   1 file changed, 78 insertions(+)
>>
>> diff --git a/epan/dissectors/packet-netlink-nl80211.c b/epan/dissectors/packet-netlink-nl80211.c
>> index 9e7fac5a31..f03dd7f008 100644
>> --- a/epan/dissectors/packet-netlink-nl80211.c
>> +++ b/epan/dissectors/packet-netlink-nl80211.c
>> @@ -27,6 +27,55 @@ typedef struct  {
>>   static dissector_handle_t ieee80211_handle;
>>   static dissector_table_t ieee80211_tag_dissector_table;
>>   +// TODO: These are defined in packet-ieee80211.c
>> +#define AKMS_NONE                 0x000FAC00
>> +#define AKMS_WPA                  0x000FAC01
>> +#define AKMS_PSK                  0x000FAC02
>> +#define AKMS_FT_IEEE802_1X        0x000FAC03
>> +#define AKMS_FT_PSK               0x000FAC04
>> +#define AKMS_WPA_SHA256           0x000FAC05
>> +#define AKMS_PSK_SHA256           0x000FAC06
>> +#define AKMS_TDLS                 0x000FAC07
>> +#define AKMS_SAE                  0x000FAC08
>> +#define AKMS_FT_SAE               0x000FAC09
>> +#define AKMS_AP_PEER_KEY          0x000FAC0A
>> +#define AKMS_WPA_SHA256_SUITEB    0x000FAC0B
>> +#define AKMS_WPA_SHA384_SUITEB    0x000FAC0C
>> +#define AKMS_FT_IEEE802_1X_SHA384 0x000FAC0D
>> +#define AKMS_FILS_SHA256          0x000FAC0E
>> +#define AKMS_FILS_SHA384          0x000FAC0F
>> +#define AKMS_FT_FILS_SHA256       0x000FAC10
>> +#define AKMS_FT_FILS_SHA384       0x000FAC11
>> +#define AKMS_OWE                  0x000FAC12
>> +#define AKMS_SAE_GROUP_DEPEND     0x000FAC18
>> +#define AKMS_FT_SAE_GROUP_DEPEND  0x000FAC19
>> +
>> +static const value_string ws_nl80211_cipher_suites_vals[] = {
>> +    { AKMS_NONE,                 "NONE" },
>> +    { AKMS_WPA,                  "WPA" },
>> +    { AKMS_PSK,                  "PSK" },
>> +    { AKMS_FT_IEEE802_1X,        "FT IEEE802.1X" },
>> +    { AKMS_FT_PSK,               "FT PSK" },
>> +    { AKMS_WPA_SHA256,           "WPA SHA256" },
>> +    { AKMS_PSK_SHA256,           "PSK SHA256" },
>> +    { AKMS_TDLS,                 "TDLS" },
>> +    { AKMS_SAE,                  "SAE" },
>> +    { AKMS_FT_SAE,               "FT SAE"},
>> +    { AKMS_AP_PEER_KEY,          "AP PEER KEY" },
>> +    { AKMS_WPA_SHA256_SUITEB,    "WPA SHA256 SUITEB" },
>> +    { AKMS_WPA_SHA384_SUITEB,    "WPA SHA256 SUITEB" },
>> +    { AKMS_FT_IEEE802_1X_SHA384, "FT IEEE8021.X SHA384" },
>> +    { AKMS_FILS_SHA256,          "FILS SHA256" },
>> +    { AKMS_FILS_SHA384,          "FILS SHA384" },
>> +    { AKMS_FT_FILS_SHA256,       "FT FILS SHA256" },
>> +    { AKMS_FT_FILS_SHA384,       "FT FILS SHA384" },
>> +    { AKMS_OWE,                  "OWE" },
>> +    { AKMS_SAE_GROUP_DEPEND,     "SAE GROUP DEPEND" },
>> +    { AKMS_FT_SAE_GROUP_DEPEND,  "FT SAE GROUP DEPEND" },
>> +    { 0, NULL }
>> +};
>> +static value_string_ext ws_nl80211_cipher_suites_vals_ext =
>> VALUE_STRING_EXT_INIT(ws_nl80211_cipher_suites_vals);
> You're mixing AKMS and cipher suites. Check Table 9-149 in 802.11-2020 or ieee80211_rsn_cipher_vals
> in packet-ieee80211.c to get the cipher suites values.
>> +
>>   /* Extracted using tools/generate-nl80211-fields.py */
>>   /* Definitions from linux/nl80211.h {{{ */
>>   enum ws_nl80211_commands {
>> @@ -4170,10 +4219,12 @@ static int hf_nl80211_ifname;
>>   static int hf_nl80211_mac;
>>   static int hf_nl80211_alpha2;
>>   static int hf_nl80211_dbm;
>> +static int hf_nl80211_cipher_suites;
>>     static int ett_nl80211;
>>   static int ett_nl80211_frame;
>>   static int ett_nl80211_tag;
>> +static int ett_nl80211_cipher_suites;
>>     static int
>>   dissect_nl80211_generic(tvbuff_t *tvb, void *data _U_, struct packet_netlink_data *nl_data,
>> proto_tree *tree, int nla_type _U_, int offset, int len)
>> @@ -4419,6 +4470,24 @@ dissect_nl80211_sta_info(tvbuff_t *tvb, void *data, struct
>> packet_netlink_data *
>>       return offset;
>>   }
>>   +static int
>> +dissect_nl80211_cipher_suites(tvbuff_t *tvb, void *data _U_, struct packet_netlink_data *nl_data
>> _U_, proto_tree *tree, int nla_type _U_, int offset, int len)
>> +{
>> +    if (len < 4) {
>> +        // TODO: Error here, expect at least one u32
>> +        //expert_add_info_format(pinfo, field_data->item_tag_length, &ei_ieee80211_tag_length,
>> +        //                    "Tag length %u too short, must be greater than 0",
>> +        //                    tag_len);
>> +        return offset;
>> +    }
>> +
>> +    while (offset < len) {
>> +        proto_tree_add_item(tree, hf_nl80211_cipher_suites, tvb, offset, 4, ENC_LITTLE_ENDIAN);
>> +        offset += 1;
> You made a copy paste error here; the offset increment you want here is the size of a u32, so 4.
>> +    }
>> +
>> +    return offset;
>> +}
>>     static int
>>   dissect_nl80211_attrs(tvbuff_t *tvb, void *data, struct packet_netlink_data *nl_data, proto_tree
>> *tree, int nla_type, int offset, int len)
>> @@ -4472,6 +4541,7 @@ dissect_nl80211_attrs(tvbuff_t *tvb, void *data, struct packet_netlink_data
>> *nl_
>>           { WS_NL80211_ATTR_REG_TYPE, &hf_nl80211_reg_type, NULL, NULL },
>>           { WS_NL80211_ATTR_AUTH_TYPE, &hf_nl80211_auth_type, NULL, NULL },
>>           { WS_NL80211_ATTR_KEY_TYPE, &hf_nl80211_key_type, NULL, NULL },
>> +        { WS_NL80211_ATTR_CIPHER_SUITES, &hf_nl80211_cipher_suites, &ett_nl80211_cipher_suites,
>> dissect_nl80211_cipher_suites },
> This table is only for simple values that do not need a dissector. Parsing
> WS_NL80211_ATTR_CIPHER_SUITES is very specific; you'll have to handle it specifically inside the
> switch case of dissect_nl80211_attrs.
>>           { WS_NL80211_ATTR_USE_MFP, &hf_nl80211_mfp, NULL, NULL },
>>           { WS_NL80211_ATTR_PS_STATE, &hf_nl80211_ps_state, NULL, NULL },
>>           { WS_NL80211_ATTR_WIPHY_TX_POWER_SETTING, &hf_nl80211_tx_power_setting, NULL, NULL },
>> @@ -4618,6 +4688,13 @@ proto_register_netlink_nl80211(void)
>>                 FT_INT32, BASE_DEC, NULL, 0x00,
>>                 NULL, HFILL }
>>           },
>> +
>> +        // TODO: Add to script?
>> +        { &hf_nl80211_cipher_suites,
>> +            { "Cipher Suite", "nl80211.cipher_suite",
>> +              FT_UINT32, BASE_HEX | BASE_EXT_STRING, &ws_nl80211_cipher_suites_vals_ext, 0x0,
>> +              NULL, HFILL },
>> +        },
>>   /* Extracted using tools/generate-nl80211-fields.py */
>>   /* Definitions from linux/nl80211.h {{{ */
>>           { &hf_nl80211_commands,
>> @@ -5371,6 +5448,7 @@ proto_register_netlink_nl80211(void)
>>           &ett_nl80211,
>>           &ett_nl80211_frame,
>>           &ett_nl80211_tag,
>> +        &ett_nl80211_cipher_suites,
>>   /* Extracted using tools/generate-nl80211-fields.py */
>>   /* Definitions from linux/nl80211.h {{{ */
>>           &ett_nl80211_commands,
>
> BR
>
> Pablo MG
>