Perform validation before reading new data for FTrace header parser

According to OWASP 2021 top 10 list [1], injection attacks are the third most common type of cyber attack in 2021. When parsing a specific section in the binary Ftrace header, the parser needs to read the section size, then use this value as the limit to read the content of the section. One can take advantage of this by modifying the section sizes to go out of the trace size limit, thus gaining access into unrelated memory locations. This commit adds a security layer for the Ftrace header parser. It ensures that the parser will not go out of bound by comparing the expected different sections with the binary FTrace format and file size. [1] https://owasp.org/Top10/ Change-Id: I561c94838324b0c5da86401d7405081ef29affbe Signed-off-by: Hoang Thuan Pham <hoang.pham@calian.ca> Reviewed-on: https://git.eclipse.org/r/c/tracecompass.incubator/org.eclipse.tracecompass.incubator/+/191407 Tested-by: Trace Compass Bot <tracecompass-bot@eclipse.org> Tested-by: Matthew Khouzam <matthew.khouzam@ericsson.com> Tested-by: Bernd Hufmann <bernd.hufmann@ericsson.com> Reviewed-by: Matthew Khouzam <matthew.khouzam@ericsson.com> Reviewed-by: Bernd Hufmann <bernd.hufmann@ericsson.com>
eclipse-tracecompass-incubator · Mar 9, 2022 · e65e905 · e65e905
1 parent a734f67
commit e65e905
Show file tree

Hide file tree

