Wrong usage of pcapngepb and pcapngpb in wpapcap2john.c

[lucic71]

While running some static code analysis with clang-tidy over the john code-base I saw a dubious use of pcapngepb: https://github.com/openwall/john/blob/1c97519d09500ffcd24fd8d56541c1d3cf10ea87/src/wpapcap2john.c#L1983

It's dubious because pcapngepb is used uninitialized if this branch is taken: https://github.com/openwall/john/blob/1c97519d09500ffcd24fd8d56541c1d3cf10ea87/src/wpapcap2john.c#L1960

Instead, I think at line 1983 pcapngpb should be used instead of pcapngepb. pcapngepb is only initialized on this branch: https://github.com/openwall/john/blob/1c97519d09500ffcd24fd8d56541c1d3cf10ea87/src/wpapcap2john.c#L2021

I think naming is the problem here because the two names differ by one letter.

[claudioandre-br]

Or there is something wrong in the source code and no tests with pcapngbh.block_type == 2 were run

Or

There is another possibility:

• if pcapngbh.block_type == 6 is read from the file before pcapngbh.block_type == 2, the data will be filled in correctly (in the correct order) and there is no logic problem in the program (which seems more likely if some real testing for the problematic branch was executed, see line 1990).

What can we do:

• silence the warning using initialization for the structure in question;
• find a sample that allows us to properly test the 2john tool. If it's wrong, it's very wrong

[lucic71]

I think the easiest is to initialize the struct. If your hypotheses is true (block_type 6 is executed before block_type 2) then at least the struct is 0 initialized and no undefined behavior is triggered.

[solardiz]

Reviewing the code, we appear to have the same issue also for pcapngepb.caplen, which I guess the static analyzer missed.

All of this code was added in just one commit by @magnumripper in 2018.

Unfortunately, none of the samples we have in https://github.com/openwall/john-samples/tree/main/WPA-PSK trigger these code paths for me. https://wiki.wireshark.org/SampleCaptures has some pcapng files, but not for WPA. Maybe @magnumripper has a sample for us to test this with?

I agree that initializing the struct is the safest bet - will definitely not make things worse - but it may also paper over the bug.

[solardiz]

OK, I just did:

tshark -r wpa-Induction.pcap -w wpa-Induction.pcapng

and got with file:

wpa-Induction.pcap:   pcap capture file, microsecond ts (little-endian) - version 2.4 (802.11 with radiotap header, capture length 65535)
wpa-Induction.pcapng: pcapng capture file - version 1.0

Running wpapcap2john on these files produces the same output (except for the different embedded filenames). On the pcapng file, the type 6 path is reached many times, but type 2 is never reached. So unfortunately this tells us nothing about correctness of the type 2 code.

[solardiz]

Reviewing the code, we appear to have the same issue also for pcapngepb.caplen

For this one, the code was obviously wrong. So I'm fixing it to use the current block's fields. But I have nothing to test it with, and it may well be broken in other ways as well.