diff --git a/src/daemon/dlt_daemon_client.c b/src/daemon/dlt_daemon_client.c index 4d576f58..fc438a7c 100644 --- a/src/daemon/dlt_daemon_client.c +++ b/src/daemon/dlt_daemon_client.c @@ -4027,84 +4027,121 @@ void dlt_daemon_control_set_trace_status_v2(int sock, char *apid = NULL; char *ctid = NULL; - DltServiceSetLogLevelV2 *req = NULL; DltDaemonContext *context = NULL; - int8_t apid_length = 0; - int8_t ctid_length = 0; + uint32_t service_id = 0; + uint8_t apidlen = 0; + uint8_t ctidlen = 0; + uint8_t log_level = 0; + int offset = 0; if ((daemon == NULL) || (msg == NULL) || (msg->databuffer == NULL)) return; - //TBD: Review sizeof(DltServiceSetLogLevelV2)) or fixed size - if (dlt_check_rcv_data_size(msg->datasize, sizeof(DltServiceSetLogLevelV2)) < 0) + if (dlt_check_rcv_data_size(msg->datasize, DLT_SERVICE_SET_LOG_LEVEL_FIXED_SIZE_V2) < 0) return; - req = (DltServiceSetLogLevelV2 *)(msg->databuffer); + /* Parse wire bytes incrementally instead of casting msg->databuffer to + * DltServiceSetLogLevelV2 *. The struct contains char *apid / char *ctid + * pointer fields. Under a direct cast those fields would have been + * filled with attacker-controlled pointer values from network input, + * then dereferenced by dlt_set_id_v2 — an arbitrary memory read. The + * previous code also fell into the same NULL-deref pattern as #863 + * (apid / ctid NULL when used at apid[apid_length - 1] etc.). + * See #866 for the broader pattern. */ + memcpy(&service_id, msg->databuffer + offset, sizeof(uint32_t)); + offset = offset + (int)sizeof(uint32_t); + memcpy(&apidlen, msg->databuffer + offset, sizeof(uint8_t)); + offset = offset + (int)sizeof(uint8_t); - if (daemon_local->flags.enforceContextLLAndTS) - req->log_level = (uint8_t) getStatus(req->log_level, daemon_local->flags.contextTraceStatus); + /* Bounds check for apid (apidlen bytes) + the upcoming ctidlen byte. + * The fixed-size precheck at the top only validates the empty case. */ + if ((offset + (int)apidlen + (int)sizeof(uint8_t)) > msg->datasize) + return; - apid_length = (int8_t) req->apidlen; - dlt_set_id_v2(apid, req->apid, req->apidlen); - ctid_length = (int8_t) req->ctidlen; - dlt_set_id_v2(ctid, req->ctid, req->ctidlen); + if (apidlen > 0) { + apid = (char *)(msg->databuffer + offset); + offset = offset + (int)apidlen; + } + memcpy(&ctidlen, msg->databuffer + offset, sizeof(uint8_t)); + offset = offset + (int)sizeof(uint8_t); - //TBD: Review apid[apid_length - 1] == '*' - if ((apid_length != 0) && (apid[apid_length - 1] == '*') && (ctid == NULL)) { /*apid provided having '*' in it and ctid is null*/ + /* Bounds check for ctid (ctidlen bytes) + log_level (1 byte) + + * the trailing com field (DLT_ID_SIZE bytes). */ + if ((offset + (int)ctidlen + (int)sizeof(uint8_t) + DLT_ID_SIZE) > msg->datasize) + return; + + if (ctidlen > 0) { + ctid = (char *)(msg->databuffer + offset); + offset = offset + (int)ctidlen; + } + memcpy(&log_level, msg->databuffer + offset, sizeof(uint8_t)); + + if (daemon_local->flags.enforceContextLLAndTS) + log_level = (uint8_t) getStatus(log_level, daemon_local->flags.contextTraceStatus); + + /* Use unsigned widths throughout. Casting uint8_t apidlen / ctidlen + * to int8_t (as the previous code did) would produce a negative value + * when the length exceeds 127, underflowing apid[apid_length - 1] etc. + * Note: downstream helpers below still take int8_t lengths; that + * narrowing remains a separate latent issue, out of scope here. */ + if ((apidlen != 0) && (apid != NULL) && (apid[apidlen - 1] == '*') && (ctid == NULL)) { + /* apid provided having '*' in it and ctid is null */ dlt_daemon_find_multiple_context_and_send_trace_status_v2(sock, daemon, daemon_local, 1, apid, - (int8_t) (apid_length - 1), - (int8_t) req->log_level, + (int8_t) (apidlen - 1), + (int8_t) log_level, verbose); } - else if ((ctid_length != 0) && (ctid[ctid_length - 1] == '*') && (apid == NULL)) /*ctid provided is having '*' in it and apid is null*/ - + else if ((ctidlen != 0) && (ctid != NULL) && (ctid[ctidlen - 1] == '*') && (apid == NULL)) { + /* ctid provided is having '*' in it and apid is null */ dlt_daemon_find_multiple_context_and_send_trace_status_v2(sock, daemon, daemon_local, 0, ctid, - (int8_t) (ctid_length - 1), - (int8_t) req->log_level, + (int8_t) (ctidlen - 1), + (int8_t) log_level, verbose); } - else if ((apid_length != 0) && (apid[apid_length - 1] != '*') && (ctid == NULL)) /*only app id case*/ + else if ((apidlen != 0) && (apid != NULL) && (apid[apidlen - 1] != '*') && (ctid == NULL)) { + /* only app id case */ dlt_daemon_find_multiple_context_and_send_trace_status_v2(sock, daemon, daemon_local, 1, apid, - apid_length, - (int8_t) req->log_level, + (int8_t) apidlen, + (int8_t) log_level, verbose); } - else if ((ctid_length != 0) && (ctid[ctid_length - 1] != '*') && (apid == NULL)) /*only context id case*/ + else if ((ctidlen != 0) && (ctid != NULL) && (ctid[ctidlen - 1] != '*') && (apid == NULL)) { + /* only context id case */ dlt_daemon_find_multiple_context_and_send_trace_status_v2(sock, daemon, daemon_local, 0, ctid, - ctid_length, - (int8_t) req->log_level, + (int8_t) ctidlen, + (int8_t) log_level, verbose); } else { - context = dlt_daemon_context_find_v2(daemon, (uint8_t)apid_length, apid, (uint8_t)ctid_length, ctid, daemon->ecuid2len, daemon->ecuid2, verbose); + context = dlt_daemon_context_find_v2(daemon, apidlen, apid, ctidlen, ctid, daemon->ecuid2len, daemon->ecuid2, verbose); /* Set trace status */ if (context != 0) { - dlt_daemon_send_trace_status_v2(sock, daemon, daemon_local, context, (int8_t) req->log_level, verbose); + dlt_daemon_send_trace_status_v2(sock, daemon, daemon_local, context, (int8_t) log_level, verbose); } else { dlt_vlog(LOG_ERR, "Could not set trace status: %d. Context [%.4s:%.4s] not found:", - req->log_level, + log_level, apid ? apid : "", ctid ? ctid : ""); dlt_daemon_control_service_response_v2(sock,