Skip to content

Commit

Permalink
TMF: Fix unsafe expanding of archive file
Browse files Browse the repository at this point in the history 
Fix issue for zipFile expanding archive file HotSpots. Related to
[1]'s HotSpots issue.

Unit tests were added to add code coverage to the new method in the
class StandardImportAndReadSmokeTest.

[1] https://sonarcloud.io/organizations/eclipse/rules?open=java%3AS5042&rule_key=java%3AS5042

Change-Id: I87d2275842f2c7f928748f14c3e40de1fe0d1179
Signed-off-by: Estelle Foisy <estelle.foisy@ericsson.com>
Reviewed-on: https://git.eclipse.org/r/c/tracecompass/org.eclipse.tracecompass/+/201026
Reviewed-by: Marco Miller <marco.miller@ericsson.com>
Reviewed-by: Matthew Khouzam <matthew.khouzam@ericsson.com>
Tested-by: Trace Compass Bot <tracecompass-bot@eclipse.org>
Tested-by: Matthew Khouzam <matthew.khouzam@ericsson.com>
  • Loading branch information
@MatthewKhouzam
Estelle Foisy authored and MatthewKhouzam committed Apr 28, 2023
1 parent 1e16a7c commit 4b7b979
Show file tree
Hide file tree
Showing 3 changed files with 105 additions and 11 deletions.
26 changes: 26 additions & 0 deletions .../src/org/eclipse/tracecompass/tmf/ctf/ui/swtbot/tests/StandardImportAndReadSmokeTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@
import org.eclipse.swtbot.swt.finder.widgets.SWTBotTree;
import org.eclipse.swtbot.swt.finder.widgets.SWTBotTreeItem;
import org.eclipse.swtbot.swt.finder.widgets.TimeoutException;
import org.eclipse.tracecompass.internal.tmf.ui.project.wizards.importtrace.ArchiveUtil;
import org.eclipse.tracecompass.internal.tmf.ui.project.wizards.importtrace.ImportConfirmation;
import org.eclipse.tracecompass.internal.tmf.ui.project.wizards.importtrace.ImportTraceWizard;
import org.eclipse.tracecompass.internal.tmf.ui.project.wizards.importtrace.ImportTraceWizardPage;
Expand Down Expand Up @@ -104,6 +105,9 @@ public class StandardImportAndReadSmokeTest extends AbstractImportAndReadSmokeTe

private static final int TEST_OPTION_CHECK_EXPERIMENT = 1 << 15;

private static final int ABOVE_THRESHOLD_ENTRIES = 5001;
private static final int ABOVE_THRESHOLD_SIZE = 1000000001;

