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

XML Security Issue #48

Closed
KevinHock opened this issue Nov 22, 2017 · 7 comments
Closed

XML Security Issue #48

KevinHock opened this issue Nov 22, 2017 · 7 comments

Comments

@KevinHock
Copy link

Please read salesforce-python-client/pyforce#35 for more information, the PR should be the same as that one, I can make it if you'd like but this repo seems to not get updated much.

@superfell
Copy link
Owner

Thanks for raising this, I looked at the pyforce PR, which seems straightforward enough but adds additional dependencies, something beatbox was trying to avoid. Typically the sax handler can disable this, i'll have a poke around.

@hynekcer
Copy link
Contributor

hynekcer commented Nov 23, 2017

It is interesting that the external package defusexml is one of two safe alternatives recommended in Python docs about XML. I think that Salesforce is more trustworthy for its customers than any trustworthy external package, e.g. by hijackinging that additional site.

Internal ElementTree package would be safe against data leak. The remaining "billion laughs" or "quadratic blowup" DDoS attacks are improbable by a response from Salesforce. Salesforce has so many easier legal ways to directly refuse a service that a DDoS attack by XML is a nonsense.

@KevinHock
Copy link
Author

There are a few different things here:

If you're concerned with the extra attack surface of an external package getting updated with malicious code in the future then you can just lock it to ==0.5.0 -- I think that's the latest and I did read it all. (Or just inline the relevant code, although I don't remember if it has xmlsec etc. as a dependency.)

If you're concerned with PyPi getting hacked you should have internal mirrors and audit everything when you update.

When I say Salesforce I'm not talking about the brand name or company ethos, I have nothing against them [1], it's more that if they have a disgruntled employee or are compromised. If I was on their security team I wouldn't want this library being vulnerable, as it potentially makes things worse in the event of an incident.

Is potential RCE (by using sax) an acceptable risk, given the pretty low likelihood? No.

Is DoS (not DDoS), by using ElementTree, an acceptable risk, given the pretty low likelihood? I might say Yes, you say Yes, but do we want to decide for everyone using the library? I don't know, I fixed my fork with defusedxml and don't really know what to say.

[1] one small thing

@KevinHock
Copy link
Author

KevinHock commented Nov 23, 2017

Sure thing, I'll have a look at whatever you come up with without defusedxml.

@KevinHock
Copy link
Author

KevinHock commented Nov 23, 2017

ElementTree doesn't export make_parser though, and I've never seen anyone else use sax, so there is probably a legitimate reason why you can't replace it with ElementTree easily.

@hynekcer
Copy link
Contributor

Sax module in current Python versions >= 3.6.7, >= 3.7.1, >= 3.8.0 is safe against RCE attacks by xml (external entity expansion attack and DTD retrieval). In other words: all Python versions >= 3.6 with a security fix from October 2018 or later are safe.

The official end of life of Python 2.7 was on 1th January 2020. The end of life of 3.5 is on 13th September 2020.

If Salesforce is used as a storage of important private economic data then providing no data by Salesforce would an effective form of Denial of Service that can't be prevented. Providing invalid economic data by Salesforce (or someone who can falsify a TLS certificate of SFDC) would be even worse than a DoS.

@KevinHock
Copy link
Author

That's a great point about Python 2, I'm okay with closing this 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants