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 content is not escaped. #7

Open
fisx opened this issue Feb 20, 2019 · 2 comments
Open

XML content is not escaped. #7

fisx opened this issue Feb 20, 2019 · 2 comments

Comments

@fisx
Copy link
Contributor

fisx commented Feb 20, 2019

I just got the following test case to pass over in saml2-web-sso:

      HS.samlToXML (HS.simpleNameID HS.NameIDFormatUnspecified "<something>")
        `shouldBe` "<NameID xmlns=\"urn:oasis:names:tc:SAML:2.0:assertion\"><something></NameID>"
        -- it really shouldn't, though!

Not sure how big of a security issue that is, but it doesn't seem right. I'm wondering if there is some escaping functionality in HXT that is just hard to call in all the right places?

I don't think this'll be trivial to fix. I have worked around this in saml2-web-sso in hopefully all the places for now.

@dylex
Copy link
Owner

dylex commented Feb 21, 2019

Urm, yeah that doesn't seem right. It should at least be easy to fix, I just am not sure how, because I've never been clear about how hxt intends to deal with this. I believe the actual problem is in the implementation of docToXML (for samlToXML). This currently uses Text.XML.HXT.Arrow.XmlArrow.xshowBlob, which uses Text.XML.HXT.DOM.ShowXml.xshowBlob, which, unlike other functions like xshow', doesn't do any escaping.

I've now changed this to using xshow' and escapeXmlRefs directly, though it doesn't feel quite right to me either. I do think this is at least the right place for a fix, rather than xpString or higher places for example. Totally open if you have a better hxt solution, though.

@fisx
Copy link
Contributor Author

fisx commented Feb 22, 2019

I'm still not the right person to ask for opinions on HXT. :-) I think it's just not ready, and it's so old and vast it will never be.

Thanks for looking into this! Your patch idea reasonable to me. I'll let you know if I can find the time to test it and/or merge our fork with it.

tathougies pushed a commit to tunnelhound/hsaml2 that referenced this issue Oct 5, 2020
Not having it here is problematic, as packages depending on it can't pin
the repository as the hash of the repository depends on the version of
hpack used which can vary from system to system.

Stack is actually changing this behaviour soon:
https://tech.fpcomplete.com/blog/storing-generated-cabal-files

We hit this bug here:
wireapp/wire-server#1027 (comment)
where now CI isn't reproducible because the cabal file generated on my
machine doesn't match the cabal file generated on CI
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

2 participants