Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sast security fix #455

Closed
wants to merge 3 commits into from
Closed

Sast security fix #455

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f7893e5
contrast security issue fixes
arunvenmany-ibm Sep 25, 2024
98e16ea
contrast security issue fixes
arunvenmany-ibm Sep 25, 2024
7b80406
rolling back variable names as its causing failure in ci maven and ci…
arunvenmany-ibm Sep 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 18 additions & 12 deletions src/main/java/io/openliberty/tools/common/arquillian/util/HttpPortUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import java.util.regex.Matcher;
import java.util.regex.Pattern;

import javax.xml.XMLConstants;
import javax.xml.parsers.DocumentBuilder;
import javax.xml.parsers.DocumentBuilderFactory;
import javax.xml.parsers.ParserConfigurationException;
Expand All @@ -46,16 +47,21 @@ public class HttpPortUtil {
public static final int DEFAULT_PORT = 9080;
private static final XPath XPATH = XPathFactory.newInstance().newXPath();

private static final DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();
private static boolean factoryInitialized = false;

public static void initDocumentBuilderFactory() throws ParserConfigurationException {
if (!factoryInitialized) {
factory.setNamespaceAware(true);
factory.setFeature("http://apache.org/xml/features/nonvalidating/load-dtd-grammar", false);
factory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
}
}
public static DocumentBuilderFactory getDocumentBuilderFactory() throws ParserConfigurationException {
DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();

factory.setNamespaceAware(true);
factory.setFeature("http://apache.org/xml/features/nonvalidating/load-dtd-grammar", false);
factory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
factory.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
factory.setFeature("http://xml.org/sax/features/external-general-entities", false);
factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
factory.setXIncludeAware(false);
factory.setNamespaceAware(true);
factory.setExpandEntityReferences(false);
return factory;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we not allowed to have a singleton instance of the factory? We were only doing the initialization once before. This can get called many times from maven/gradle plugins. This new code is creating a new one every single time.

}

public static Integer getHttpPort(File serverXML, File bootstrapProperties)
throws FileNotFoundException, IOException, ParserConfigurationException, SAXException,
Expand Down Expand Up @@ -89,7 +95,7 @@ public static Integer getHttpPort(File serverXML, File bootstrapProperties, File

protected static Integer getHttpPortForServerXML(String serverXML, Properties bootstrapProperties, String configVariableXML) throws ParserConfigurationException, SAXException, IOException, XPathExpressionException,
ArquillianConfigurationException {
initDocumentBuilderFactory();
DocumentBuilderFactory factory = getDocumentBuilderFactory();
DocumentBuilder builder = factory.newDocumentBuilder();
Document doc = builder.parse(new ByteArrayInputStream(serverXML.getBytes()));

Expand Down Expand Up @@ -141,7 +147,7 @@ private static String getHttpPortFromConfigVariableXML(String configVariableXML,
}

// get input XML Document
DocumentBuilderFactory inputBuilderFactory = DocumentBuilderFactory.newInstance();
DocumentBuilderFactory inputBuilderFactory = getDocumentBuilderFactory();
inputBuilderFactory.setIgnoringComments(true);
inputBuilderFactory.setCoalescing(true);
inputBuilderFactory.setIgnoringElementContentWhitespace(true);
Expand Down
30 changes: 23 additions & 7 deletions src/main/java/io/openliberty/tools/common/plugins/config/ServerConfigDocument.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import java.util.Map;
import java.util.Properties;

import javax.xml.XMLConstants;
import javax.xml.parsers.DocumentBuilder;
import javax.xml.parsers.DocumentBuilderFactory;
import javax.xml.parsers.ParserConfigurationException;
Expand Down Expand Up @@ -71,6 +72,7 @@ public class ServerConfigDocument {
private static final XPathExpression XPATH_SERVER_ENTERPRISE_APPLICATION;
private static final XPathExpression XPATH_SERVER_INCLUDE;
private static final XPathExpression XPATH_SERVER_VARIABLE;
private static final XPathExpression XPATH_ALL_SERVER_APPLICATIONS;

static {
try {
Expand All @@ -80,6 +82,7 @@ public class ServerConfigDocument {
XPATH_SERVER_ENTERPRISE_APPLICATION = xPath.compile("/server/enterpriseApplication");
XPATH_SERVER_INCLUDE = xPath.compile("/server/include");
XPATH_SERVER_VARIABLE = xPath.compile("/server/variable");
XPATH_ALL_SERVER_APPLICATIONS = xPath.compile("/server/application | /server/webApplication | /server/enterpriseApplication");
} catch (XPathExpressionException ex) {
// These XPath expressions should all compile statically.
// Compilation failures mean the expressions are not syntactically
Expand Down Expand Up @@ -141,7 +144,14 @@ private DocumentBuilder getDocumentBuilder() {
docBuilderFactory.setValidating(false);
try {
docBuilderFactory.setFeature("http://apache.org/xml/features/nonvalidating/load-dtd-grammar", false);
docBuilderFactory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
docBuilderFactory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
docBuilderFactory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
docBuilderFactory.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
docBuilderFactory.setFeature("http://xml.org/sax/features/external-general-entities", false);
docBuilderFactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
docBuilderFactory.setXIncludeAware(false);
docBuilderFactory.setNamespaceAware(true);
docBuilderFactory.setExpandEntityReferences(false);
docBuilder = docBuilderFactory.newDocumentBuilder();
} catch (ParserConfigurationException e) {
// fail catastrophically if we can't create a document builder
Expand Down Expand Up @@ -229,20 +239,20 @@ private void initializeAppsLocation(CommonLoggerI log, File serverXML, File conf
parseApplication(doc, XPATH_SERVER_APPLICATION);
parseApplication(doc, XPATH_SERVER_WEB_APPLICATION);
parseApplication(doc, XPATH_SERVER_ENTERPRISE_APPLICATION);
parseNames(doc, "/server/application | /server/webApplication | /server/enterpriseApplication");
parseNames(doc, XPATH_ALL_SERVER_APPLICATIONS);
parseInclude(doc);
parseConfigDropinsDir();

} catch (Exception e) {
e.printStackTrace();
log.error("Exception while initializing app location " + e.getMessage());
}
}

//Checks for application names in the document. Will add locations without names to a Set
private void parseNames(Document doc, String expression) throws XPathExpressionException, IOException, SAXException {
private void parseNames(Document doc, XPathExpression expression) throws XPathExpressionException, IOException, SAXException {
// parse input document
XPath xPath = XPathFactory.newInstance().newXPath();
NodeList nodeList = (NodeList) xPath.compile(expression).evaluate(doc, XPathConstants.NODESET);
NodeList nodeList = (NodeList) expression
.evaluate(doc, XPathConstants.NODESET);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spacing is weird here


for (int i = 0; i < nodeList.getLength(); i++) {
if (nodeList.item(i).getAttributes().getNamedItem("name") != null) {
Expand Down Expand Up @@ -511,8 +521,14 @@ private Document parseDocument(URL url) throws IOException, SAXException {
}

private Document parseDocument(InputStream in) throws SAXException, IOException {
try (InputStream ins = in) { // ins will be auto-closed
InputStream ins = null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we getting rid of try with resources which is the new better way to ensure streams are closed?

try { // ins will be auto-closed
ins = in;
return getDocumentBuilder().parse(ins);
} finally {
if (ins != null) {
ins.close();
}
}
}

Expand Down
37 changes: 29 additions & 8 deletions src/main/java/io/openliberty/tools/common/plugins/config/XmlDocument.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,16 +16,19 @@
package io.openliberty.tools.common.plugins.config;

import java.io.File;
import java.io.FileOutputStream;
import java.io.IOException;
import java.io.OutputStream;
import java.io.OutputStreamWriter;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;

import javax.xml.XMLConstants;
import javax.xml.parsers.DocumentBuilder;
import javax.xml.parsers.DocumentBuilderFactory;
import javax.xml.parsers.ParserConfigurationException;
import javax.xml.transform.OutputKeys;
import javax.xml.transform.Transformer;
import javax.xml.transform.TransformerConfigurationException;
import javax.xml.transform.TransformerException;
import javax.xml.transform.TransformerFactory;
import javax.xml.transform.dom.DOMSource;
Expand All @@ -52,12 +55,20 @@ public void createDocument(String rootElement) throws ParserConfigurationExcepti

public void createDocument(File xmlFile) throws ParserConfigurationException, SAXException, IOException {
DocumentBuilderFactory builderFactory = DocumentBuilderFactory.newInstance();

builderFactory.setFeature("http://apache.org/xml/features/nonvalidating/load-dtd-grammar", false);
builderFactory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
builderFactory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
builderFactory.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
builderFactory.setFeature("http://xml.org/sax/features/external-general-entities", false);
builderFactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
builderFactory.setXIncludeAware(false);
builderFactory.setNamespaceAware(true);
builderFactory.setExpandEntityReferences(false);
builderFactory.setCoalescing(true);
builderFactory.setIgnoringElementContentWhitespace(true);
builderFactory.setValidating(false);
builderFactory.setFeature("http://apache.org/xml/features/nonvalidating/load-dtd-grammar", false);
builderFactory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
DocumentBuilder builder = builderFactory.newDocumentBuilder();
DocumentBuilder builder = builderFactory.newDocumentBuilder();
doc = builder.parse(xmlFile);
}

Expand All @@ -70,12 +81,12 @@ public void writeXMLDocument(File f) throws IOException, TransformerException {
if (!f.getParentFile().exists()) {
f.getParentFile().mkdirs();
}
FileOutputStream outFile = new FileOutputStream(f);
OutputStream outFile = Files.newOutputStream(f.toPath());

DOMSource source = new DOMSource(doc);
StreamResult result = new StreamResult(outFile);
StreamResult result = new StreamResult(new OutputStreamWriter(outFile, StandardCharsets.UTF_8));

TransformerFactory transformerFactory = TransformerFactory.newInstance();
TransformerFactory transformerFactory = getTransformerFactory();
Transformer transformer = transformerFactory.newTransformer();
transformer.setOutputProperty(OutputKeys.OMIT_XML_DECLARATION, "no");
transformer.setOutputProperty(OutputKeys.DOCTYPE_PUBLIC, "yes");
Expand Down Expand Up @@ -109,4 +120,14 @@ public static void addNewlineBeforeFirstElement(File f) throws IOException {
xmlContents = xmlContents.replace("?><", "?>"+System.getProperty("line.separator")+"<");
Files.write(f.toPath(), xmlContents.getBytes());
}

private static TransformerFactory getTransformerFactory() throws TransformerConfigurationException {
TransformerFactory transformerFactory = TransformerFactory.newInstance();
transformerFactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, Boolean.TRUE);
// XMLConstants.ACCESS_EXTERNAL_DTD uses an empty string to deny all access to external references;
transformerFactory.setAttribute(XMLConstants.ACCESS_EXTERNAL_DTD, "");
// XMLConstants.ACCESS_EXTERNAL_STYLESHEET uses an empty string to deny all access to external references;
transformerFactory.setAttribute(XMLConstants.ACCESS_EXTERNAL_STYLESHEET, "");
return transformerFactory;
}
}
36 changes: 18 additions & 18 deletions src/main/java/io/openliberty/tools/common/plugins/util/BinaryScannerUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,8 @@ public abstract class BinaryScannerUtil {
"in the application’s API usage: %s. Review and update your application to ensure it is not using conflicting APIs " +
"from different levels of MicroProfile, Java EE, or Jakarta EE.";
public static final String BINARY_SCANNER_CONFLICT_MESSAGE4 = "[None available]"; // format should match JVM Set.toString()
public static final String BINARY_SCANNER_CONFLICT_MESSAGE5 = "A working set of features could not be generated due to conflicts " +
"in the required features: %s and required levels of MicroProfile: %s, Java EE or Jakarta EE: %s. Review and update your application to ensure it " +
public static final String BINARY_SCANNER_CONFLICT_MESSAGE5 = "A working set of features could not be generated due to conflicts " +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any actual changes to this file? or all formatting? If the latter, please revert the changes to this file.

"in the required features: %s and required levels of MicroProfile: %s, Java EE or Jakarta EE: %s. Review and update your application to ensure it " +
"is using the correct levels of MicroProfile, Java EE, or Jakarta EE, or consider removing the following set of features: %s.";
public static final String BINARY_SCANNER_INVALID_MP_MESSAGE = "The MicroProfile version number %s specified in the build file " +
"is not supported for feature generation.";
Expand Down Expand Up @@ -94,7 +94,7 @@ public BinaryScannerUtil(File binaryScanner) {
* optimize parameter. The currentFeatureSet parameter indicates the starting list of features and all the
* generated features will be compatible. The generated features will also be compatible with the indicated
* versions of Java EE or Jakarta EE and MicroProfile.
*
*
* @param currentFeatureSet - the features already specified in the server configuration
* @param classFiles - a set of class files for the scanner to handle. Should be a subset of allClassesDirectories
* @param allClassesDirectories - the directories containing all the class files of the application
Expand All @@ -109,7 +109,7 @@ public BinaryScannerUtil(File binaryScanner) {
* @throws NoRecommendationException - indicates a problem and there are no recommended features
* @throws RecommendationSetException - indicates a problem but the scanner was able to generate a set of
* features that should work to run the application
* @throws FeatureModifiedException - indicates a problem but the scanner was able to generate a set of features
* @throws FeatureModifiedException - indicates a problem but the scanner was able to generate a set of features
* that should work if certain features are modified
* @throws FeatureUnavailableException - indicates a problem between required features and required MP/EE levels but
* the scanner was able to generate a set of features that should be removed
Expand All @@ -118,7 +118,7 @@ public BinaryScannerUtil(File binaryScanner) {
* scanner when used in combination with each other. E.g. EE 7 and MP 2.1
*/
public Set<String> runBinaryScanner(Set<String> currentFeatureSet, List<String> classFiles, Set<String> allClassesDirectories,
String logLocation, String targetJavaEE, String targetMicroProfile, boolean optimize)
String logLocation, String targetJavaEE, String targetMicroProfile, boolean optimize)
throws PluginExecutionException, NoRecommendationException, RecommendationSetException, FeatureModifiedException,
FeatureUnavailableException, IllegalTargetException, IllegalTargetComboException {
Set<String> featureList = null;
Expand Down Expand Up @@ -189,7 +189,7 @@ public Set<String> runBinaryScanner(Set<String> currentFeatureSet, List<String>
Set<String> modifications = getFeatures(scannerException);
// rerun binary scanner with all class files and without the current feature set
Set<String> sampleFeatureList = reRunIfFailed ? reRunBinaryScanner(allClassesDirectories, logLocation, targetJavaEE, targetMicroProfile) : null;
throw new FeatureModifiedException(modifications,
throw new FeatureModifiedException(modifications,
(sampleFeatureList == null) ? getNoSampleFeatureList() : sampleFeatureList, scannerException.getLocalizedMessage());
} else if (scannerException.getClass().getName().equals(FEATURE_NOT_AVAILABLE_EXCEPTION)) {
// The list of features required by app or passed to binary scanner do not exist
Expand Down Expand Up @@ -241,10 +241,10 @@ public Set<String> runBinaryScanner(Set<String> currentFeatureSet, List<String>
/**
* The method is intended to call the binary scanner to generate a list of the optimal features for an
* application. This optimal list can be reported to the user as a suggested list of features.
*
*
* In order to generate the optimal list we must scan all classes in the application and we do not consider
* the features already specified in the server configuration (server.xml).
*
*
* @param allClassesDirectories - the scanner will find all the class files in this set of directories
* @param logLocation - directory name relative to project or absolute path passed to binary scanner
* @param targetJavaEE - generate features valid for the indicated version of EE
Expand All @@ -267,13 +267,13 @@ public Set<String> reRunBinaryScanner(Set<String> allClassesDirectories, String
logLocation = null;
}
debug("Recalling binary scanner with the following inputs...\n" +
" binaryInputs: " + binaryInputs + "\n" +
" targetJavaEE: " + targetJavaEE + "\n" +
" targetMicroP: " + targetMicroProfile + "\n" +
" currentFeatures: " + currentFeaturesSet + "\n" +
" logLocation: " + logLocation + "\n" +
" logLevel: " + logLevel + "\n" +
" locale: " + java.util.Locale.getDefault());
" binaryInputs: " + binaryInputs + "\n" +
" targetJavaEE: " + targetJavaEE + "\n" +
" targetMicroP: " + targetMicroProfile + "\n" +
" currentFeatures: " + currentFeaturesSet + "\n" +
" logLocation: " + logLocation + "\n" +
" logLevel: " + logLevel + "\n" +
" locale: " + java.util.Locale.getDefault());
featureList = (Set<String>) generateFeatureSetMethod.invoke(null, binaryInputs, targetJavaEE, targetMicroProfile,
currentFeaturesSet, logLocation, logLevel, java.util.Locale.getDefault());
for (String s : featureList) {debug(s);};
Expand Down Expand Up @@ -446,12 +446,12 @@ public static String composeMPVersion(String ver) {

/**
* Convenience method to build the string reported to the user when the exception is detected.
*
*
* This is used after the caller has analyzed the Java or Jakarta EE version number and the MicroProfile
* version number and generated argument values to pass to the binary scanner. If the binary scanner
* detects a problem and throws an exception it reports the invalid arguments. We must map the invalid
* arguments back to the user specified version number in order to fix the problem.
*
*
* @param invalidEEArg - the argument passed to the binary scanner which may be returned as invalid.
* @param invalidMPArg - the argument passed to the binary scanner which may be returned as invalid.
* @param eeVersion - the user specified version string from the build file used to generate the arg.
Expand Down Expand Up @@ -593,4 +593,4 @@ public class IllegalTargetComboException extends AbstractIllegalTargetException
super(eeLevel, mpLevel);
}
}
}
}
Loading
Loading