Skip to content

Commit

Permalink
Merge pull request from GHSA-r2hc-pmr7-4c9r
Browse files Browse the repository at this point in the history
* Configured XML parsers to resist XXE attacks

Signed-off-by: Kai Kreuzer <kai@openhab.org>

* added fix for avmfritz

Signed-off-by: Kai Kreuzer <kai@openhab.org>

* added fix for sonos

Signed-off-by: Kai Kreuzer <kai@openhab.org>

* added fix for vitotronic and bosesoundtouch

Signed-off-by: Kai Kreuzer <kai@openhab.org>

* changed avmfritz to singleton pattern

Signed-off-by: Kai Kreuzer <kai@openhab.org>

* addressed roku binding

Signed-off-by: Kai Kreuzer <kai@openhab.org>

* address all uses of DocumentBuilderFactory

Signed-off-by: Kai Kreuzer <kai@openhab.org>

* fixed other occurrences in roku binding

Signed-off-by: Kai Kreuzer <kai@openhab.org>
  • Loading branch information
kaikreuzer authored and thinkingstone committed Nov 7, 2021
1 parent b1e1206 commit 4b2620d
Show file tree
Hide file tree
Showing 33 changed files with 235 additions and 34 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@

import javax.xml.bind.JAXBException;
import javax.xml.bind.Unmarshaller;
import javax.xml.stream.XMLStreamException;
import javax.xml.stream.XMLStreamReader;