/** Test Class setup */
@BeforeClass
public static void beforeClass() {
Expand Down Expand Up @@ -481,6 +485,28 @@ public void testArchiveWithColons() {
SWTBotUtils.clearTracesFolder(fBot, TRACE_PROJECT_NAME);
}

/**
* Test the method verifyZipFileIsSafe with a size argument that is above
* the size threshold permitted.
*/
@Test
public void testArchiveSizeTooBig() {
boolean actual = ArchiveUtil.verifyZipFileIsSafe(ABOVE_THRESHOLD_SIZE, ABOVE_THRESHOLD_ENTRIES - 1);
assertFalse(actual);

}

/**
* Test the method verifyZipFileIsSafe with a number of entries in argument
* that is above the number of entries threshold permitted.
*/
@Test
public void testArchiveTooManyEntries() {
boolean actual = ArchiveUtil.verifyZipFileIsSafe(ABOVE_THRESHOLD_SIZE - 1, ABOVE_THRESHOLD_ENTRIES);
assertFalse(actual);

}

private static void assertNoTraces() {
TmfProjectElement tmfProject = TmfProjectRegistry.getProject(getProjectResource(), true);
assertNotNull(tmfProject);
Expand Down
61 changes: 57 additions & 4 deletions ...src/org/eclipse/tracecompass/internal/tmf/ui/project/wizards/importtrace/ArchiveUtil.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,16 +27,27 @@
import org.eclipse.core.runtime.IPath;
import org.eclipse.core.runtime.Path;
import org.eclipse.swt.widgets.Shell;
import org.eclipse.tracecompass.internal.tmf.ui.Activator;
import org.eclipse.tracecompass.tmf.core.project.model.TmfTraceCoreUtils;
import org.eclipse.tracecompass.tmf.core.util.Pair;
import org.eclipse.ui.wizards.datatransfer.FileSystemStructureProvider;

import com.google.common.annotations.VisibleForTesting;

/**
* Various utilities for dealing with archives in the context of importing
* traces.
*/
public class ArchiveUtil {

/**
* Threshold for size was selected according to SonarCloud suggestion and
* entries were revised to be smaller than SonarCould suggestion:
* https://sonarcloud.io/organizations/eclipse/rules?open=java%3AS5042&rule_key=java%3AS5042&tab=how_to_fix
*/
private static final int THRESHOLD_ENTRIES = 5000;
private static final int THRESHOLD_SIZE = 1000000000;

private ArchiveUtil() {
// Do nothing, not meant to be instantiated
}
Expand Down Expand Up @@ -168,7 +179,12 @@ static boolean closeTarFile(TarFile file) {
}

/**
* Get the archive size
* Get the archive size.
*
* Since an archive entry could also be an archive: a variable counting the
* number of file entries was added to the method as per SonarCloud
* suggestion:
* https://sonarcloud.io/organizations/eclipse/rules?open=java%3AS5042&rule_key=java%3AS5042&tab=how_to_fix
*
* @param archivePath
* Path of the archive
Expand All @@ -188,16 +204,53 @@ public static long getArchiveSize(String archivePath) {

ZipFile zipFile = getSpecifiedZipSourceFile(archivePath);
if (zipFile != null) {
for (Enumeration<? extends ZipEntry> e = zipFile.entries(); e.hasMoreElements();) {
archiveSize += Objects.requireNonNull(e.nextElement()).getSize();
Enumeration<? extends ZipEntry> entries = zipFile.entries();
long archiveEntries = 0;
while (entries.hasMoreElements()) {
archiveSize += Objects.requireNonNull(entries.nextElement()).getSize();
++archiveEntries;
}
closeZipFile(zipFile);
return archiveSize;
// ***SIZE OR LENGTH OF ENTRIES
if (verifyZipFileIsSafe(archiveSize, archiveEntries)) {
return archiveSize;
}
}

return -1;
}

/**
* Verify if zip file is a zip bomb by checking if number of entries is not
* above threshold entries, or if archive uncompressed size is not above
* threshold size.
*
* @param archiveSize
* the uncompressed size of zip file
* @param archiveEntries
* the number of entries in zip file
* @return true if file is within size or number of entries threshold,
* otherwise it returns false
*/
@VisibleForTesting
public static boolean verifyZipFileIsSafe(long archiveSize, long archiveEntries) {
String unsafeZipEntriesMessage = ". File seems unsafe to open."; //$NON-NLS-1$
String unsafeZipSizeMessage = " bytes. File seems unsafe to open."; //$NON-NLS-1$

if (archiveEntries > THRESHOLD_ENTRIES) {
Activator activator = new Activator();
activator.logError("Zip file has too many entries: " + archiveEntries + unsafeZipEntriesMessage); //$NON-NLS-1$
return false;
}

if (archiveSize > THRESHOLD_SIZE) {
Activator activator = new Activator();
activator.logError("Zip file is too large: " + archiveSize + unsafeZipSizeMessage); //$NON-NLS-1$
return false;
}
return true;
}

/**
* Get the root file system object and it's associated import provider for
* the specified source file. A shell is used to display messages in case of
Expand Down
29 changes: 22 additions & 7 deletions ...tracecompass/internal/tmf/ui/project/wizards/importtrace/ZipLeveledStructureProvider.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import org.eclipse.core.runtime.Path;
import org.eclipse.jdt.annotation.Nullable;
import org.eclipse.tracecompass.common.core.NonNullUtils;
import org.eclipse.tracecompass.internal.tmf.ui.Activator;
import org.eclipse.ui.internal.ide.IDEWorkbenchPlugin;
import org.eclipse.ui.internal.wizards.datatransfer.DataTransferMessages;
import org.eclipse.ui.internal.wizards.datatransfer.ILeveledImportStructureProvider;
Expand Down Expand Up @@ -198,9 +199,8 @@ public ZipFile getZipFile() {
return zipFile;
}


@Override
public boolean closeArchive(){
public boolean closeArchive() {
try {
getZipFile().close();
} catch (IOException e) {
Expand All @@ -214,27 +214,42 @@ public boolean closeArchive(){
/**
* Initializes this object's children table based on the contents of the
* specified source file.
*
* Since an archive entry could also be an archive: a variable counting the
* number of file entries was added to the method as per SonarCloud
* suggestion:
* https://sonarcloud.io/organizations/eclipse/rules?open=java%3AS5042&rule_key=java%3AS5042&tab=how_to_fix
*/
protected void initialize() {
children = new HashMap<>(1000);

children.put(root, new ArrayList<>());
Enumeration<? extends ZipEntry> entries = zipFile.entries();
long archiveEntries = 0;
long archiveSize = 0;

while (entries.hasMoreElements()) {
ZipEntry entry = Objects.requireNonNull(entries.nextElement());
IPath path = new Path(entry.getName()).addTrailingSeparator();
archiveSize += entry.getSize();
++archiveEntries;

if (entry.isDirectory()) {
createContainer(path);
} else
{
// Ensure the container structure for all levels above this is initialized
// Once we hit a higher-level container that's already added we need go no further
} else {
// Ensure the container structure for all levels above this is
// initialized. Once we hit a higher-level container that's
// already added we need go no further
int pathSegmentCount = path.segmentCount();
if (pathSegmentCount > 1) {
createContainer(path.uptoSegment(pathSegmentCount - 1));
}
createFile(new ZipArchiveEntry(entry.getName()));
if (ArchiveUtil.verifyZipFileIsSafe(archiveSize, archiveEntries)) {
createFile(new ZipArchiveEntry(entry.getName()));
} else {
Activator activator = new Activator();
activator.logError("Unable to create file, Zip file is unsafe");
}
}
}
}
Expand Down

0 comments on commit 4b7b979

Please sign in to comment.