-
Notifications
You must be signed in to change notification settings - Fork 44
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
Critical security vulnerability #23
Comments
@elwint did you hear back? I'm curious what you found. |
@adamdecaf I haven't heard back from him regarding the vulnerability. He only told me he hasn't been actively maintaining/using this library for a while. My recommendation would be to use a more established library for signature validation, e.g. https://github.com/russellhaering/goxmldsig @ma314smith What are your plans for this library? I will disclose more information about the vulnerability next week. |
Thanks @elwint, I've tried using goxmldsig and haven't had luck. It's a bit too specific for SAML and my use-case fails validation. We're working on a fork/update to goxmldsig, but that isn't very active either. |
@adamdecaf as you suggested in your PR, I'm open to handing this off if you're interested. I can forward you the info from @elwint and you can decide. Sounds like maybe it wasn't as critical as initially thought. |
@ma314smith Thanks for your reply, but note to other users, the vulnerability is critical if the signed payload returned by |
Thanks, yes this looks to be mostly resolved with ValidateReferences. Callers need to check/use the returned references though. |
@adamdecaf I think the vulnerability is still present and should be properly mitigated, the deprecated |
I've removed references to We can create a security advisory on pre-1.0 releases. We're planning a 1.0 release soon. |
@elwint could you provide more background and details on what security vulnerability exists. @adamdecaf and I are very interesting in fixing any vulnerabilities. With regards the Validate() and ValidateReferences() methods, the validate() method validates signed XML. It does this by applying some transforms to the xml that canonicalize it before checking the signature. If the validation succeeds, then XML received is guaranteed to semantically match what was signed. The strings returned from the ValidateReferences() method are just the canonicalized XML. Nothing in the canonicalization process changes the semantics of the XML, so once a signature is validated there is no reason to use the canonicalized XML as opposed to the received XML. They both represent the same information. @elwint is this at all related to your concerns? |
@adamdecaf I'm not sure what would be the best way to mitigate the attack without affecting legitimate use. However, the same attack doesn't work in goxmldsig because it always checks if the signature is referencing the top-element of the XML. Perhaps something similar can be implemented? @rowland66 The vulnerability exists when the received XML is parsed by Go's XML unmarshaller, instead of the signed payload returned by ValidateReferences. Parsing the received raw XML can result in a different output than parsing the signed payload/canonicalized XML. This makes it possible to bypass the signature validation using a Signature Wrapping attack (XSW). Adam should've received an email earlier containing more information about the vulnerability and a PoC that exploits the vulnerability. |
Because a signature validation bypass is possible when the deprecated |
Agreed. |
I would like to report a potentially critical security vulnerability, however I couldn't find a way to contact you. Could you please provide contact details?
The text was updated successfully, but these errors were encountered: