reject too-short PROXY v2 address length in mod_remoteip

[arshsmith1]

remoteip_process_v2_header only bounds the advertised v2 header length from above, so a peer sending a TCPv4 or TCPv6 family with a length shorter than its address block makes the code read past what was actually received from the uninitialized ctx->header buffer and adopt those bytes as the client IP used for logging and Require ip decisions. Reject the header when ntohs(hdr->v2.len) is smaller than the family address struct, matching the existing too-long check right beside it.

[notroj]

Please use APLOGNO() and let a maintainer fill in the next number as appropriate to avoid conflicts.

[arshsmith1]

Done, both are bare APLOGNO() now and I dropped the next-number bump so the file isn't touched at all.

Your concern was well founded, by the way: trunk has moved to 10597 since I opened this, and 10596 is now taken by mod_cern_meta.c, so my hardcoded number would have collided exactly as you describe.

[notroj]

As above.

[arshsmith1]

Same here, this one's bare APLOGNO() too.

[notroj]

Thanks, sorry a couple more bits of feedback -

1. remoteip_get_v2_len should probably be used here for consistency rather than the ntohs(...)
2. would the size comparison not be better as a != rather than < - is there a case where it could be validly longer? I'm not familiar with the PROXY protocol but from a quick scan it looks like those records are fixed length

[arshsmith1]

Both done in bfcf178. Swapped the two checks over to remoteip_get_v2_len and moved that helper above remoteip_process_v2_header so it's in scope; the log format is now APR_SIZE_T_FMT to match the return type (same idiom as the need=/rcvd= message further down).

On >= vs ==: the records are fixed length, but the v2 len field covers the address block plus any TLVs that follow it, so a longer value is legitimate. Spec section 2.2 says a receiver must use the length to skip the extra bytes it doesn't understand rather than reject them, and the block right after this already handles that (the too-long gate only rejects past sizeof(proxy_v2), and the parser just skips the remainder). So == would break any sender that appends TLVs, e.g. haproxy's PP2_TYPE_SSL or authority. The only unsafe case is len being smaller than the address struct we're about to read out of the buffer, which is what < catches.

[notroj]

Makes sense, thanks a lot.