Re: [PATCH v2] nslookup: fix out-of-bounds reads in TXT and SOA record parsing

Roberto,

I read 9bd9b414a since you pointed at it as covering more cases. The
TXT hunk matches mine, but I do not think the SOA rewrite is safe to
carry:

	case ns_t_soa:
		const unsigned char *ip;
		static const char *prtstr[] = {
			"serial", "refresh", "retry", "expire", "minimum"
		};
		...
		for(i = 0, ip += rdlen; cp < ip; cp += 4, i++)
			printf("\t%s = %lu\n", prtstr[i], ns_get32(cp));

- ip is never assigned. The only write to it is "ip += rdlen" in the
  loop header, which reads an indeterminate value. Depending on what
  that value is, the loop either does not run at all, so the five
  fields silently vanish from the output, or it runs until cp catches
  up with a garbage bound.

- i is the counter of the enclosing answer-section loop:

	for (i = 0; i < ns_msg_count(handle, ns_s_an); i++)

  Resetting it inside the case makes the outer loop skip the records
  between 0 and the number of fields printed, and changes what
  parse_reply() returns.

- prtstr[] has five entries, but the loop bound comes from rdlen, not
  from 5. Even with ip initialized to ns_rr_rdata(rr), rdlen is a
  uint16 and both names can be two-byte compression pointers, so the
  loop can run thousands of times and hand printf("%s") a pointer read
  from past the end of the array.

- "cp < ip" together with "cp += 4" still enters the body with one to
  three bytes left, so the final ns_get32() reads past RDATA anyway.

I built it here with busybox's own flags and it produces no
diagnostic, which is the awkward part: a normal build does not catch
any of this.

The SOA case only needs the length check to happen after the two
variable-length names are consumed, since rdlen >= 20 is currently
tested before they are parsed. That is the second hunk of my v2, and
it is three lines.

Ali