Trace Redaction - Reject invalid traces
In order to ensure proper redaction, trace redaction can and should
be very strict about its input. Small errors (e.g. missing a proc
free packet) can lead to major issues (e.g. two threads get merged
into a single thread because the proc free message was missing).
Bug: 338425440
Change-Id: I0b8abcd5fc2f196c8bf94f9edfbcf223319d0770
diff --git a/Android.bp b/Android.bp
index 7e67314..a770cb5 100644
--- a/Android.bp
+++ b/Android.bp
@@ -13609,6 +13609,7 @@
"src/trace_redaction/suspend_resume.cc",
"src/trace_redaction/trace_redaction_framework.cc",
"src/trace_redaction/trace_redactor.cc",
+ "src/trace_redaction/verify_integrity.cc",
],
}
@@ -13632,6 +13633,7 @@
"src/trace_redaction/remap_scheduling_events_unittest.cc",
"src/trace_redaction/remove_process_free_comm_unittest.cc",
"src/trace_redaction/suspend_resume_unittest.cc",
+ "src/trace_redaction/verify_integrity_unittest.cc",
],
}
diff --git a/src/trace_redaction/BUILD.gn b/src/trace_redaction/BUILD.gn
index b4aecb6..c9ab86e 100644
--- a/src/trace_redaction/BUILD.gn
+++ b/src/trace_redaction/BUILD.gn
@@ -81,6 +81,8 @@
"trace_redaction_framework.h",
"trace_redactor.cc",
"trace_redactor.h",
+ "verify_integrity.cc",
+ "verify_integrity.h",
]
deps = [
"../../gn:default_deps",
@@ -111,6 +113,7 @@
"scrub_process_trees_integrationtest.cc",
"trace_redaction_integration_fixture.cc",
"trace_redaction_integration_fixture.h",
+ "verify_integrity_integrationtest.cc",
]
deps = [
":trace_redaction",
@@ -146,6 +149,7 @@
"remap_scheduling_events_unittest.cc",
"remove_process_free_comm_unittest.cc",
"suspend_resume_unittest.cc",
+ "verify_integrity_unittest.cc",
]
deps = [
":trace_redaction",
diff --git a/src/trace_redaction/main.cc b/src/trace_redaction/main.cc
index 78bf3d0..798d1a0 100644
--- a/src/trace_redaction/main.cc
+++ b/src/trace_redaction/main.cc
@@ -39,6 +39,7 @@
#include "src/trace_redaction/suspend_resume.h"
#include "src/trace_redaction/trace_redaction_framework.h"
#include "src/trace_redaction/trace_redactor.h"
+#include "src/trace_redaction/verify_integrity.h"
namespace perfetto::trace_redaction {
@@ -48,6 +49,12 @@
std::string_view package_name) {
TraceRedactor redactor;
+ // VerifyIntegrity breaks the CollectPrimitive pattern. Instead of writing to
+ // the context, its job is to read trace packets and return errors if any
+ // packet does not look "correct". This primitive is added first in an effort
+ // to detect and react to bad input before other collectors run.
+ redactor.emplace_collect<VerifyIntegrity>();
+
// Add all collectors.
redactor.emplace_collect<FindPackageUid>();
redactor.emplace_collect<CollectTimelineEvents>();
diff --git a/src/trace_redaction/verify_integrity.cc b/src/trace_redaction/verify_integrity.cc
new file mode 100644
index 0000000..53fdc0b
--- /dev/null
+++ b/src/trace_redaction/verify_integrity.cc
@@ -0,0 +1,104 @@
+/*
+ * Copyright (C) 2024 The Android Open Source Projectf
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include "src/trace_redaction/verify_integrity.h"
+
+#include "protos/perfetto/trace/ftrace/ftrace_event.pbzero.h"
+#include "protos/perfetto/trace/ftrace/ftrace_event_bundle.pbzero.h"
+#include "protos/perfetto/trace/trace_packet.pbzero.h"
+
+namespace perfetto::trace_redaction {
+namespace {
+// All constants are from
+// "system/core/libcutils/include/private/android_filesystem_config.h"
+//
+// AID 1000 == system (those are probably frame_timeline packets you will see
+// those on) AID 9999 is nobody == traced/traced_probes
+constexpr int32_t kAidSystem = 1000;
+constexpr int32_t kAidNobody = 9999;
+} // namespace
+
+base::Status VerifyIntegrity::Collect(
+ const protos::pbzero::TracePacket::Decoder& packet,
+ Context*) const {
+ if (!packet.has_trusted_uid()) {
+ return base::ErrStatus(
+ "VerifyIntegrity: missing field (TracePacket::kTrustedUid).");
+ }
+
+ if (packet.trusted_uid() != kAidSystem &&
+ packet.trusted_uid() != kAidNobody) {
+ return base::ErrStatus(
+ "VerifyIntegrity: invalid field (TracePacket::kTrustedUid).");
+ }
+
+ if (packet.has_ftrace_events()) {
+ protos::pbzero::FtraceEventBundle::Decoder ftrace_events(
+ packet.ftrace_events());
+
+ // The other clocks in ftrace are only used on very old kernel versions. No
+ // device with V should have such an old version. As a failsafe though,
+ // check that the ftrace_clock field is unset to ensure no invalid
+ // timestamps get by.
+ if (ftrace_events.has_ftrace_clock()) {
+ return base::ErrStatus(
+ "VerifyIntegrity: unexpected field "
+ "(FtraceEventBundle::kFtraceClock).");
+ }
+
+ // Every ftrace event bundle should have a CPU field. This is necessary for
+ // switch/waking redaction to work.
+ if (!ftrace_events.has_cpu()) {
+ return base::ErrStatus(
+ "VerifyIntegrity: missing field (FtraceEventBundle::kCpu).");
+ }
+
+ for (auto event_buffer = ftrace_events.event(); event_buffer;
+ ++event_buffer) {
+ protos::pbzero::FtraceEvent::Decoder event(*event_buffer);
+
+ if (!event.has_timestamp()) {
+ return base::ErrStatus(
+ "VerifyIntegrity: missing field (FtraceEvent::kTimestamp).");
+ }
+
+ if (!event.has_pid()) {
+ return base::ErrStatus(
+ "VerifyIntegrity: missing field (FtraceEvent::kPid).");
+ }
+ }
+ }
+
+ // If there is a process tree, there should be a timestamp on the packet. This
+ // is the only way to know when the process tree was collected.
+ if (packet.has_process_tree() && !packet.has_timestamp()) {
+ return base::ErrStatus(
+ "VerifyIntegrity: missing fields (TracePacket::kProcessTree + "
+ "TracePacket::kTimestamp).");
+ }
+
+ // If there are a process stats, there should be a timestamp on the packet.
+ // This is the only way to know when the stats were collected.
+ if (packet.has_process_stats() && !packet.has_timestamp()) {
+ return base::ErrStatus(
+ "VerifyIntegrity: missing fields (TracePacket::kProcessStats + "
+ "TracePacket::kTimestamp).");
+ }
+
+ return base::OkStatus();
+}
+} // namespace perfetto::trace_redaction
diff --git a/src/trace_redaction/verify_integrity.h b/src/trace_redaction/verify_integrity.h
new file mode 100644
index 0000000..2ac5232
--- /dev/null
+++ b/src/trace_redaction/verify_integrity.h
@@ -0,0 +1,38 @@
+/*
+ * Copyright (C) 2024 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include "protos/perfetto/trace/ftrace/ftrace_event_bundle.pbzero.h"
+#include "src/trace_redaction/trace_redaction_framework.h"
+
+#include "protos/perfetto/trace/trace_packet.pbzero.h"
+
+#ifndef SRC_TRACE_REDACTION_VERIFY_INTEGRITY_H_
+#define SRC_TRACE_REDACTION_VERIFY_INTEGRITY_H_
+
+namespace perfetto::trace_redaction {
+
+// This breaks the normal collect primitive pattern. Rather than collecting
+// information, it looks at packets and returns an error if the packet violates
+// any requirements.
+class VerifyIntegrity : public CollectPrimitive {
+ public:
+ base::Status Collect(const protos::pbzero::TracePacket::Decoder& packet,
+ Context* context) const override;
+};
+
+} // namespace perfetto::trace_redaction
+
+#endif // SRC_TRACE_REDACTION_VERIFY_INTEGRITY_H_
diff --git a/src/trace_redaction/verify_integrity_integrationtest.cc b/src/trace_redaction/verify_integrity_integrationtest.cc
new file mode 100644
index 0000000..d97f3c9
--- /dev/null
+++ b/src/trace_redaction/verify_integrity_integrationtest.cc
@@ -0,0 +1,35 @@
+/*
+ * Copyright (C) 2024 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include "src/base/test/status_matchers.h"
+#include "src/trace_redaction/trace_redaction_integration_fixture.h"
+#include "src/trace_redaction/trace_redactor.h"
+#include "src/trace_redaction/verify_integrity.h"
+#include "test/gtest_and_gmock.h"
+
+namespace perfetto::trace_redaction {
+
+class VerifyIntegrityIntegrationTest : public testing::Test,
+ public TraceRedactionIntegrationFixure {
+};
+
+// The trace used in the integration tests should pass the verify primitive.
+TEST_F(VerifyIntegrityIntegrationTest, VerifiesValidTrace) {
+ trace_redactor()->emplace_collect<VerifyIntegrity>();
+ ASSERT_OK(Redact());
+}
+
+} // namespace perfetto::trace_redaction
diff --git a/src/trace_redaction/verify_integrity_unittest.cc b/src/trace_redaction/verify_integrity_unittest.cc
new file mode 100644
index 0000000..638f056
--- /dev/null
+++ b/src/trace_redaction/verify_integrity_unittest.cc
@@ -0,0 +1,196 @@
+/*
+ * Copyright (C) 2024 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include "src/trace_redaction/verify_integrity.h"
+#include "src/base/test/status_matchers.h"
+#include "test/gtest_and_gmock.h"
+
+#include "protos/perfetto/trace/ftrace/ftrace_event.gen.h"
+#include "protos/perfetto/trace/ftrace/ftrace_event_bundle.gen.h"
+#include "protos/perfetto/trace/trace_packet.gen.h"
+
+namespace perfetto::trace_redaction {
+
+namespace {
+// The trace packet uid must be 1000 (system) or 9999 (nobody). If it is
+// anything else, the packet is invalid.
+int32_t kNobodyUid = 9999;
+int32_t kSystemUid = 1000;
+int32_t kInvalidUid = 9;
+
+uint64_t kSomeTime = 1234;
+uint32_t kSomePid = 7;
+
+uint32_t kSomeCpu = 3;
+} // namespace
+
+class VerifyIntegrityUnitTest : public testing::Test {
+ protected:
+ base::Status Verify(const protos::gen::TracePacket& packet) {
+ auto packet_buffer = packet.SerializeAsString();
+ protos::pbzero::TracePacket::Decoder packer_decoder(packet_buffer);
+
+ VerifyIntegrity verify;
+ Context context;
+ return verify.Collect(packer_decoder, &context);
+ }
+};
+
+TEST_F(VerifyIntegrityUnitTest, InvalidPacketNoUid) {
+ protos::gen::TracePacket packet;
+ ASSERT_FALSE(Verify(packet).ok());
+}
+
+TEST_F(VerifyIntegrityUnitTest, InvalidPacketInvalidUid) {
+ protos::gen::TracePacket packet;
+
+ packet.set_trusted_uid(kInvalidUid);
+
+ ASSERT_FALSE(Verify(packet).ok());
+}
+
+TEST_F(VerifyIntegrityUnitTest, ValidPacketSystemUid) {
+ protos::gen::TracePacket packet;
+
+ packet.set_trusted_uid(kSystemUid);
+
+ ASSERT_OK(Verify(packet));
+}
+
+TEST_F(VerifyIntegrityUnitTest, ValidPacketNobodyUid) {
+ protos::gen::TracePacket packet;
+
+ packet.set_trusted_uid(kNobodyUid);
+
+ ASSERT_OK(Verify(packet));
+}
+
+TEST_F(VerifyIntegrityUnitTest, InvalidPacketFtraceBundleMissingCpu) {
+ protos::gen::TracePacket packet;
+
+ packet.set_trusted_uid(kSystemUid);
+
+ packet.mutable_ftrace_events();
+
+ ASSERT_FALSE(Verify(packet).ok());
+}
+
+TEST_F(VerifyIntegrityUnitTest, ValidPacketFtraceBundle) {
+ protos::gen::TracePacket packet;
+
+ packet.set_trusted_uid(kSystemUid);
+
+ // A bundle doesn't need to have anything in it (other than cpu).
+ auto* ftrace_events = packet.mutable_ftrace_events();
+ ftrace_events->set_cpu(kSomeCpu);
+
+ ASSERT_OK(Verify(packet));
+}
+
+TEST_F(VerifyIntegrityUnitTest, InvalidPacketFtraceEventMissingPid) {
+ protos::gen::TracePacket packet;
+
+ packet.set_trusted_uid(kSystemUid);
+
+ auto* ftrace_events = packet.mutable_ftrace_events();
+ ftrace_events->set_cpu(kSomeCpu);
+
+ // A valid event has a pid and timestamp. Add the time (but not the pid) to
+ // ensure the pid caused the error.
+ auto* event = ftrace_events->add_event();
+ event->set_timestamp(kSomeTime);
+
+ ASSERT_FALSE(Verify(packet).ok());
+}
+
+TEST_F(VerifyIntegrityUnitTest, InvalidPacketFtraceEventMissingTime) {
+ protos::gen::TracePacket packet;
+
+ packet.set_trusted_uid(kSystemUid);
+
+ auto* ftrace_events = packet.mutable_ftrace_events();
+ ftrace_events->set_cpu(kSomeCpu);
+
+ // A valid event has a pid and timestamp. Add the pid (but not the time) to
+ // ensure the time caused the error.
+ auto* event = ftrace_events->add_event();
+ event->set_pid(kSomePid);
+
+ ASSERT_FALSE(Verify(packet).ok());
+}
+
+TEST_F(VerifyIntegrityUnitTest, ValidPacketFtraceEvent) {
+ protos::gen::TracePacket packet;
+
+ packet.set_trusted_uid(kSystemUid);
+
+ auto* ftrace_events = packet.mutable_ftrace_events();
+ ftrace_events->set_cpu(kSomeCpu);
+
+ auto* event = ftrace_events->add_event();
+ event->set_pid(kSomePid);
+ event->set_timestamp(kSomeTime);
+
+ ASSERT_OK(Verify(packet));
+}
+
+TEST_F(VerifyIntegrityUnitTest, InvalidPacketProcessTreeMissingTime) {
+ protos::gen::TracePacket packet;
+
+ packet.set_trusted_uid(kSystemUid);
+
+ // When the packet has a process tree, the packet must have a timestamp.
+ packet.mutable_process_tree();
+
+ ASSERT_FALSE(Verify(packet).ok());
+}
+
+TEST_F(VerifyIntegrityUnitTest, ValidPacketProcessTree) {
+ protos::gen::TracePacket packet;
+
+ packet.set_trusted_uid(kSystemUid);
+
+ // When the packet has a process tree, the packet must have a timestamp.
+ packet.mutable_process_tree();
+ packet.set_timestamp(kSomeTime);
+
+ ASSERT_OK(Verify(packet));
+}
+
+TEST_F(VerifyIntegrityUnitTest, InvalidPacketProcessStatsMissingTime) {
+ protos::gen::TracePacket packet;
+
+ packet.set_trusted_uid(kSystemUid);
+
+ // When the packet has process stats, the packet must have a timestamp.
+ packet.mutable_process_stats();
+
+ ASSERT_FALSE(Verify(packet).ok());
+}
+
+TEST_F(VerifyIntegrityUnitTest, ValidPacketProcessStats) {
+ protos::gen::TracePacket packet;
+
+ packet.set_trusted_uid(kSystemUid);
+
+ // When the packet has a process tree, the packet must have a timestamp.
+ packet.mutable_process_stats();
+ packet.set_timestamp(kSomeTime);
+
+ ASSERT_OK(Verify(packet));
+}
+
+} // namespace perfetto::trace_redaction
