Radius improvements

[jkroonza]

No description provided.

[jkroonza]

I probably missed stuff that is affected by the change from data[2] to data[0] - so please check me as if I'm an idiot here. Just to be clear: nothing is broken - pointless warnings in logs, accounting does actually work. Spotted the under-run whilst looking to verify the use of the data[] array in the struct.

[Neustradamus]

@paulusmack: What do you think about this @jkroonza PR?

[paulusmack]

@Neustradamus I think I'm traveling and also that I don't need to be continually poked.

[Neustradamus]

@paulusmack: Ok no problem ^^

[paulusmack]

I probably missed stuff that is affected by the change from data[2] to data[0] - so please check me as if I'm an idiot here. Just to be clear: nothing is broken - pointless warnings in logs, accounting does actually work. Spotted the under-run whilst looking to verify the use of the data[] array in the struct.

I didn't see anything relating to the change from data[2] to data[0] that you missed. I haven't been able to convince myself that the check at line 460 of sendserver.c (totallen + secretlen vs. bufferlen) is unnecessary, though. The comment above it related to when the code had a check for totallen < 4096 earlier on, but now we don't have that check, totallen could conceivably be as large as 8192, so I think the check is still needed.

[jkroonza]

I enabled MSG_TRUNC on the recvfrom in rc_send_server (which really aught to not be receive as well - meaning we can possibly axe the blocking nature of the plugin by splitting this into send_server and receive_server. But I think that's too big a change here.

Directly after I added a check for packet length > sizeof(recv_buffer), which would detect "truncated receives" and error on that immediately and return, meaning that in the case of length > BUFFER_LEN we won't even call rc_check_reply. Thus the added comment that datalen will always be <= buffer size.

We thus just need to verify that the length as specified in the received datagram actually matches the length of the received datagram. Anything else is indicative of corruption or tampering. The specified length in the datagram as far as I could determine must include the secretlen. Don't recall which RFCs I looked at right now. Regardless, when calling rc_check_reply we're guaranteed that datalen <= bufferlen. There we verify that datalen is >= AUTH_HDR size (20), before extracting what the packet believes the length to be, and then checking that totallen == datalen (<= buffersize).

However, I see now how to checksum is calculated, that's messy. Let me double-check that. I know OpenSSL MD5 library we can add smaller chunks so we don't have to get everything into a linear buffer then either, but it will add an OpenSSL dependency always when using radius, even if we don't need it otherwise. That or the MD5 code needs to be re-worked to maintain state similar to what OpenSSL does, basically initialise the (IIRC) 64-byte mixing block, then mix in until we filled the buffer, perform a round and then keep computing, and then a finalise round to seal the thing. That might well end up being cleaner than this memcpy() mess to linearise the input. The problem sits on this line here:

478         memcpy ((char *) auth + totallen, secret, secretlen);

If we could avoid that memcpy() we wouldn't need the check, but as it stands you're absolutely right. Will see what I can do about this.

Note (edited): rc_md5_calc() uses PPP_DigestInit() + PPP_DigestUpdate() + PPP_DigestFinal() underneath, so we can just invoke that directly with two DigestUpdate() calls instead of the wrapped up way that adds the constraints.

The change for data[2] to data[0] is in the radiusclient.h header, which aligns the absolute minimum packet size (20) with that of the structure now. Which in my opinion is how it should have been in the first place. data[0] is thus the first TAG and data[1] the first length. Both of which overruns data[0], but since it's just a special way to indicate that we've got a dynamic tail-array we honestly could not care about that since I've also adjusted avpair.c to check that we have at least two bytes in the remaining data buffer before accessing them (which was previously not done, which meant that we could potentially have comsumed TL bytes with no VALUE, or possibly even end up with a zero-length VALUE for some TAG, that's sorted now too).

[jkroonza]

scope creep ... @paulusmack this should be better overall. I think the next push will (should) be a move towards radius-ng.

[paulusmack]

one thing doesn't seem right...