Skip to content

Commit

Permalink
Merge pull request #10 from LegalSifter/feature/fix-docx4j-vulnerabil…
Browse files Browse the repository at this point in the history 
…ities-#184540251

Feature/fix docx4j vulnerabilities #184540251
  • Loading branch information
@TheDanDLion
TheDanDLion authored Sep 28, 2023
2 parents dc4ae75 + a0947c7 commit a0f9f59
Show file tree
Hide file tree
Showing 22 changed files with 208 additions and 88 deletions.
4 changes: 2 additions & 2 deletions docx4j-core/src/main/java/org/docx4j/com/google/common/cache/Striped64.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@

package org.docx4j.com.google.common.cache;

import java.util.Random;
import java.security.SecureRandom;
import org.checkerframework.checker.nullness.qual.Nullable;
import org.docx4j.com.google.common.annotations.GwtIncompatible;

Expand Down Expand Up @@ -128,7 +128,7 @@ final boolean cas(long cmp, long val) {
static final ThreadLocal<int[]> threadHashCode = new ThreadLocal<>();

/** Generator of new random hash codes */
static final Random rng = new Random();
static final SecureRandom rng = new SecureRandom();

/** Number of CPUS, to place bound on table size */
static final int NCPU = Runtime.getRuntime().availableProcessors();
Expand Down
108 changes: 58 additions & 50 deletions docx4j-core/src/main/java/org/docx4j/fonts/fop/fonts/FontCache.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -157,37 +157,41 @@ public static FontCache load() {
* file exists or if it could not be read)
*/
public static FontCache loadFrom(File cacheFile) {
if (cacheFile.exists()) {
try {
if (log.isTraceEnabled()) {
log.trace("Loading font cache from "
+ cacheFile.getCanonicalPath());
}
InputStream in = new BufferedInputStream(new FileInputStream(cacheFile));
ObjectInputStream oin = new ObjectInputStream(in);
try {
return (FontCache) oin.readObject();
} finally {
IOUtils.closeQuietly(oin);
}
} catch (ClassNotFoundException e) {
// We don't really care about the exception since it's just a
// cache file
log.warn("Could not read font cache. Discarding font cache file. Reason: "
+ e.getMessage());
} catch (IOException ioe) {
// We don't really care about the exception since it's just a
// cache file
log.warn("I/O exception while reading font cache ("
+ ioe.getMessage() + "). Discarding font cache file.");
try {
cacheFile.delete();
} catch (SecurityException ex) {
log.warn("Failed to delete font cache file: "
+ cacheFile.getAbsolutePath());
}
}
}
// disabling cache: https://stackoverflow.com/questions/39228918/java-deserialization-of-untrusted-data-workaround
// TODO: follow controls here: https://owasp.org/www-community/vulnerabilities/Deserialization_of_untrusted_data
// ideally, we should use Java Cryptography Extension and/or Java Cryptography Architecture to sign the files
// if (cacheFile.exists()) {
// try {
// if (log.isTraceEnabled()) {
// log.trace("Loading font cache from "
// + cacheFile.getCanonicalPath());
// }
// InputStream in = new BufferedInputStream(new FileInputStream(cacheFile));
// ObjectInputStream oin = new ObjectInputStream(in);
// try {
// return (FontCache) oin.readObject();
// } finally {
// IOUtils.closeQuietly(oin);
// }
// } catch (ClassNotFoundException e) {
// // We don't really care about the exception since it's just a
// // cache file
// log.warn("Could not read font cache. Discarding font cache file. Reason: "
// + e.getMessage());
// } catch (IOException ioe) {
// // We don't really care about the exception since it's just a
// // cache file
// log.warn("I/O exception while reading font cache ("
// + ioe.getMessage() + "). Discarding font cache file.");
// try {
// cacheFile.delete();
// } catch (SecurityException ex) {
// log.warn("Failed to delete font cache file: "
// + cacheFile.getAbsolutePath());
// }
// }
// }
log.warn("Cache is disabled");
return null;
}

Expand All @@ -210,25 +214,29 @@ public void save() throws FOPException {
* fop exception
*/
public void saveTo(File cacheFile) throws FOPException {
synchronized (changeLock) {
if (changed) {
try {
log.trace("Writing font cache to " + cacheFile.getCanonicalPath());
OutputStream out = new java.io.FileOutputStream(cacheFile);
out = new java.io.BufferedOutputStream(out);
ObjectOutputStream oout = new ObjectOutputStream(out);
try {
oout.writeObject(this);
} finally {
IOUtils.closeQuietly(oout);
}
} catch (IOException ioe) {
LogUtil.handleException(log, ioe, true);
}
changed = false;
log.trace("Cache file written.");
}
}
// disabling cache: https://stackoverflow.com/questions/39228918/java-deserialization-of-untrusted-data-workaround
// TODO: follow controls here: https://owasp.org/www-community/vulnerabilities/Deserialization_of_untrusted_data
// ideally, we should use Java Cryptography Extension and/or Java Cryptography Architecture to sign the files
// synchronized (changeLock) {
// if (changed) {
// try {
// log.trace("Writing font cache to " + cacheFile.getCanonicalPath());
// OutputStream out = new java.io.FileOutputStream(cacheFile);
// out = new java.io.BufferedOutputStream(out);
// ObjectOutputStream oout = new ObjectOutputStream(out);
// try {
// oout.writeObject(this);
// } finally {
// IOUtils.closeQuietly(oout);
// }
// } catch (IOException ioe) {
// LogUtil.handleException(log, ioe, true);
// }
// changed = false;
// log.trace("Cache file written.");
// }
// }
log.warn("Cache is disabled");
}

/**
Expand Down
1 change: 1 addition & 0 deletions docx4j-core/src/main/java/org/docx4j/openpackaging/contenttype/ContentTypeManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -844,6 +844,7 @@ public void parseContentTypesFile(InputStream contentTypes)
XMLInputFactory xif = XMLInputFactory.newInstance();
xif.setProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, false);
xif.setProperty(XMLInputFactory.SUPPORT_DTD, false); // a DTD is merely ignored, its presence doesn't cause an exception
xif.setProperty(XMLInputFactory.IS_REPLACING_ENTITY_REFERENCES, false);
XMLStreamReader xsr = xif.createXMLStreamReader(contentTypes);

Unmarshaller u = Context.jcContentTypes.createUnmarshaller();
Expand Down
2 changes: 2 additions & 0 deletions docx4j-core/src/main/java/org/docx4j/openpackaging/io/Load.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.docx4j.XmlUtils;
import org.docx4j.convert.out.fopconf.Substitutions.Substitution.To;
import org.docx4j.docProps.coverPageProps.CoverPageProperties;
import org.docx4j.jaxb.Context;
import org.docx4j.model.datastorage.BindingHandler;
Expand Down Expand Up @@ -187,6 +188,7 @@ public static Part getRawPart(InputStream is, ContentTypeManager ctm, String res
XMLInputFactory xif = XMLInputFactory.newInstance();
xif.setProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, false);
xif.setProperty(XMLInputFactory.SUPPORT_DTD, false); // a DTD is merely ignored, its presence doesn't cause an exception
xif.setProperty(XMLInputFactory.IS_REPLACING_ENTITY_REFERENCES, false);
XMLStreamReader xsr = xif.createXMLStreamReader(is);

Unmarshaller u = Context.jc.createUnmarshaller();
Expand Down
6 changes: 6 additions & 0 deletions docx4j-core/src/main/java/org/docx4j/openpackaging/io/LoadFromZipNG.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@

import org.apache.commons.io.IOUtils;
import org.docx4j.XmlUtils;
import org.docx4j.convert.out.fopconf.Substitutions.Substitution.To;
import org.docx4j.docProps.coverPageProps.CoverPageProperties;
import org.docx4j.jaxb.Context;
import org.docx4j.model.datastorage.CustomXmlDataStorage;
Expand Down Expand Up @@ -74,6 +75,7 @@
import org.docx4j.openpackaging.parts.relationships.Namespaces;
import org.docx4j.openpackaging.parts.relationships.RelationshipsPart;
import org.docx4j.relationships.Relationship;
import org.docx4j.utils.StringUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand Down Expand Up @@ -117,6 +119,9 @@ public LoadFromZipNG() {


public OpcPackage get(String filepath) throws Docx4JException {
if (!StringUtils.validFilePath(filepath)) {
throw new Docx4JException("Invalid filepath, filepath in LoadFromZipNG contains characters that could be used for directory traversal");
}
return get(new File(filepath));
}

Expand Down Expand Up @@ -623,6 +628,7 @@ public static Part getRawPart(HashMap<String, ByteArray> partByteArrays,
XMLInputFactory xif = XMLInputFactory.newInstance();
xif.setProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, false);
xif.setProperty(XMLInputFactory.SUPPORT_DTD, false); // a DTD is merely ignored, its presence doesn't cause an exception
xif.setProperty(XMLInputFactory.IS_REPLACING_ENTITY_REFERENCES, false);
XMLStreamReader xsr = xif.createXMLStreamReader(is);

Unmarshaller u = Context.jc.createUnmarshaller();
Expand Down
11 changes: 11 additions & 0 deletions docx4j-core/src/main/java/org/docx4j/openpackaging/io/SaveToZipFile.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@
import org.docx4j.openpackaging.parts.relationships.Namespaces;
import org.docx4j.openpackaging.parts.relationships.RelationshipsPart;
import org.docx4j.relationships.Relationship;
import org.docx4j.utils.StringUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.w3c.dom.Document;
Expand Down Expand Up @@ -201,6 +202,10 @@ public void saveRawXmlPart(ZipOutputStream out, Part part) throws Docx4JExcepti

public void saveRawXmlPart(ZipOutputStream out, Part part, String zipEntryName) throws Docx4JException {

if(!StringUtils.validFilePath(zipEntryName)) {
throw new Docx4JException("Invalid zipEntryName, zipEntryName in SaveToZipFile/saveRawXmlPart contains characters that could be used for directory traversal");
}

try {

if (part instanceof org.docx4j.openpackaging.parts.JaxbXmlPart) {
Expand Down Expand Up @@ -419,6 +424,9 @@ public void savePart(ZipOutputStream out, Part part)

// Drop the leading '/'
String resolvedPartUri = part.getPartName().getName().substring(1);
if(!StringUtils.validFilePath(resolvedPartUri)) {
throw new Docx4JException("Invalid path, path in SaveToZipFile/savePart contains characters that could be used for directory traversal");
}

if (handled.get(resolvedPartUri)!=null) {
log.debug(".. duplicate save avoided .." );
Expand Down Expand Up @@ -461,6 +469,9 @@ protected void saveRawBinaryPart(ZipOutputStream out, Part part) throws Docx4JEx

// Drop the leading '/'
String resolvedPartUri = part.getPartName().getName().substring(1);
if (!StringUtils.validFilePath(resolvedPartUri)) {
throw new Docx4JException("Invalid filepath, resolvedPartUri in SaveToZipFile/saveRawBinaryPart contains characters that could be used for directory traversal");
}

try {
// Add ZIP entry to output stream.
Expand Down
2 changes: 2 additions & 0 deletions docx4j-core/src/main/java/org/docx4j/openpackaging/io3/Load3.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@

import org.apache.commons.io.IOUtils;
import org.docx4j.XmlUtils;
import org.docx4j.convert.out.fopconf.Substitutions.Substitution.To;
import org.docx4j.docProps.coverPageProps.CoverPageProperties;
import org.docx4j.jaxb.Context;
import org.docx4j.model.datastorage.CustomXmlDataStorage;
Expand Down Expand Up @@ -508,6 +509,7 @@ public Part getRawPart(
XMLInputFactory xif = XMLInputFactory.newInstance();
xif.setProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, false);
xif.setProperty(XMLInputFactory.SUPPORT_DTD, false); // a DTD is merely ignored, its presence doesn't cause an exception
xif.setProperty(XMLInputFactory.IS_REPLACING_ENTITY_REFERENCES, false);
XMLStreamReader xsr = xif.createXMLStreamReader(is);

Unmarshaller u = Context.jc.createUnmarshaller();
Expand Down
29 changes: 27 additions & 2 deletions docx4j-core/src/main/java/org/docx4j/openpackaging/io3/stores/UnzippedPartStore.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
import org.docx4j.openpackaging.parts.PartName;
import org.docx4j.openpackaging.parts.XmlPart;
import org.docx4j.openpackaging.parts.WordprocessingML.BinaryPart;
import org.docx4j.utils.StringUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.w3c.dom.Document;
Expand Down Expand Up @@ -108,8 +109,9 @@ public void setSourcePartStore(PartStore partStore) {
public InputStream loadPart(String partName) throws Docx4JException {

String filePath = dir.getPath() + dir.separator + partName;

System.out.println("Using " + filePath);
if (!StringUtils.validFilePath(filePath)) {
throw new Docx4JException("Invalid filepath, filepath in UnzippedPartStore/loadPart contains characters that could be used for directory traversal");
}

InputStream is;
try {
Expand All @@ -126,6 +128,9 @@ public InputStream loadPart(String partName) throws Docx4JException {
public long getPartSize(String partName) throws Docx4JException {

String filePath = dir.getPath() + dir.separator + partName;
if (!StringUtils.validFilePath(filePath)) {
throw new Docx4JException("Invalid filepath, filepath in UnzippedPartStore/getPartSize contains characters that could be used for directory traversal");
}

File f = new File(filePath);
if (f.exists()) {
Expand All @@ -145,6 +150,10 @@ public void rename(PartName oldName, PartName newName) {
log.info("Renaming part " + oldName.getName() + " to " + newName.getName() );

String filePath = dir.getPath() + dir.separator + oldName.getName();
if (!StringUtils.validFilePath(filePath)) {
log.error("Invalid filepath, filepath in UnzippedPartStore/rename contains characters that could be used for directory traversal; not renaming");
return;
}
File f = new File(filePath);
f.renameTo(new File(dir.getPath() + dir.separator + newName.getName()) );

Expand All @@ -164,6 +173,9 @@ public void saveContentTypes(ContentTypeManager ctm) throws Docx4JException {
try {

String filePath = dir.getPath() + dir.separator + "[Content_Types].xml";
if (!StringUtils.validFilePath(filePath)) {
throw new Docx4JException("Invalid filepath, filepath in UnzippedPartStore/saveContentTypes contains characters that could be used for directory traversal");
}
FileOutputStream fos = new FileOutputStream(new File(filePath));
ctm.marshal(fos);
fos.close();
Expand All @@ -184,6 +196,9 @@ public void saveJaxbXmlPart(JaxbXmlPart part) throws Docx4JException {
}

String filePath = dir.getPath() + dir.separator + targetName;
if (!StringUtils.validFilePath(filePath)) {
throw new Docx4JException("Invalid filepath, filepath in UnzippedPartStore/saveJaxbXmlPart contains characters that could be used for directory traversal");
}
System.out.println("Saving " + filePath);
try {

Expand Down Expand Up @@ -231,6 +246,9 @@ public void saveCustomXmlDataStoragePart(CustomXmlDataStoragePart part) throws D
String targetName = part.getPartName().getName().substring(1);

String filePath = dir.getPath() + dir.separator + targetName;
if (!StringUtils.validFilePath(filePath)) {
throw new Docx4JException("Invalid filepath, filepath in UnzippedPartStore/saveCustomXmlDataStoragePart contains characters that could be used for directory traversal");
}

File file = new File(filePath);
file.getParentFile().mkdirs();
Expand All @@ -252,6 +270,10 @@ public void saveXmlPart(XmlPart part) throws Docx4JException {
String targetName = part.getPartName().getName().substring(1);

String filePath = dir.getPath() + dir.separator + targetName;
if (!StringUtils.validFilePath(filePath)) {
throw new Docx4JException("Invalid filepath, filepath in UnzippedPartStore/saveXmlPart contains characters that could be used for directory traversal");
}

File file = new File(filePath);
file.getParentFile().mkdirs();

Expand Down Expand Up @@ -291,6 +313,9 @@ public void saveBinaryPart(Part part) throws Docx4JException {
// Drop the leading '/'
String resolvedPartUri = part.getPartName().getName().substring(1);
String filePath = dir.getPath() + dir.separator + resolvedPartUri;
if (!StringUtils.validFilePath(filePath)) {
throw new Docx4JException("Invalid filepath, filepath in UnzippedPartStore/saveBinaryPart contains characters that could be used for directory traversal");
}
System.out.println("saveBinaryPart " + filePath);

File file = new File(filePath);
Expand Down
2 changes: 2 additions & 0 deletions docx4j-core/src/main/java/org/docx4j/openpackaging/parts/JaxbXmlPart.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,7 @@ public void transform(Templates xslt,
XMLInputFactory xif = XMLInputFactory.newInstance();
xif.setProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, false);
xif.setProperty(XMLInputFactory.SUPPORT_DTD, false); // a DTD is merely ignored, its presence doesn't cause an exception
xif.setProperty(XMLInputFactory.IS_REPLACING_ENTITY_REFERENCES, false);
XMLStreamReader xsr = xif.createXMLStreamReader(is);
XmlUtils.transform(new StAXSource(xsr), xslt, transformParameters, result);

Expand Down Expand Up @@ -1191,6 +1192,7 @@ protected XMLStreamReader inputStreamToXSR(InputStream is) throws XMLStreamExcep
XMLInputFactory xif = XMLInputFactory.newInstance();
xif.setProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, false);
xif.setProperty(XMLInputFactory.SUPPORT_DTD, false); // a DTD is merely ignored, its presence doesn't cause an exception
xif.setProperty(XMLInputFactory.IS_REPLACING_ENTITY_REFERENCES, false);
return xif.createXMLStreamReader(is);

}
Expand Down
6 changes: 3 additions & 3 deletions docx4j-core/src/main/java/org/docx4j/openpackaging/parts/PresentationML/JaxbPmlPart.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@

package org.docx4j.openpackaging.parts.PresentationML;

import java.util.Random;
import java.security.SecureRandom;

import org.docx4j.openpackaging.contenttype.ContentTypes;
import org.docx4j.openpackaging.exceptions.InvalidFormatException;
Expand Down Expand Up @@ -60,8 +60,8 @@ public abstract class JaxbPmlPart<E> extends JaxbXmlPartXPathAware<E> {

protected final static String COLOR_MAPPING = "<p:clrMap xmlns:p=\"http://schemas.openxmlformats.org/presentationml/2006/main\" bg1=\"lt1\" tx1=\"dk1\" bg2=\"lt2\" tx2=\"dk2\" accent1=\"accent1\" accent2=\"accent2\" accent3=\"accent3\" accent4=\"accent4\" accent5=\"accent5\" accent6=\"accent6\" hlink=\"hlink\" folHlink=\"folHlink\"/>";

protected static Random random = new Random();
protected static SecureRandom random = new SecureRandom();

public static long getSlideLayoutOrMasterId() {
// See spec 4.8.18 (ST_SlideLayoutId) and 4.8.20 (ST_SlideMasterId)
long val = random.nextInt(2147483647) + 2147483648l;
Expand Down
1 change: 1 addition & 0 deletions ...4j-core/src/main/java/org/docx4j/openpackaging/parts/relationships/RelationshipsPart.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -964,6 +964,7 @@ public Relationships unmarshal( java.io.InputStream is ) throws JAXBException {
XMLInputFactory xif = XMLInputFactory.newInstance();
xif.setProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, false);
xif.setProperty(XMLInputFactory.SUPPORT_DTD, false); // a DTD is merely ignored, its presence doesn't cause an exception
xif.setProperty(XMLInputFactory.IS_REPLACING_ENTITY_REFERENCES, false);
XMLStreamReader xsr = xif.createXMLStreamReader(is);

Unmarshaller u = jc.createUnmarshaller();
Expand Down
Loading

0 comments on commit a0f9f59

Please sign in to comment.