rpcap: compare against the new authentication reply size

[paolo364]

rpcap_doauth() validates the payload length of an authentication reply against the two possible reply layouts:

struct rpcap_authreply_old  - 2 bytes (minvers, maxvers)
struct rpcap_authreply      - 8 bytes (adds padding and byte_order_magic)

The chain of tests is:

if (plen < sizeof(struct rpcap_authreply_old))        /* < 2  */
        ... too short, fail
if (plen == sizeof(struct rpcap_authreply_old))       /* == 2 */
        reply_len = sizeof(struct rpcap_authreply_old);
else if (plen >= sizeof(struct rpcap_authreply_old))  /* >= 2 */
        reply_len = sizeof(struct rpcap_authreply);
else
        ... "Too long for old reply, too short for new reply", fail

By the time the third test is reached, plen > sizeof(struct rpcap_authreply_old) is already known, so the test is always true and the final else is unreachable. Its comment describes the case it was meant to catch - a reply longer than the old layout but shorter than the new one, i.e. 3 to 7 bytes - which currently takes the "read the full new reply" branch instead.

Comparing against sizeof(struct rpcap_authreply) makes the last branch reachable and restores the intended check.

No security impact: rpcap_recv() already refuses to read more than the declared payload (if (toread > *plen)), so a 3..7 byte reply is rejected either way, currently with "Message payload is too short" rather than the more specific message. This is a correctness and diagnostics fix, which is why it is sent as an ordinary pull request rather than through the private security reporting process.

[infrastation]

Thank you, at the branching level this logic is clear. One thing that would need verifying before merging this is whether this change unmasks another dormant bug — remote capture code tends to have bugs.

[paolo364]

Thanks for looking at it.

I don't think this can unmask anything downstream, because it doesn't turn a rejected reply into an accepted one — it only changes which error path a rejected reply takes. Going through the cases:

• plen < 2 — first branch, unchanged.
• plen == 2 — old layout, unchanged.
• plen >= 8 — >= sizeof(struct rpcap_authreply_old) and >= sizeof(struct rpcap_authreply) are both true, so reply_len is sizeof(struct rpcap_authreply) before and after. Unchanged, including the case of a reply longer than 8 bytes, where rpcap_recv() discards the excess as it does today.
• plen in 3..7 — the only class whose behaviour changes. Before: reply_len = sizeof(struct rpcap_authreply), then rpcap_recv() fails because toread > *plen, giving "Message payload is too short", followed by rpcap_discard(sockctrl, ssl, plen, NULL) and return -1. After: the final else, giving "Authentication reply from server is too short", followed by the same rpcap_discard(sockctrl, ssl, plen, NULL) and return -1.

So before and after, a 3..7 byte reply ends the same way: errbuf set, payload discarded, -1 returned. No new code becomes reachable on a success path, and the error path that does become reachable mirrors the two that were already there — same discard, same return value.

Happy to back this with a stub server replying with 0, 1, 2, 3, 5, 7, 8 and 12 bytes and show identical caller-visible behaviour before and after, if you'd like that on the record before merging.

[guyharris]

There are three possible valid reply sizes:

1. 0 - the original size, in which the reply neither indicates the minimum and maximum protocol versions supported by the server nor the byte order of the server, meaning that the minimum and maximum protocol versions are 0, the only current protocol version (as it's an old server, and no new protocol versions have been introduced yet, so there are no new versions to support) and the byte order is unknown.
2. sizeof(struct rpcap_authreply_old) - the size after I introduced the version information but before I introduced the byte order, in which case the minimum and maximum protocol version are known but the byte order isn't known.
3. sizeof(struct rpcap_authreply) - the current size, in which the reply indicates both;

In db93927, in late January 2019, I added protocol version negotiation. That's case 2 above.

In dc14a7b, in early August 2022, I added the byte order. That's case 3 above.

The if (plen != 0) checks whether this is case 1. Inside that if is code to handle cases other than 1:

• the if (plen < sizeof(struct rpcap_authreply_old) test checks whether it's too short to be cases 2 or 3, and rejects the reply if so;
• the if (plen == sizeof(struct rpcap_authreply_old)) test checks whether it's case 2 rather than either case 3 or another invalid reply, and extracts the minimum and maximum version and sets has_byte_order to 0 (meaning false - we should make it a bool in the main branch);
• the else if (plen >= sizeof(struct rpcap_authreply_old)) test is intended to check whether it's a valid case 3 reply (we allow additional stuff after that, in case we do a further extension to add more information in the authentication reply - we just ignore that);
• the else after that handles "too large to be case 2 but too small to be case 3", rejecting the reply if so.

The else if (plen >= sizeof(struct rpcap_authreply_old)) test should be else if (plen >= sizeof(struct rpcap_authreply)), so that something too large to be case 2 but too small to be case 3 isn't accepted but something large enough to be case 3 is accepted, so your change in this pull request is correct.

[guyharris]

I'm currently horsewhipping Appveyor into getting past the failures to connect when trying to download tools, to which it's unfortunately prone (it appears to be going through a particularly bad time of that right now).