wpapcap2john fixes

[magnumripper]

After reading just the IDB, it could try to dump data not yet read from the pcap due to an uninitialized variable and sloppy code. When this happened, it would segfault.

Many other robustness fixes, added some comments, decreased variable scopes.

Now handles (deprecated) PB properly (without the confusing hack leading to #5471 and 939203c, which broke the support for it).
Also adds proper support for (legacy) SPB. They were just skipped prior to this, but I doubt they are used anywhere. I'm not aware of any tool that can produce these two type of blocks at all - any modern tool just go with EPB.

Closes #5938

[solardiz]

I skimmed these commits one by one, then all at once. Saw nothing obviously wrong, except that we're still not checking returns from fseek.

Note that the removed duplicated read/seek block wasn't benign - we're actually changing behavior by removing it now, because the second read's offset was different - but looks like the old behavior was more likely wrong?

Sorry that I seem to have broken something with my referenced commit. Re-reading the comments I wrote at the time, I was unsure about much of this, but was somehow sure about which caplen to use? So was my fix completely wrong or partially wrong (so partially still needed)?

[solardiz]

Previously, we'd print a message when the return is non-zero but also not BH_SIZE, now we'll be silent or print just "End of data" if at high verbosity. Is this right?

[magnumripper]

No. I changed it during my confusion between fseek and fread behaviour - got the idea we'd either get 0 or -1 here. I will change this back!

[solardiz]

You'll probably also want to have the error message go to stderr.

[magnumripper]

Ouch, thanks!

[magnumripper]

I skimmed these commits one by one, then all at once. Saw nothing obviously wrong, except that we're still not checking returns from fseek.

I added checking everywhere with an "fseek_chk" macro, but decided to remove it because I got confused and thought we'd get an error for truncated files (didn't risk changing behavior for that, truncated pcap file are very common). Thinking again and verifying with the man page, this is not the case, right? I'll put it back, it's easy. If nothing else it could reveal bugs.

Note that the removed duplicated read/seek block wasn't benign - we're actually changing behavior by removing it now, because the second read's offset was different - but looks like the old behavior was more likely wrong?

The old was definitely wrong, it would skip every other packet.

Sorry that I seem to have broken something with my referenced commit. Re-reading the comments I wrote at the time, I was unsure about much of this, but was somehow sure about which caplen to use? So was my fix completely wrong or partially wrong (so partially still needed)?

Due to the dupe read it was broken regardless. Also, looking again I think my intention was fine (piggy-back epb to simplify code) but it looks like it wasn't done properly anyway. Not to mention how confusing that was even for myself now. Anyway it's all good after these fixes I'm pretty sure.

[magnumripper]

Done. I did a fix-up for both things into the "right" commit but here's the changes from what you reviewed:

$ git diff aa9b70d..e34e5da

diff --git a/src/wpapcap2john.c b/src/wpapcap2john.c
index 819ac219b..12b1fa877 100644
--- a/src/wpapcap2john.c
+++ b/src/wpapcap2john.c
@@ -94,6 +94,15 @@ static const char* const ctl_subtype[16] = {
 	"Subtype 15"
 };
 
+#define fseek_chk(s, o, w)	  \
+	do { \
+		if (fseek(s, o, w) == -1) { \
+			fprintf(stderr, "%s: Seek failed: %s, %s:%u\n", \
+			        filename, strerror(errno), __FILE__, __LINE__); \
+			exit(1); \
+		} \
+	} while (0);
+
 #if HAVE___MINGW_ALIGNED_MALLOC
 char *strdup_MSVC(const char *str)
 {
@@ -225,9 +234,9 @@ static int convert_ivs2(FILE *f_in)
 	unsigned char *p, *w;
 	int ess = -1;
 
-	fseek(f_in, 0, SEEK_END);
+	fseek_chk(f_in, 0, SEEK_END);
 	length = ftell(f_in);
-	fseek(f_in, 0, SEEK_SET);
+	fseek_chk(f_in, 0, SEEK_SET);
 
 	safe_malloc(ivs_buf, length);
 
@@ -1878,7 +1887,7 @@ void pcapng_option_walk(FILE *in, uint32_t tl)
 				break;
 		} else {
 			// Just skip unknown options
-			fseek(in, pad_len, SEEK_CUR);
+			fseek_chk(in, pad_len, SEEK_CUR);
 		}
 	}
 }