Showing 8 changed files with 369 additions and 67 deletions.
diff --git a/...se/tracecompass/incubator/ftrace/core/tests/binary/buffer/BinaryFTraceByteBufferTest.java b/...se/tracecompass/incubator/ftrace/core/tests/binary/buffer/BinaryFTraceByteBufferTest.java
@@ -31,9 +31,6 @@ public class BinaryFTraceByteBufferTest {
     private static final String TRACING = "tracing";
     private static final String VERSION = "6";
     private static final int RANDOM_OFFSET = 3;
-    private static final int MAGIC_VALUE_SIZE = 3;
-    private static final int TRACING_STRING_SIZE = 7;
-    private static final int FTRACE_VERSION_SIZE = 1;
 
     /**
      * Test to make sure the current offset is incremented properly when reading
@@ -50,17 +47,17 @@ public void testOffsetCounter() throws IOException {
 
         try (BinaryFTraceByteBuffer buffer = new BinaryFTraceByteBuffer(traceLocation)) {
             // Read the magic values
-            buffer.getNextBytes(MAGIC_VALUE_SIZE);
-            currentOffset += MAGIC_VALUE_SIZE;
+            buffer.getNextBytes(BinaryFTraceHeaderElementSize.MAGIC_VALUE);
+            currentOffset += BinaryFTraceHeaderElementSize.MAGIC_VALUE;
             assertEquals(buffer.getCurrentOffset(), currentOffset);
 
-            String tracingString = buffer.getNextBytesAsString(TRACING_STRING_SIZE);
-            currentOffset += TRACING_STRING_SIZE;
+            String tracingString = buffer.getNextBytesAsString(BinaryFTraceHeaderElementSize.TRACING_STRING);
+            currentOffset += BinaryFTraceHeaderElementSize.TRACING_STRING;
             assertEquals(tracingString, TRACING);
             assertEquals(buffer.getCurrentOffset(), currentOffset);
 
             String versionString = buffer.getNextString();
-            currentOffset += (FTRACE_VERSION_SIZE + BinaryFTraceHeaderElementSize.STRING_TERMINATOR);
+            currentOffset += (BinaryFTraceHeaderElementSize.FTRACE_VERSION + BinaryFTraceHeaderElementSize.STRING_TERMINATOR);
             assertEquals(versionString, VERSION);
             assertEquals(buffer.getCurrentOffset(), currentOffset);
 

diff --git a/...se/tracecompass/incubator/ftrace/core/tests/binary/iterator/BinaryFTraceIteratorTest.java b/...se/tracecompass/incubator/ftrace/core/tests/binary/iterator/BinaryFTraceIteratorTest.java
@@ -14,7 +14,6 @@
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
-import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
 
 import java.io.IOException;
@@ -40,10 +39,7 @@
  * @author Hoang Thuan Pham
  */
 public class BinaryFTraceIteratorTest {
-    // Trace with 4 CPUs
     private static BinaryFTraceHeaderInfo multipleEventTrace;
-    // An empty trace that is recorded on a machine that has 4 CPUs
-    private static BinaryFTraceHeaderInfo emptyTrace;
 
     private static BinaryFTrace ftrace;
 
@@ -59,7 +55,6 @@ public class BinaryFTraceIteratorTest {
     public static void initTest() throws TmfTraceException, IOException {
         ftrace = new BinaryFTrace();
         multipleEventTrace = BinaryFTraceFileParser.parse(FTraceUtils.getTraceAbsolutePath(FtraceTestTrace.TEST_2_6_MULTIPLE_CPUS));
-        emptyTrace = BinaryFTraceFileParser.parse(FTraceUtils.getTraceAbsolutePath(FtraceTestTrace.TEST_2_6_EMPTY));
     }
 
     /**
@@ -100,13 +95,6 @@ public void constructorTestWithNoLocation() throws IOException {
      */
     @Test
     public void testHasMoreEvents() throws IOException {
-        // This trace has no events, so has more events returns false
-        assertNotNull(ftrace);
-        try (BinaryFTraceIterator iterator = new BinaryFTraceIterator(emptyTrace, ftrace)) {
-            assertFalse(iterator.hasMoreEvents());
-        }
-
-        // This trace has multiple events, so has more events returns true
         assertNotNull(ftrace);
         try (BinaryFTraceIterator iterator = new BinaryFTraceIterator(multipleEventTrace, ftrace)) {
             assertTrue(iterator.hasMoreEvents());
@@ -127,12 +115,6 @@ public void testGetCurrentEvent() throws IOException {
             assertEquals(event.getName(), BinaryFTraceIteratorTestData.FIRST_TRACE_EVENT_NAME);
             assertEquals(event.getTimestamp().toNanos(), BinaryFTraceIteratorTestData.FIRST_TRACE_EVENT_TS);
         }
-
-        assertNotNull(ftrace);
-        try (BinaryFTraceIterator iterator = new BinaryFTraceIterator(emptyTrace, ftrace)) {
-            GenericFtraceEvent event = iterator.getCurrentEvent();
-            assertNull(event);
-        }
     }
 
     /**

diff --git a/...ipse/tracecompass/incubator/ftrace/core/tests/binary/iterator/BinaryFTraceReaderTest.java b/...ipse/tracecompass/incubator/ftrace/core/tests/binary/iterator/BinaryFTraceReaderTest.java
@@ -32,9 +32,7 @@
  * @author Hoang Thuan Pham
  */
 public class BinaryFTraceReaderTest {
-    private static BinaryFTraceHeaderInfo multipleEventTrace; // trace with 4
-                                                              // CPU
-    private static BinaryFTraceHeaderInfo emptyTrace; // empty trace file
+    private static BinaryFTraceHeaderInfo multipleEventTrace;
 
     /**
      * Initialize data for the test
@@ -45,7 +43,6 @@ public class BinaryFTraceReaderTest {
     @BeforeClass
     public static void initTest() throws Exception {
         multipleEventTrace = BinaryFTraceFileParser.parse(FTraceUtils.getTraceAbsolutePath(FtraceTestTrace.TEST_2_6_MULTIPLE_CPUS));
-        emptyTrace = BinaryFTraceFileParser.parse(FTraceUtils.getTraceAbsolutePath(FtraceTestTrace.TEST_2_6_EMPTY));
     }
 
     /**
@@ -110,10 +107,6 @@ public void testGetStartTimeAndEndTime() throws Exception {
      */
     @Test
     public void testHasMoreEvents() throws Exception {
-        try (BinaryFTraceReader reader = new BinaryFTraceReader(emptyTrace)) {
-            assertFalse(reader.hasMoreEvents());
-        }
-
         try (BinaryFTraceReader reader = new BinaryFTraceReader(multipleEventTrace)) {
             // The first event in trace
             assertTrue(reader.hasMoreEvents());

diff --git a/.../tracecompass/incubator/ftrace/core/tests/binary/security/BinaryFTraceFileParserTest.java b/.../tracecompass/incubator/ftrace/core/tests/binary/security/BinaryFTraceFileParserTest.java
@@ -0,0 +1,68 @@
+/*******************************************************************************
+ * Copyright (c) 2022 Ericsson
+ *
+ * All rights reserved. This program and the accompanying materials are made
+ * available under the terms of the Eclipse Public License 2.0 which
+ * accompanies this distribution, and is available at
+ * https://www.eclipse.org/legal/epl-2.0/
+ *
+ * SPDX-License-Identifier: EPL-2.0
+ *******************************************************************************/
+
+package org.eclipse.tracecompass.incubator.ftrace.core.tests.binary.security;
+
+import java.io.IOException;
+
+import org.eclipse.tracecompass.incubator.ftrace.core.tests.shared.FTraceUtils;
+import org.eclipse.tracecompass.incubator.internal.ftrace.core.binary.parser.BinaryFTraceFileParser;
+import org.eclipse.tracecompass.testtraces.ftrace.FtraceTestTrace;
+import org.eclipse.tracecompass.tmf.core.exceptions.TmfTraceException;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+/**
+ * Perform various security tests for the BinaryFTraceFileParserImp
+ *
+ * @author Hoang Thuan Pham
+ */
+public class BinaryFTraceFileParserTest {
+    private static String injectedTraceHeaderPageSection;
+    private static String injectedTraceCPU;
+
+    /**
+     * Initialize the paths to the trace files
+     *
+     * @throws IOException
+     *             If an error occurred while getting the trace paths
+     */
+    @BeforeClass
+    public static void init() throws IOException {
+        injectedTraceHeaderPageSection = FTraceUtils.getTraceAbsolutePath(FtraceTestTrace.TEST_2_6_BAD_HEADER_PAGE_SECTION_SIZE);
+        injectedTraceCPU = FTraceUtils.getTraceAbsolutePath(FtraceTestTrace.TEST_2_6_BAD_CPU_SECTION_SIZE);
+    }
+
+    /**
+     * A03 Injection
+     */
+    /**
+     * Test parsing a trace injected with a bad Header Page section size
+     *
+     * @throws TmfTraceException
+     *             If the parser fails to parse the trace
+     */
+    @Test(expected = TmfTraceException.class)
+    public void testBadHeaderPageSectionSize() throws TmfTraceException {
+        BinaryFTraceFileParser.parse(injectedTraceHeaderPageSection);
+    }
+
+    /**
+     * Test parsing a trace injected with a bad CPU section size
+     *
+     * @throws TmfTraceException
+     *             If the parser fails to parse the trace
+     */
+    @Test(expected = TmfTraceException.class)
+    public void testBadCpuSectionSize() throws TmfTraceException {
+        BinaryFTraceFileParser.parse(injectedTraceCPU);
+    }
+}
diff --git a/...lipse/tracecompass/incubator/internal/ftrace/core/binary/event/BinaryFTraceConstants.java b/...lipse/tracecompass/incubator/internal/ftrace/core/binary/event/BinaryFTraceConstants.java
@@ -157,6 +157,14 @@ private BinaryFTraceConstants() {
     public static final String FUNCTION_ADDRESS_NAME_SEPARATOR = " "; //$NON-NLS-1$
 
     // Constants to parse the trace
+    /**
+     * The header of the Header Event section.
+     */
+    public static final String HEADER_EVENT_SECTION_HEADER = "header_event"; //$NON-NLS-1$
+    /**
+     * The header of the Header Page section.
+     */
+    public static final String HEADER_PAGE_SECTION_HEADER = "header_page"; //$NON-NLS-1$
     /**
      * Label for each trace file option.
      */

diff --git a/...pse/tracecompass/incubator/internal/ftrace/core/binary/parser/BinaryFTraceByteBuffer.java b/...pse/tracecompass/incubator/internal/ftrace/core/binary/parser/BinaryFTraceByteBuffer.java
@@ -20,8 +20,6 @@
 
 import org.eclipse.tracecompass.incubator.internal.ftrace.core.binary.header.BinaryFTraceDataType;
 
-import com.google.common.annotations.VisibleForTesting;
-
 /**
  * A reader for Ftrace files that utilizes ByteBuffer
  *
@@ -258,8 +256,18 @@ public void movePointerToOffset(long offset) throws IOException {
      *
      * @return The current offset of the file pointer
      */
-    @VisibleForTesting
     public long getCurrentOffset() {
         return fCurrentOffset;
     }
+
+    /**
+     * Get the size of the file that is currently being read
+     *
+     * @return The file size
+     * @throws IOException
+     *             If an error occurred while reading the file size
+     */
+    public long getFileSize() throws IOException {
+        return fTraceFile.length();
+    }
 }