Add fix for xml entities not working

[zimri-leisher]

Related Issue(s)	#3111
Has Unit Tests (y/n)	n
Documentation Included (y/n)	n

---

See #3111
Tested locally. Would be ideal if someone can replicate the issue briefly and test this single line fix.
Instructions:

1. Create a packets.xml as defined in the above issue
2. Make sure lxml version is 5.3.0
3. Try to build
4. See that it fails without this fix, and works with it

[LeStarch]

@zimri-leisher can you provide an example of the external file? I can see how to import entities, but not how to define them.

[LeStarch]

Looks like it is a copy-paste mechanism.

[LeStarch]

Confirmed that this functionally works. I have asked @bitWarrior to weigh-in on the security considerations.

[zimri-leisher]

@zimri-leisher can you provide an example of the external file? I can see how to import entities, but not how to define them.

I provided an example in the issue, I think. Let me know if that's not sufficient

[bitWarrior]

XML external entity processing is disabled by default to protect against XML External Entity (XXE) attacks which can allow an adversary to execute remote code and/or access internal memory by inserting malicious code in the entity field of an XML file. Although it could be argued the probability of an XXE attack against an F' deployment would require a sophisticated attacker (for obvious reasons I will not detail how it could be done), my option is the probability and negative impact of an XXE attack against an F' deployment is serious enough to keep F's default XML entity protection.

I need to know more about the implementation and the functionality that is required that this issue is requesting, but below are my recommendations that may, or may not be applicable:

1. Consider alternative data format - F' is moving from XML to JSON.
2. Whitelisting - If the entity format is specifically defined without modification then F' can enable "whitelisting" for this implementation.
3. Validation and sanitization of XML entity fields - To implement this would be a major change (functionality) to F' and is above the scope of this issue. I am including this option for completeness.
4. Replace lxml with a secure XML parser such as defusedxml - I am unsure if this can be done locally or if the F' Core requires modification.
5. Enable external entity resolution locally - If an F deployment's security posture accepts the risk of an XXE vulnerability for their specific F deployment, I do not have an issue as long as the F baseline configuration is preserved. However, open source fragmentation is a real concern, and I would defer to @LeStarch's thoughts on this option.

[zimri-leisher]

A few comments:

1. this feature was enabled by default (that is, F` did not have default XXE protection) as of v3.4.3, and correct me if wrong, but it was not disabled on purpose. Rather, it just was a consequence of updating the version of lxml.
2. Note my fix disables network access via a setting in the XML parser. This severely limits the exploitability of this feature to loading files already on the computer.
3. A theoretical malicious packets.xml would only run its exploit at build time (while calling fprime-util build), as that is the only time the XML DTD data (the exploitable XML feature) is parsed. You can already compile and run arbitrary code at build time, including network access, with simple CMake commands or shell scripts. I would argue this introduces no new risk--if you're already compiling untrusted code, you should accept that you are giving someone code execution privileges on your computer.

In response to your recommendations:

1. Moving to JSON would indeed mitigate this risk. However, at the moment, there is no option to switch. Maybe @LeStarch can comment on whether moving the TlmPacketizer packets.xml file to be packets.json is on the roadmap.
2. I believe this is what my PR here is doing--I'm blacklisting all networked access.
3. Yes, this sounds like a large change and there are better options.
4. Would have to modify several tools and autocoders, but could be done
5. Sure, perhaps an environment variable or likewise could be checked at runtime to enable or disable this feature.

I believe the best option is:
Keep the previous behavior (allowing entity references) with network access disabled (preventing malicious packet.xml files from accessing the web). In the future, move to JSON, and I guess we'll just have to figure out some other way of modularizing packet definition files.

[zimri-leisher]

To follow up my comment, I want to be clear that I share your security concerns and agree that having a common and easily preventable vulnerability in F Prime is awful! However, I don't think that allowing the feature of XML entities means that we introduce the vulnerability. As far as I can tell, XXE is a web-focused vuln in servers that expose XML parsers to users. The very nature of the F Prime project makes it unlikely to ever be considered as an attack vector.

[LeStarch]

We are working on adding packet specification to the FPP model. Thus long term this will not be an issue. It is not JSON, but it is something.

My favorite is option 5 above. To that end, I see several possible paths forward:

1. load_dtd=os.environ.get("FPRIME_ENABLE_PPACKET_DTD", False). In this way, projects that want the feature could set it in the environment. It is clumsy, but meets the goals of item 5.
2. Add an argument to specify load_dtd. It is more work, as this would need to map to a CMake hook of some kind, but it is cleaner.
3. Recommend projects who care downgrade lxml if that can work.

With network access disabled and a project being responsible for authoring the Packets.xml that uses external entities, it seems to me that the attack vector is fairly mitigated (and would require compromise of a build machine). However, I look to @bitWarrior for clear direction in security.

[LeStarch]

We discussed this issue today. The expected deliver for the FPP format Telemetry Packet Specifications is in the next F´  release (when this would also be delivered).

Thus the best solution using FPP and include files will be available. I will close this PR, but keep the issue (with a link to this PR) in case the schedule changes.