@@ -1892,8 +1901,15 @@ static int process_ng(FILE *in)
 		block_header_t pcapng_bh;
 		interface_description_block_t pcapng_idb;
 
-		if ((res = fread(&pcapng_bh, 1, BH_SIZE, in)) != BH_SIZE)
+		res = fread(&pcapng_bh, 1, BH_SIZE, in);
+		if (res == 0) {
+			break;
+		}
+		if (res != BH_SIZE) {
+			printf("%s: Failed to read pcap-ng header block: %s\n",
+			       filename, strerror(errno));
 			break;
+		}
 
 		if (verbosity >= 4)
 			fprintf(stderr, "pcap-ng block type %u\n", pcapng_bh.block_type);
@@ -1928,7 +1944,7 @@ static int process_ng(FILE *in)
 
 			memset(&pcapng_idb, 0, sizeof(pcapng_idb));
 
-			fseek(in, aktseek + pcapng_bh.total_length - BH_SIZE - SHB_SIZE, SEEK_SET);
+			fseek_chk(in, aktseek + pcapng_bh.total_length - BH_SIZE - SHB_SIZE, SEEK_SET);
 		}
 
 		/* IDB, Interface Description Block */
@@ -1944,7 +1960,7 @@ static int process_ng(FILE *in)
 				pcapng_idb.snaplen  = swap32u(pcapng_idb.snaplen);
 			}
 
-			fseek(in, pcapng_bh.total_length - BH_SIZE - IDB_SIZE, SEEK_CUR);
+			fseek_chk(in, pcapng_bh.total_length - BH_SIZE - IDB_SIZE, SEEK_CUR);
 		}
 
 		/* PB, Packet Block (deprecated) */
@@ -1980,7 +1996,7 @@ static int process_ng(FILE *in)
 				        filename, feof(in) ? "EOF" : strerror(errno));
 				break;
 			}
-			fseek(in, pcapng_bh.total_length - BH_SIZE - PB_SIZE - pcapng_pb.caplen, SEEK_CUR);
+			fseek_chk(in, pcapng_bh.total_length - BH_SIZE - PB_SIZE - pcapng_pb.caplen, SEEK_CUR);
 
 			if (pcapng_pb.caplen > 0) {
 				snap_len = pcapng_pb.caplen;
@@ -2017,7 +2033,7 @@ static int process_ng(FILE *in)
 				        filename, feof(in) ? "EOF" : strerror(errno));
 				break;
 			}
-			fseek(in, pcapng_bh.total_length - BH_SIZE - SPB_SIZE - pcapng_spb.len, SEEK_CUR);
+			fseek_chk(in, pcapng_bh.total_length - BH_SIZE - SPB_SIZE - pcapng_spb.len, SEEK_CUR);
 
 			if (pcapng_spb.len > 0) {
 				snap_len = pcapng_spb.len;
@@ -2055,7 +2071,7 @@ static int process_ng(FILE *in)
 				        filename, feof(in) ? "EOF" : strerror(errno));
 				break;
 			}
-			fseek(in, pcapng_bh.total_length - BH_SIZE - EPB_SIZE - pcapng_epb.caplen, SEEK_CUR);
+			fseek_chk(in, pcapng_bh.total_length - BH_SIZE - EPB_SIZE - pcapng_epb.caplen, SEEK_CUR);
 
 			if (pcapng_epb.caplen > 0) {
 				snap_len = pcapng_epb.caplen;
@@ -2076,7 +2092,7 @@ static int process_ng(FILE *in)
 			if (verbosity >= 4)
 				fprintf(stderr, "Skipping (unhandled block type)\n");
 
-			fseek(in, pcapng_bh.total_length - BH_SIZE, SEEK_CUR);
+			fseek_chk(in, pcapng_bh.total_length - BH_SIZE, SEEK_CUR);
 		}
 	}
 
@@ -2149,7 +2165,7 @@ static int process(FILE *in)
 	else if (main_hdr.magic_number == 0xd4c3b2a1)
 		swap_needed = 1;
 	else if (main_hdr.magic_number == PCAPNGBLOCKTYPE) {
-		fseek(in, 0, SEEK_SET);
+		fseek_chk(in, 0, SEEK_SET);
 		return process_ng(in);
 	} else {
 		if (convert_ivs2(in)) {