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.

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

xmerl allows XXE by default #7539

Closed
feld opened this issue Aug 5, 2023 · 5 comments
Closed

xmerl allows XXE by default #7539

feld opened this issue Aug 5, 2023 · 5 comments
Assignees
Labels
bug Issue is reported as a bug team:PO Assigned to OTP team PO team:PS Assigned to OTP team PS

Comments

@feld
Copy link

feld commented Aug 5, 2023

Hello,

xmerl is possibly the most mature XML parsing library in the Erlang ecosystem, but unfortunately it permits XXE vulnerabilities by default. Can this be disabled so everyone writing XML parsing code doesn't have to provide a custom fetch_fun for xmerl_scan or setting {allow_entities, False} to close the security hole?

This also affects some downstream consumers, including some wrappers for Elixir

https://vuln.be/post/xxe-in-erlang-and-elixir/

@feld feld added the bug Issue is reported as a bug label Aug 5, 2023
@lanodan
Copy link

lanodan commented Aug 5, 2023

Alternatively it should at least be documented, for example:

diff --git a/lib/xmerl/src/xmerl_scan.erl b/lib/xmerl/src/xmerl_scan.erl
index 62b5dd8..77c7b9a 100644
--- a/lib/xmerl/src/xmerl_scan.erl
+++ b/lib/xmerl/src/xmerl_scan.erl
@@ -36,6 +36,12 @@
 %% functions.
 %% </p>
 %% <p>
+%% <strong>Warning:</strong> By default XML entities, including external ones
+%% are loaded, this can lead to XML External Entity (XXE) vulnerabilities if
+%% you're parsing untrusted XML.<br />
+%% To avoid this you need to pass <code>{allow_entities, False}</code>.
+%% </p>
+%% <p>
 %% Possible options are:
 %% </p>
 %% <dl>

(My example might not be right, I write Elixir code rather than Erlang)

@IngelaAndin IngelaAndin added the team:PS Assigned to OTP team PS label Aug 7, 2023
@kikofernandez kikofernandez self-assigned this Aug 17, 2023
@kikofernandez
Copy link
Contributor

We are discussing internally if we could change the defaults. The main issue is (as always) breaking backwards compatibility.
I will post any decision here. Thanks for opening the ticket

@IngelaAndin IngelaAndin added the team:PO Assigned to OTP team PO label Sep 13, 2023
@kikofernandez
Copy link
Contributor

@feld we are looking into solving this, possibly by changing the default as {allow_entities, false}
The delay is due to breaking backwards compatibility and deciding on whether we add this to OTP-27, but we are working on it and will soon (a week or two) come to a conclusion

@kikofernandez
Copy link
Contributor

Same as before. We are working on it, slowly but steady considering options and fixes, to break backwards compatibility as little as possible. If you have any suggestions, please bring them on :)

@lthor lthor self-assigned this Nov 21, 2023
@IngelaAndin IngelaAndin added the stalled waiting for input by the Erlang/OTP team label Dec 12, 2023
lthor added a commit that referenced this issue May 3, 2024
…OTP-19079

Xmerl: Changed default values to disable XXE vulnerabilities

    xmerl_scan: default value for allow_entities is now 'false'
    xmerl_sax_parser: default value for external_entities is now 'none'
@lthor
Copy link
Contributor

lthor commented May 3, 2024

The default values are changed to avoid XML External Entity (XXE) vulnerabilities if one are parsing untrusted XML.
xmerl_scan: the default value for allow_entities has changed to false.
xmerl_sax_parser: the default value for external_entities has changed to none.

@lthor lthor closed this as completed May 3, 2024
@IngelaAndin IngelaAndin removed the stalled waiting for input by the Erlang/OTP team label May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue is reported as a bug team:PO Assigned to OTP team PO team:PS Assigned to OTP team PS
Projects
None yet
Development

No branches or pull requests

5 participants