From 541d4591e976a6858d949847f8e0acb2fa9c3b18 Mon Sep 17 00:00:00 2001
From: Steve Karg <steve@kargs.net>
Date: Fri, 23 Apr 2021 13:44:39 -0500
Subject: [PATCH] Reject DCC password out of range (#166)

* Reject DCC password out of range

* Fix unit test after return value change

Co-authored-by: Steve Karg <skarg@users.sourceforge.net>
---
 src/bacnet/basic/service/h_dcc.c | 20 ++++++++++++++------
 src/bacnet/dcc.c                 | 27 +++++++++++++++++----------
 2 files changed, 31 insertions(+), 16 deletions(-)

diff --git a/src/bacnet/basic/service/h_dcc.c b/src/bacnet/basic/service/h_dcc.c
index 5f047c78..12018e0e 100644
--- a/src/bacnet/basic/service/h_dcc.c
+++ b/src/bacnet/basic/service/h_dcc.c
@@ -138,15 +138,23 @@ void handler_device_communication_control(uint8_t *service_request,
             (unsigned)timeDuration, (unsigned)state,
             characterstring_value(&password));
 #endif
-    /* bad decoding or something we didn't understand - send an abort */
+    /* bad decoding or invalid service parameter
+       send an abort or reject */
     if (len < 0) {
-        len = abort_encode_apdu(&Handler_Transmit_Buffer[pdu_len],
-            service_data->invoke_id, ABORT_REASON_OTHER, true);
+        if (len == BACNET_STATUS_ABORT) {
+            len = abort_encode_apdu(&Handler_Transmit_Buffer[pdu_len],
+                service_data->invoke_id, ABORT_REASON_OTHER, true);
 #if PRINT_ENABLED
-        fprintf(stderr,
-            "DeviceCommunicationControl: "
-            "Sending Abort - could not decode.\n");
+            fprintf(stderr, "DCC: Sending Abort!\n");
 #endif
+        } else if (len == BACNET_STATUS_REJECT) {
+            len = reject_encode_apdu(&Handler_Transmit_Buffer[pdu_len],
+                service_data->invoke_id,
+                REJECT_REASON_PARAMETER_OUT_OF_RANGE);
+#if PRINT_ENABLED
+            fprintf(stderr, "DCC: Sending Reject!\n");
+#endif
+        }
         goto DCC_ABORT;
     }
     if (state >= MAX_BACNET_COMMUNICATION_ENABLE_DISABLE) {
diff --git a/src/bacnet/dcc.c b/src/bacnet/dcc.c
index 7d4e16b1..aec64c64 100644
--- a/src/bacnet/dcc.c
+++ b/src/bacnet/dcc.c
@@ -217,7 +217,7 @@ int dcc_encode_apdu(uint8_t *apdu,
  * enable/disable.
  * @param password  Pointer to the password [optional]
  *
- * @return Bytes decoded.
+ * @return Bytes decoded, or BACNET_STATUS_ABORT or BACNET_STATUS_REJECT
  */
 int dcc_decode_service_request(uint8_t *apdu,
     unsigned apdu_len_max,
@@ -231,6 +231,7 @@ int dcc_decode_service_request(uint8_t *apdu,
     uint32_t len_value_type = 0;
     BACNET_UNSIGNED_INTEGER decoded_unsigned = 0;
     uint32_t decoded_enum = 0;
+    uint32_t password_length = 0;
 
     if (apdu && apdu_len_max) {
         /* Tag 0: timeDuration, in minutes --optional-- */
@@ -243,10 +244,10 @@ int dcc_decode_service_request(uint8_t *apdu,
                     *timeDuration = (uint16_t)decoded_unsigned;
                 }
             } else {
-                return BACNET_STATUS_ERROR;
+                return BACNET_STATUS_REJECT;
             }
         } else if (len < 0) {
-            return BACNET_STATUS_ERROR;
+            return BACNET_STATUS_ABORT;
         } else if (timeDuration) {
             /* zero indicates infinite duration and
                results in no timeout */
@@ -263,7 +264,7 @@ int dcc_decode_service_request(uint8_t *apdu,
                         (BACNET_COMMUNICATION_ENABLE_DISABLE)decoded_enum;
                 }
             } else {
-                return BACNET_STATUS_ERROR;
+                return BACNET_STATUS_ABORT;
             }
         }
         if ((unsigned)apdu_len < apdu_len_max) {
@@ -272,7 +273,7 @@ int dcc_decode_service_request(uint8_t *apdu,
                 /* since this is the last value of the packet,
                    if there is data here it must be the specific
                    context tag number or result in an error */
-                return BACNET_STATUS_ERROR;
+                return BACNET_STATUS_ABORT;
             }
             len = bacnet_tag_number_and_value_decode(&apdu[apdu_len],
                 apdu_len_max - apdu_len, &tag_number, &len_value_type);
@@ -282,15 +283,21 @@ int dcc_decode_service_request(uint8_t *apdu,
                     len = bacnet_character_string_decode(&apdu[apdu_len],
                         apdu_len_max - apdu_len, len_value_type, password);
                     if (len > 0) {
-                        apdu_len += len;
+                        password_length = len_value_type - 1;
+                        if ((password_length >= 1) &&
+                            (password_length <= 20)) {
+                            apdu_len += len;
+                        } else {
+                            return BACNET_STATUS_REJECT;
+                        }
                     } else {
-                        return BACNET_STATUS_ERROR;
+                        return BACNET_STATUS_ABORT;
                     }
                 } else {
-                    return BACNET_STATUS_ERROR;
+                    return BACNET_STATUS_ABORT;
                 }
             } else {
-                return BACNET_STATUS_ERROR;
+                return BACNET_STATUS_ABORT;
             }
         } else if (password) {
             /* no optional password - set to NULL */
@@ -419,7 +426,7 @@ void test_DeviceCommunicationControlMalformedData(Test *pTest)
     ct_test(pTest, len == BACNET_STATUS_ERROR);
     len = dcc_decode_apdu(&payload_4[0], sizeof(payload_4), &test_invoke_id,
         &test_timeDuration, &test_enable_disable, &test_password);
-    ct_test(pTest, len == BACNET_STATUS_ERROR);
+    ct_test(pTest, len == BACNET_STATUS_ABORT);
     len = dcc_decode_apdu(&payload_5[0], sizeof(payload_5), &test_invoke_id,
         &test_timeDuration, &test_enable_disable, &test_password);
     ct_test(pTest, len == BACNET_STATUS_ERROR);