Feature/api secure endpoints#466
Conversation
- Introduced /getPropertyList endpoint returning application properties as JSON. - Introduced /getXml endpoint returning XML content filtered via XPath. - Added HTTPS enforcement for non-localhost requests to prevent exposure of sensitive information. - Implemented isLocalhost(Request) helper for local testing exceptions. - Added XML parsing with XXE protection. - Updated imports and cleaned up unused imports.
- Ignore log files under Server/OPENAS2_LOG_DIR_IS_UNDEFINED - Remove redundant comment for XPath validation in ApiResource
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
- Restrict allowed XPath expressions to a predefined set, including "*". - Prevent XPath injection and XXE attacks by validating input and disabling DOCTYPE. - Wrap returned XML nodes in a <results> root element to avoid DOM hierarchy errors. - Simplify XML transformation and response building.
|
Security & CodeQL fixes applied: Whitelisted XPath expressions to prevent injection. Wrapped XML results in a safe root element. Disabled DOCTYPE parsing to prevent XXE. Enforced SSL/TLS for non-local requests. All CodeQL/security issues resolved. |
uhurusurfa
left a comment
uhurusurfa
left a comment
There was a problem hiding this comment.
Please can you provide more insight into why you are retirieving XML from disk using OpenAS2 to return to a service that is running on the same server?
Would it not be better to just read the file directly in the other service?
| private NodeList getNodes(String xmlFileName, String xpathExpression) { | ||
| NodeList nodeList = null; | ||
| try { | ||
| DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance(); | ||
|
|
||
| // === XXE Protection === | ||
| dbf.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); | ||
| 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); | ||
|
|
||
| DocumentBuilder db = dbf.newDocumentBuilder(); | ||
| File file = new File(xmlFileName); | ||
| Document document = db.parse(file); | ||
|
|
||
| XPathExpression xPathExpr = XPathFactory.newInstance().newXPath().compile(xpathExpression); | ||
| nodeList = (NodeList) xPathExpr.evaluate(document, XPathConstants.NODESET); | ||
|
|
||
| } catch (Exception ex) { | ||
| LoggerFactory.getLogger(ApiResource.class.getName()).error("Error parsing XML file: " + xmlFileName, ex); | ||
| // return null on error | ||
| } | ||
| return nodeList; | ||
| } |
There was a problem hiding this comment.
This appears to be a redundant method.
What is the reason this is here?
There was a problem hiding this comment.
The XML read here is used specifically to retrieve partnership endpoint information (polling location).
This allows the end-user UI to present available partnerships so that AS2 payloads can be placed in the appropriate directories.
Additionally, I intend to provide an API endpoint that can receive a partnership and an AS2 payload, and route the payload according to the partnership definitions.
There was a problem hiding this comment.
This method is not called from anywhere so I am unclear why it is here unless you have other code that is not included in this PR.
| } | ||
|
|
||
| Session session = getProcessor().getSession(); | ||
| String filePath = session.getBaseDirectory() + "/" + filename; |
There was a problem hiding this comment.
Since there is no validation of the file path this could retrieve any XML file on the server and is a security risk.
Is there a reason why you need to pass a filename in at all since you are only interested in the partnerships file?
EDIT: I was too quick off the mark on this comment - I see it is "protected" by enforcing HTTPS and can therefore be retrieved from anywhere in the world but it will still return sensitive information such as passwords if they are hard coded into the config.xml file and since you do not restrict which files this API endpoint can acces it can return the config.xml file.
| } | ||
| Map<String, String> result = new HashMap<>(); | ||
| try { | ||
| result = (Map<String, String>) Properties.getProperties(); |
There was a problem hiding this comment.
Note that this returns the parsed properties not the properties as set in the config.xml proerties section or the properties file.
If any properties use property references or envoronment variable references within their value then those references will have been replaced with actual values.
Based off the comment in your previous PR, you are using this for administration and debugging.
Are you intending to use this in a UI that allows you to change properties?
There are extensive debug logging statements that support figuring out issuses with properties if you set TRACE level logging.
Note also that there are a number of passwords in the properties list and passwords should not be returned in the list as it is a security risk. You will have to filter out password properties.
There was a problem hiding this comment.
Yes, intended usage is primarily for administration and debugging. Regarding the passwords, would redacting properties before returning the properties address the concern? I can implement that to ensure sensitive information is not exposed
There was a problem hiding this comment.
Yes - if you remove any property that has the word "password" in it's property name or property value that should work.
These are the currently known properties that have password information:
as2_keystore_password="testas2"
ssl_trust_keystore_password="testas2"
ssl_keystore_password="testas2"
restapi.command.processor.password="pWd"
module.HealthCheckModule.keystore_password="$properties.ssl_keystore_password$"
msg_tracking.tcp_server_password="openas2"
- Restrict allowed XPath expressions to a predefined set, including "*". - Prevent XPath injection and XXE attacks by validating input and disabling DOCTYPE. - Wrap returned XML nodes in a <results> root element to avoid DOM hierarchy errors. - Simplify XML transformation and response building.
…to feature/api-secure-endpoints
3 participants
Remove redundant comment regarding XPath validation