From a60dd245331d5935cd2f0986d97e1dbae45aa85a Mon Sep 17 00:00:00 2001 From: Carlos Ruiz Date: Sun, 18 Oct 2020 05:04:54 +0200 Subject: [PATCH] IDEMPIERE-4495 github code scanning alerts (#308) * IDEMPIERE-4495 github code scanning alerts Failure to use secure cookies * Failure to use secure cookies - one more * Fix: Arbitrary file write during archive extraction ("Zip Slip") * Fix: Resolving XML external entity in user-controlled data --- .../src/com/akunagroup/uk/postcode/AddressLookup.java | 4 ++-- .../src/org/compiere/model/AttachmentFileSystem.java | 4 ++-- org.adempiere.base/src/org/compiere/util/WebUtil.java | 2 ++ org.adempiere.pipo/src/org/adempiere/pipo2/PackIn.java | 7 +++++-- .../org/adempiere/webui/install/WTranslationDialog.java | 7 ++++++- 5 files changed, 17 insertions(+), 7 deletions(-) diff --git a/org.adempiere.base/src/com/akunagroup/uk/postcode/AddressLookup.java b/org.adempiere.base/src/com/akunagroup/uk/postcode/AddressLookup.java index 9238806926..d1b7235288 100644 --- a/org.adempiere.base/src/com/akunagroup/uk/postcode/AddressLookup.java +++ b/org.adempiere.base/src/com/akunagroup/uk/postcode/AddressLookup.java @@ -335,8 +335,8 @@ public class AddressLookup implements AddressLookupInterface { private Document fetchResult(URL cgiUrl) { try { // Get document builder. - DocumentBuilderFactory docBuilderFactory = DocumentBuilderFactory - .newInstance(); + DocumentBuilderFactory docBuilderFactory = DocumentBuilderFactory.newInstance(); + docBuilderFactory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); DocumentBuilder docBuilder = docBuilderFactory.newDocumentBuilder(); // Get the connection. URLConnection URLconnection = cgiUrl.openConnection(); diff --git a/org.adempiere.base/src/org/compiere/model/AttachmentFileSystem.java b/org.adempiere.base/src/org/compiere/model/AttachmentFileSystem.java index b50404260f..9990ae04f5 100644 --- a/org.adempiere.base/src/org/compiere/model/AttachmentFileSystem.java +++ b/org.adempiere.base/src/org/compiere/model/AttachmentFileSystem.java @@ -170,9 +170,9 @@ public class AttachmentFileSystem implements IAttachmentStore { if (data.length == 0) return true; - DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); - try { + DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); + factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); final DocumentBuilder builder = factory.newDocumentBuilder(); final Document document = builder.parse(new ByteArrayInputStream(data)); final NodeList entries = document.getElementsByTagName("entry"); diff --git a/org.adempiere.base/src/org/compiere/util/WebUtil.java b/org.adempiere.base/src/org/compiere/util/WebUtil.java index e2c895a21e..15f3ce0006 100644 --- a/org.adempiere.base/src/org/compiere/util/WebUtil.java +++ b/org.adempiere.base/src/org/compiere/util/WebUtil.java @@ -941,6 +941,7 @@ public final class WebUtil cookie.setComment("adempiere Web User"); cookie.setPath(request.getContextPath()); cookie.setMaxAge(1); // second + cookie.setSecure(true); response.addCookie(cookie); } // deleteCookieWebUser @@ -970,6 +971,7 @@ public final class WebUtil cookie.setComment("adempiere Web User"); cookie.setPath(request.getContextPath()); cookie.setMaxAge(2592000); // 30 days in seconds 60*60*24*30 + cookie.setSecure(true); response.addCookie(cookie); } catch (UnsupportedEncodingException e) { e.printStackTrace(); diff --git a/org.adempiere.pipo/src/org/adempiere/pipo2/PackIn.java b/org.adempiere.pipo/src/org/adempiere/pipo2/PackIn.java index 4edef89d52..54b53b56d1 100644 --- a/org.adempiere.pipo/src/org/adempiere/pipo2/PackIn.java +++ b/org.adempiere.pipo/src/org/adempiere/pipo2/PackIn.java @@ -246,7 +246,10 @@ public class PackIn { try{ while (e.hasMoreElements()) { ZipEntry ze = (ZipEntry) e.nextElement(); - File file = new File(m_packageDirectory + File.separator + ze.getName()); + File file = new File(m_packageDirectory, ze.getName()); + if (!file.toPath().normalize().startsWith(m_packageDirectory)) { + throw new AdempiereException("Bad zip entry: " + ze.getName()); + } FileOutputStream fout = new FileOutputStream(file); InputStream in = zf.getInputStream(ze); for (int c = in.read(); c != -1; c = in.read()) { @@ -258,7 +261,7 @@ public class PackIn { } retValue = new File[files.size()]; files.toArray(retValue); - }catch (Exception ex){ + } finally { zf.close(); } return retValue; diff --git a/org.adempiere.ui.zk/WEB-INF/src/org/adempiere/webui/install/WTranslationDialog.java b/org.adempiere.ui.zk/WEB-INF/src/org/adempiere/webui/install/WTranslationDialog.java index 0c35c9c78f..5a1036e2e6 100644 --- a/org.adempiere.ui.zk/WEB-INF/src/org/adempiere/webui/install/WTranslationDialog.java +++ b/org.adempiere.ui.zk/WEB-INF/src/org/adempiere/webui/install/WTranslationDialog.java @@ -396,9 +396,14 @@ public class WTranslationDialog extends TranslationController implements IFormCo log.warning("Ignored file " + entry.getName()); continue; } + File outFile = new File(tempfolder.getPath(), entry.getName()); + if (!outFile.toPath().normalize().startsWith(tempfolder.toPath())) { + log.severe("Bad zip entry: " + entry.getName()); + continue; + } if (log.isLoggable(Level.INFO)) log.info("Extracting file: " + entry.getName()); - copyInputStream(zipFile.getInputStream(entry), new BufferedOutputStream(new FileOutputStream(tempfolder.getPath() + File.separator + entry.getName()))); + copyInputStream(zipFile.getInputStream(entry), new BufferedOutputStream(new FileOutputStream(outFile))); validfile = true; } } catch (Throwable e) {