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 b8955bcbb7dc3c7830b0fcd54074fd1221362605..0ed4fd0708269a853215683f0a5fd1153b458d5c 100644 (file)
--- 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 03332b902e9733479c8445fa7c1fb9ca48179932..ea3abb0a0e2d89d77f60540ef1c17fdd01b94c9e 100644 (file)
--- 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 3075f2ea75aa056965584936e67660a6af62a0a6..9925d6922510f5d7fa9c35a6515445035fb51985 100644 (file)
--- 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)");