From dbdbb0cfe693f8a55aff7f287fc4b7ba071c6e56 Mon Sep 17 00:00:00 2001 From: Dominik Stadler Date: Mon, 20 Jul 2015 14:03:35 +0000 Subject: [PATCH] Bug 58156: Possible data corruption in hasPOIFSHeader and hasOOXMLHeader git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@1691948 13f79535-47bb-0310-9956-ffa450edef68 --- .../poifs/filesystem/NPOIFSFileSystem.java | 10 ++-- src/java/org/apache/poi/util/IOUtils.java | 2 +- .../java/org/apache/poi/POIXMLDocument.java | 5 +- .../org/apache/poi/TestDetectAsOOXML.java | 20 ++++++++ .../TestOffice2007XMLException.java | 47 +++++++++++++++++-- 5 files changed, 74 insertions(+), 10 deletions(-) diff --git a/src/java/org/apache/poi/poifs/filesystem/NPOIFSFileSystem.java b/src/java/org/apache/poi/poifs/filesystem/NPOIFSFileSystem.java index c27ba3cd14..9817c3d818 100644 --- a/src/java/org/apache/poi/poifs/filesystem/NPOIFSFileSystem.java +++ b/src/java/org/apache/poi/poifs/filesystem/NPOIFSFileSystem.java @@ -354,7 +354,11 @@ public class NPOIFSFileSystem extends BlockStore * has a POIFS (OLE2) header at the start of it. * If your InputStream does not support mark / reset, * then wrap it in a PushBackInputStream, then be - * sure to always use that, and not the original! + * sure to always use that and not the original! + * + * After the method call, the InputStream is at the + * same position as of the time of entering the method. + * * @param inp An InputStream which supports either mark/reset, or is a PushbackInputStream */ public static boolean hasPOIFSHeader(InputStream inp) throws IOException { @@ -362,13 +366,13 @@ public class NPOIFSFileSystem extends BlockStore inp.mark(8); byte[] header = new byte[8]; - IOUtils.readFully(inp, header); + int bytesRead = IOUtils.readFully(inp, header); LongField signature = new LongField(HeaderBlockConstants._signature_offset, header); // Wind back those 8 bytes if(inp instanceof PushbackInputStream) { PushbackInputStream pin = (PushbackInputStream)inp; - pin.unread(header); + pin.unread(header, 0, bytesRead); } else { inp.reset(); } diff --git a/src/java/org/apache/poi/util/IOUtils.java b/src/java/org/apache/poi/util/IOUtils.java index f1d5a2378d..b5f22a1a1a 100644 --- a/src/java/org/apache/poi/util/IOUtils.java +++ b/src/java/org/apache/poi/util/IOUtils.java @@ -57,7 +57,7 @@ public final class IOUtils { // Wind back those 8 bytes if(stream instanceof PushbackInputStream) { PushbackInputStream pin = (PushbackInputStream)stream; - pin.unread(header); + pin.unread(header, 0, read); } else { stream.reset(); } diff --git a/src/ooxml/java/org/apache/poi/POIXMLDocument.java b/src/ooxml/java/org/apache/poi/POIXMLDocument.java index 0352d5c874..b0b5d4454f 100644 --- a/src/ooxml/java/org/apache/poi/POIXMLDocument.java +++ b/src/ooxml/java/org/apache/poi/POIXMLDocument.java @@ -127,18 +127,19 @@ public abstract class POIXMLDocument extends POIXMLDocumentPart implements Close inp.mark(4); byte[] header = new byte[4]; - IOUtils.readFully(inp, header); + int bytesRead = IOUtils.readFully(inp, header); // Wind back those 4 bytes if(inp instanceof PushbackInputStream) { PushbackInputStream pin = (PushbackInputStream)inp; - pin.unread(header); + pin.unread(header, 0, bytesRead); } else { inp.reset(); } // Did it match the ooxml zip signature? return ( + bytesRead == 4 && header[0] == POIFSConstants.OOXML_FILE_HEADER[0] && header[1] == POIFSConstants.OOXML_FILE_HEADER[1] && header[2] == POIFSConstants.OOXML_FILE_HEADER[2] && diff --git a/src/ooxml/testcases/org/apache/poi/TestDetectAsOOXML.java b/src/ooxml/testcases/org/apache/poi/TestDetectAsOOXML.java index 4ea2213418..ff4f5dccd8 100644 --- a/src/ooxml/testcases/org/apache/poi/TestDetectAsOOXML.java +++ b/src/ooxml/testcases/org/apache/poi/TestDetectAsOOXML.java @@ -19,8 +19,10 @@ package org.apache.poi; +import java.io.ByteArrayInputStream; import java.io.InputStream; import java.io.PushbackInputStream; +import java.util.Arrays; import junit.framework.TestCase; @@ -62,4 +64,22 @@ public class TestDetectAsOOXML extends TestCase assertFalse(POIXMLDocument.hasOOXMLHeader(in)); in.close(); } + + public void testFileCorruption() throws Exception { + + // create test InputStream + byte[] testData = { (byte)1, (byte)2, (byte)3 }; + ByteArrayInputStream testInput = new ByteArrayInputStream(testData); + + // detect header + InputStream in = new PushbackInputStream(testInput, 10); + assertFalse(POIXMLDocument.hasOOXMLHeader(in)); + + // check if InputStream is still intact + byte[] test = new byte[3]; + in.read(test); + assertTrue(Arrays.equals(testData, test)); + assertEquals(-1, in.read()); + } + } diff --git a/src/testcases/org/apache/poi/poifs/filesystem/TestOffice2007XMLException.java b/src/testcases/org/apache/poi/poifs/filesystem/TestOffice2007XMLException.java index d475b7aad0..d5d92bd6bb 100644 --- a/src/testcases/org/apache/poi/poifs/filesystem/TestOffice2007XMLException.java +++ b/src/testcases/org/apache/poi/poifs/filesystem/TestOffice2007XMLException.java @@ -17,12 +17,16 @@ package org.apache.poi.poifs.filesystem; -import junit.framework.TestCase; - -import java.io.*; +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.io.InputStream; +import java.io.PushbackInputStream; +import java.util.Arrays; import org.apache.poi.hssf.HSSFTestDataSamples; +import junit.framework.TestCase; + /** * Class to test that POIFS complains when given an Office 2007 XML document * @@ -38,7 +42,7 @@ public class TestOffice2007XMLException extends TestCase { InputStream in = openSampleStream("sample.xlsx"); try { - new POIFSFileSystem(in); + new POIFSFileSystem(in).close(); fail("expected exception was not thrown"); } catch(OfficeXmlFileException e) { // expected during successful test @@ -72,4 +76,39 @@ public class TestOffice2007XMLException extends TestCase { in.close(); } } + + public void testFileCorruption() throws Exception { + + // create test InputStream + byte[] testData = { (byte)1, (byte)2, (byte)3 }; + InputStream testInput = new ByteArrayInputStream(testData); + + // detect header + InputStream in = new PushbackInputStream(testInput, 10); + assertFalse(POIFSFileSystem.hasPOIFSHeader(in)); + + // check if InputStream is still intact + byte[] test = new byte[3]; + in.read(test); + assertTrue(Arrays.equals(testData, test)); + assertEquals(-1, in.read()); + } + + + public void testFileCorruptionOPOIFS() throws Exception { + + // create test InputStream + byte[] testData = { (byte)1, (byte)2, (byte)3 }; + InputStream testInput = new ByteArrayInputStream(testData); + + // detect header + InputStream in = new PushbackInputStream(testInput, 10); + assertFalse(OPOIFSFileSystem.hasPOIFSHeader(in)); + + // check if InputStream is still intact + byte[] test = new byte[3]; + in.read(test); + assertTrue(Arrays.equals(testData, test)); + assertEquals(-1, in.read()); + } } -- 2.39.5