import org.eclipse.jdt.annotation.NonNullByDefault;
import org.openhab.binding.avmfritz.internal.dto.DeviceListModel;
Expand Down Expand Up @@ -62,15 +64,16 @@ public void execute(int status, String response) {
logger.trace("Received State response {}", response);
if (isValidRequest()) {
try {
XMLStreamReader xsr = JAXBUtils.XMLINPUTFACTORY.createXMLStreamReader(new StringReader(response));
Unmarshaller unmarshaller = JAXBUtils.JAXBCONTEXT_DEVICES.createUnmarshaller();
DeviceListModel model = (DeviceListModel) unmarshaller.unmarshal(new StringReader(response));
DeviceListModel model = (DeviceListModel) unmarshaller.unmarshal(xsr);
if (model != null) {
handler.onDeviceListAdded(model.getDevicelist());
} else {
logger.debug("no model in response");
}
handler.setStatusInfo(ThingStatus.ONLINE, ThingStatusDetail.NONE, null);
} catch (JAXBException e) {
} catch (JAXBException | XMLStreamException e) {
logger.error("Exception creating Unmarshaller: {}", e.getLocalizedMessage(), e);
handler.setStatusInfo(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR,
e.getLocalizedMessage());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@

import javax.xml.bind.JAXBException;
import javax.xml.bind.Unmarshaller;
import javax.xml.stream.XMLStreamException;
import javax.xml.stream.XMLStreamReader;

import org.eclipse.jdt.annotation.NonNullByDefault;
import org.openhab.binding.avmfritz.internal.dto.templates.TemplateListModel;
Expand Down Expand Up @@ -58,14 +60,15 @@ public void execute(int status, String response) {
logger.trace("Received response '{}'", response);
if (isValidRequest()) {
try {
XMLStreamReader xsr = JAXBUtils.XMLINPUTFACTORY.createXMLStreamReader(new StringReader(response));
Unmarshaller unmarshaller = JAXBUtils.JAXBCONTEXT_TEMPLATES.createUnmarshaller();
TemplateListModel model = (TemplateListModel) unmarshaller.unmarshal(new StringReader(response));
TemplateListModel model = (TemplateListModel) unmarshaller.unmarshal(xsr);
if (model != null) {
handler.addTemplateList(model.getTemplates());
} else {
logger.debug("no template in response");
}
} catch (JAXBException e) {
} catch (JAXBException | XMLStreamException e) {
logger.error("Exception creating Unmarshaller: {}", e.getLocalizedMessage(), e);
}
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

import javax.xml.bind.JAXBContext;
import javax.xml.bind.JAXBException;
import javax.xml.stream.XMLInputFactory;

import org.eclipse.jdt.annotation.NonNullByDefault;
import org.eclipse.jdt.annotation.Nullable;
Expand All @@ -34,6 +35,7 @@ public class JAXBUtils {

public static final @Nullable JAXBContext JAXBCONTEXT_DEVICES = initJAXBContextDevices();
public static final @Nullable JAXBContext JAXBCONTEXT_TEMPLATES = initJAXBContextTemplates();
public static final XMLInputFactory XMLINPUTFACTORY = initXMLInputFactory();

private static @Nullable JAXBContext initJAXBContextDevices() {
try {
Expand All @@ -52,4 +54,11 @@ public class JAXBUtils {
return null;
}
}

private static XMLInputFactory initXMLInputFactory() {
XMLInputFactory xif = XMLInputFactory.newInstance();
xif.setProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, false);
xif.setProperty(XMLInputFactory.SUPPORT_DTD, false);
return xif;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ public XMLResponseProcessor(BoseSoundTouchHandler handler) {

public void handleMessage(String msg) throws SAXException, IOException {
XMLReader reader = XMLReaderFactory.createXMLReader();
reader.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
reader.setContentHandler(new XMLResponseHandler(handler, stateSwitchingMap));
reader.parse(new InputSource(new StringReader(msg)));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,8 @@ private <T> T getDocument(String uri, Class<T> response) throws IOException {
if (StringUtils.isNotBlank(result)) {
JAXBContext jc = JAXBContext.newInstance(response);
XMLInputFactory xif = XMLInputFactory.newInstance();
xif.setProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, false);
xif.setProperty(XMLInputFactory.SUPPORT_DTD, false);
XMLStreamReader xsr = xif.createXMLStreamReader(IOUtils.toInputStream(result));
xsr = new PropertyRenamerDelegate(xsr);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -261,8 +261,15 @@ private void autoConfigure() {

if (status == HttpURLConnection.HTTP_OK && response != null) {
DocumentBuilderFactory domFactory = DocumentBuilderFactory.newInstance();
DocumentBuilder builder;
try {
// see
// https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html
domFactory.setFeature("http://xml.org/sax/features/external-general-entities", false);
domFactory.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
domFactory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
domFactory.setXIncludeAware(false);
domFactory.setExpandEntityReferences(false);
DocumentBuilder builder;
builder = domFactory.newDocumentBuilder();
Document dDoc = builder.parse(new InputSource(new StringReader(response.getContentAsString())));
XPath xPath = XPathFactory.newInstance().newXPath();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,14 @@ public DLinkHNAPCommunication(final String ipAddress, final String pin) {
uri = new URI("http://" + ipAddress + "/HNAP1");
httpClient.start();

parser = DocumentBuilderFactory.newInstance().newDocumentBuilder();
DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance();
// see https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html
dbf.setFeature("http://xml.org/sax/features/external-general-entities", false);
dbf.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
dbf.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
dbf.setXIncludeAware(false);
dbf.setExpandEntityReferences(false);
parser = dbf.newDocumentBuilder();

final MessageFactory messageFactory = MessageFactory.newInstance();
requestAction = messageFactory.createMessage();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,9 +81,16 @@ public class Enigma2Client {
private final Enigma2HttpClient enigma2HttpClient;
private final DocumentBuilderFactory factory;

public Enigma2Client(String host, @Nullable String user, @Nullable String password, int requestTimeout) {
this.enigma2HttpClient = new Enigma2HttpClient(requestTimeout);
this.factory = DocumentBuilderFactory.newInstance();
public Enigma2Client(String host, @Nullable String user, @Nullable String password, int requestTimeout)
throws ParserConfigurationException {
enigma2HttpClient = new Enigma2HttpClient(requestTimeout);
factory = DocumentBuilderFactory.newInstance();
// see https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html
factory.setFeature("http://xml.org/sax/features/external-general-entities", false);
factory.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
factory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
factory.setXIncludeAware(false);
factory.setExpandEntityReferences(false);
if (StringUtils.isNotEmpty(user) && StringUtils.isNotEmpty(password)) {
this.host = "http://" + user + ":" + password + "@" + host;
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,12 @@ public class Client {
public Client() {
documentBuilderFactory.setNamespaceAware(true);
try {
// see https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html
documentBuilderFactory.setFeature("http://xml.org/sax/features/external-general-entities", false);
documentBuilderFactory.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
documentBuilderFactory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
documentBuilderFactory.setXIncludeAware(false);
documentBuilderFactory.setExpandEntityReferences(false);
documentBuilder = documentBuilderFactory.newDocumentBuilder();
} catch (ParserConfigurationException e) {
throw new IllegalStateException(e);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,12 @@ public String getSessionId() {
private Document getXmlDocFromString(String xmlString)
throws ParserConfigurationException, SAXException, IOException {
final DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();
// see https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html
factory.setFeature("http://xml.org/sax/features/external-general-entities", false);
factory.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
factory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
factory.setXIncludeAware(false);
factory.setExpandEntityReferences(false);
final DocumentBuilder builder = factory.newDocumentBuilder();
final Document xmlDocument = builder.parse(new InputSource(new StringReader(xmlString)));
return xmlDocument;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,14 @@ public StatusFileInterpreter(String hostname, Ipx800EventListener listener) {

public void read() {
try {
DocumentBuilder builder = DocumentBuilderFactory.newInstance().newDocumentBuilder();
DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();
// see https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html
factory.setFeature("http://xml.org/sax/features/external-general-entities", false);
factory.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
factory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
factory.setXIncludeAware(false);
factory.setExpandEntityReferences(false);
DocumentBuilder builder = factory.newDocumentBuilder();
String statusPage = HttpUtil.executeUrl("GET", String.format(URL_TEMPLATE, hostname), 5000);
InputStream inputStream = new ByteArrayInputStream(statusPage.getBytes());
Document document = builder.parse(inputStream);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,9 @@ public XmlRpcResponse(InputStream is, String encoding)
throws SAXException, ParserConfigurationException, IOException {
SAXParserFactory factory = SAXParserFactory.newInstance();
SAXParser saxParser = factory.newSAXParser();
factory.setFeature("https://xml.org/sax/features/external-general-entities", false);
saxParser.getXMLReader().setFeature("https://xml.org/sax/features/external-general-entities", false);
factory.setFeature("https://apache.org/xml/features/disallow-doctype-decl", true);
InputSource inputSource = new InputSource(is);
inputSource.setEncoding(encoding);
saxParser.parse(inputSource, new XmlRpcHandler());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ public class HPWebServerClient {

/**
* Creates a new HP Web Server Client object.
*
*
* @param httpClient {HttpClient} The HttpClient to use for HTTP requests.
* @param address The address for the Embedded Web Server.
*/
Expand All @@ -63,7 +63,7 @@ public HPWebServerClient(HttpClient httpClient, String address) {

/**
* Gets the Status information from the Embedded Web Server.
*
*
* @return The status information.
*/
public HPServerResult<HPStatus> getStatus() {
Expand All @@ -84,7 +84,7 @@ public HPServerResult<HPScannerStatusFeatures> getScannerFeatures() {

/**
* Gets the Usage information from the Embedded Web Server.
*
*
* @return The usage information.
*/
public HPServerResult<HPUsage> getUsage() {
Expand Down Expand Up @@ -120,6 +120,12 @@ private <T> HPServerResult<T> fetchData(String endpoint, Function<Document, T> f

private synchronized Document getDocument(String contentAsString)
throws ParserConfigurationException, SAXException, IOException {
// see https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html
factory.setFeature("http://xml.org/sax/features/external-general-entities", false);
factory.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
factory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
factory.setXIncludeAware(false);
factory.setExpandEntityReferences(false);
DocumentBuilder builder = factory.newDocumentBuilder();
InputSource source = new InputSource(new StringReader(contentAsString));
return builder.parse(source);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,12 @@ public static Document readFromFile(String filePath) throws IhcExecption {
File fXmlFile = new File(filePath);
DocumentBuilderFactory dbFactory = DocumentBuilderFactory.newInstance();
try {
// see https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html
dbFactory.setFeature("http://xml.org/sax/features/external-general-entities", false);
dbFactory.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
dbFactory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
dbFactory.setXIncludeAware(false);
dbFactory.setExpandEntityReferences(false);
DocumentBuilder dBuilder = dbFactory.newDocumentBuilder();
Document doc = dBuilder.parse(fXmlFile);
return doc;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,12 @@ public Map<String, DeviceType> getDeviceTypes() {
*/
public void loadDeviceTypesXML(InputStream in) throws ParserConfigurationException, SAXException, IOException {
DocumentBuilderFactory dbFactory = DocumentBuilderFactory.newInstance();
// see https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html
dbFactory.setFeature("http://xml.org/sax/features/external-general-entities", false);
dbFactory.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
dbFactory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
dbFactory.setXIncludeAware(false);
dbFactory.setExpandEntityReferences(false);
DocumentBuilder dBuilder = dbFactory.newDocumentBuilder();
Document doc = dBuilder.parse(in);
doc.getDocumentElement().normalize();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,12 @@ public static List<FeatureTemplate> readTemplates(InputStream input) throws IOEx
List<FeatureTemplate> features = new ArrayList<>();
try {
DocumentBuilderFactory dbFactory = DocumentBuilderFactory.newInstance();
// see https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html
dbFactory.setFeature("http://xml.org/sax/features/external-general-entities", false);
dbFactory.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
dbFactory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
dbFactory.setXIncludeAware(false);
dbFactory.setExpandEntityReferences(false);
DocumentBuilder dBuilder = dbFactory.newDocumentBuilder();
// Parse it!
Document doc = dBuilder.parse(input);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,12 @@ public static Map<String, Msg> readMessageDefinitions(InputStream input)
Map<String, Msg> messageMap = new HashMap<>();
try {
DocumentBuilderFactory dbFactory = DocumentBuilderFactory.newInstance();
// see https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html
dbFactory.setFeature("http://xml.org/sax/features/external-general-entities", false);
dbFactory.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
dbFactory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
dbFactory.setXIncludeAware(false);
dbFactory.setExpandEntityReferences(false);
DocumentBuilder dBuilder = dbFactory.newDocumentBuilder();
// Parse it!
Document doc = dBuilder.parse(input);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -498,6 +498,12 @@ public void statusUpdateReceived(String ip, EiscpMessage data) {
private void processInfo(String infoXML) {
try {
DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();
// see https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html
factory.setFeature("http://xml.org/sax/features/external-general-entities", false);
factory.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
factory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
factory.setXIncludeAware(false);
factory.setExpandEntityReferences(false);
DocumentBuilder builder = factory.newDocumentBuilder();
try (StringReader sr = new StringReader(infoXML)) {
InputSource is = new InputSource(sr);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

import javax.xml.bind.JAXBContext;
import javax.xml.bind.JAXBException;
import javax.xml.stream.XMLInputFactory;

import org.eclipse.jdt.annotation.NonNullByDefault;
import org.eclipse.jdt.annotation.Nullable;
Expand All @@ -38,6 +39,7 @@ public class JAXBUtils {
public static final @Nullable JAXBContext JAXBCONTEXT_APPS = initJAXBContextApps();
public static final @Nullable JAXBContext JAXBCONTEXT_DEVICE_INFO = initJAXBContextDeviceInfo();
public static final @Nullable JAXBContext JAXBCONTEXT_PLAYER = initJAXBContextPlayer();
public static final XMLInputFactory XMLINPUTFACTORY = initXMLInputFactory();

private static @Nullable JAXBContext initJAXBContextActiveApp() {
try {
Expand Down Expand Up @@ -74,4 +76,11 @@ public class JAXBUtils {
return null;
}
}

private static XMLInputFactory initXMLInputFactory() {
XMLInputFactory xif = XMLInputFactory.newInstance();
xif.setProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, false);
xif.setProperty(XMLInputFactory.SUPPORT_DTD, false);
return xif;
}
}
Loading

0 comments on commit 4b2620d

Please sign in to comment.