From: Alan T. DeKok Date: Tue, 7 Feb 2023 13:54:45 +0000 (-0500) Subject: cleanups and more checks on corner cases X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=3e2957e0b87107d80e5ca0fa8cee4d8af8dcbf0d;p=thirdparty%2Ffreeradius-server.git cleanups and more checks on corner cases the body_xor() function just does xor, and relies on the caller to check / set / clear the header flags --- diff --git a/src/protocols/tacacs/base.c b/src/protocols/tacacs/base.c index b8955bcbb7d..0ed4fd07082 100644 --- a/src/protocols/tacacs/base.c +++ b/src/protocols/tacacs/base.c @@ -136,24 +136,21 @@ void fr_tacacs_free(void) fr_dict_autofree(libfreeradius_tacacs_dict); } +/** XOR the body based on the secret key. + * + * This function encrypts (or decrypts) TACACS+ packets, and sets the "encrypted" flag. + */ int fr_tacacs_body_xor(fr_tacacs_packet_t const *pkt, uint8_t *body, size_t body_len, char const *secret, size_t secret_len) { uint8_t pad[MD5_DIGEST_LENGTH]; - uint8_t *buf; + uint8_t *buf, *end; int pad_offset; - size_t pos; - - if (!secret || !secret_len) { - if (pkt->hdr.flags & FR_TAC_PLUS_UNENCRYPTED_FLAG) - return 0; - else { - fr_strerror_const("Packet is encrypted but no secret for the client is set"); - return -1; - } - } - if (pkt->hdr.flags & FR_TAC_PLUS_UNENCRYPTED_FLAG) { - fr_strerror_const("Packet is unencrypted but a secret has been set for the client"); + /* + * Do some basic sanity checks. + */ + if (!secret_len) { + fr_strerror_const("Failed to encrypt/decrept the packet, as the secret has zero length."); return -1; } @@ -162,6 +159,7 @@ int fr_tacacs_body_xor(fr_tacacs_packet_t const *pkt, uint8_t *body, size_t body /* MD5_1 = MD5{session_id, key, version, seq_no} */ /* MD5_n = MD5{session_id, key, version, seq_no, MD5_n-1} */ buf = talloc_array(NULL, uint8_t, pad_offset + MD5_DIGEST_LENGTH); + if (!buf) return -1; memcpy(&buf[0], &pkt->hdr.session_id, sizeof(pkt->hdr.session_id)); memcpy(&buf[sizeof(pkt->hdr.session_id)], secret, secret_len); @@ -170,18 +168,21 @@ int fr_tacacs_body_xor(fr_tacacs_packet_t const *pkt, uint8_t *body, size_t body fr_md5_calc(pad, buf, pad_offset); - pos = 0; - do { - for (size_t i = 0; i < MD5_DIGEST_LENGTH && pos < body_len; i++, pos++) - body[pos] ^= pad[i]; + end = body + body_len; + while (body < end) { + size_t i; + + for (i = 0; i < MD5_DIGEST_LENGTH; i++) { + *body ^= pad[i]; - if (pos == body_len) - break; + if (++body == end) goto done; + } memcpy(&buf[pad_offset], pad, MD5_DIGEST_LENGTH); fr_md5_calc(pad, buf, pad_offset + MD5_DIGEST_LENGTH); - } while (1); + } +done: talloc_free(buf); return 0; diff --git a/src/protocols/tacacs/decode.c b/src/protocols/tacacs/decode.c index 03332b902e9..ea3abb0a0e2 100644 --- a/src/protocols/tacacs/decode.c +++ b/src/protocols/tacacs/decode.c @@ -420,6 +420,16 @@ ssize_t fr_tacacs_decode(TALLOC_CTX *ctx, fr_pair_list_t *out, uint8_t const *bu return -1; } + if (!secret && packet_is_encrypted(pkt)) { + fr_strerror_const("Packet is encrypted, but there is no secret to decrypt it"); + return -1; + } + + if (secret && !packet_is_encrypted(pkt)) { + fr_strerror_const("Packet is clear-text but we expected it to be encrypted"); + return -1; + } + /* * Call the struct encoder to do the actual work. */ @@ -431,13 +441,13 @@ ssize_t fr_tacacs_decode(TALLOC_CTX *ctx, fr_pair_list_t *out, uint8_t const *bu /* * 3.6. Encryption * - * Packets are encrypted if the unencrypted flag is clear. + * If there's a secret, we alway decrypt the packets. */ - if ((pkt->hdr.flags & FR_TAC_PLUS_UNENCRYPTED_FLAG) == 0) { + if (secret) { size_t length; - if (!secret || secret_len < 1) { - fr_strerror_const("Packet is encrypted, but no secret is set."); + if (!secret_len) { + fr_strerror_const("Packet should be encrypted, but the secret has zero length"); return -1; } @@ -461,7 +471,7 @@ ssize_t fr_tacacs_decode(TALLOC_CTX *ctx, fr_pair_list_t *out, uint8_t const *bu return -1; } - decrypted[3] |= 0x01; /* say it's decrypted */ + decrypted[3] |= FR_TAC_PLUS_UNENCRYPTED_FLAG; FR_PROTO_HEX_DUMP(decrypted, buffer_len, "fr_tacacs_packet_t (unencrypted)"); } diff --git a/src/protocols/tacacs/encode.c b/src/protocols/tacacs/encode.c index 3075f2ea75a..9925d692251 100644 --- a/src/protocols/tacacs/encode.c +++ b/src/protocols/tacacs/encode.c @@ -834,14 +834,9 @@ ssize_t fr_tacacs_encode(fr_dbuff_t *dbuff, uint8_t const *original_packet, char */ if (original && ((original->flags & FR_TAC_PLUS_UNENCRYPTED_FLAG) == 0)) { - packet->hdr.flags = packet->hdr.flags & ~FR_TAC_PLUS_UNENCRYPTED_FLAG; + packet->hdr.flags &= ~FR_TAC_PLUS_UNENCRYPTED_FLAG; } - /* - * Packets which have no secret cannot be encrypted. - */ - if (!secret) packet->hdr.flags |= FR_TAC_PLUS_UNENCRYPTED_FLAG; - #ifndef NDEBUG if (fr_debug_lvl >= L_DBG_LVL_4) { uint8_t flags = packet->hdr.flags; @@ -857,15 +852,23 @@ ssize_t fr_tacacs_encode(fr_dbuff_t *dbuff, uint8_t const *original_packet, char * * Packets are encrypted if the unencrypted flag is clear. */ - if ((packet->hdr.flags & FR_TAC_PLUS_UNENCRYPTED_FLAG) == 0) { - if (!secret || secret_len < 1) { - fr_strerror_const("decode: Packet is supposed to be encrypted, but no secret is set."); + if (secret) { + if (!secret_len) { + fr_strerror_const("Packet should be decrypted, but the secret has zero length"); return -1; } FR_PROTO_HEX_DUMP(fr_dbuff_start(&work_dbuff), packet_len, "fr_tacacs_packet_t (unencrypted)"); if (fr_tacacs_body_xor(packet, fr_dbuff_current(&body), body_len, secret, secret_len) != 0) return -1; + + packet->hdr.flags &= ~FR_TAC_PLUS_UNENCRYPTED_FLAG; + } else { + /* + * Packets which have no secret cannot be encrypted. + */ + packet->hdr.flags |= FR_TAC_PLUS_UNENCRYPTED_FLAG; + } FR_PROTO_HEX_DUMP(fr_dbuff_start(&work_dbuff), packet_len, "fr_tacacs_packet_t (encoded)");