Critical security vulnerability

[elwint]

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?

[adamdecaf]

@elwint did you hear back? I'm curious what you found.

[elwint]

@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.

[adamdecaf]

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.

[ma314smith]

@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.

[elwint]

@ma314smith Thanks for your reply, but note to other users, the vulnerability is critical if the signed payload returned by ValidateReferences is not used, or if the deprecated Validate function was used, and the document is parsed by Go’s XML unmarshaller.

[adamdecaf]

Thanks, yes this looks to be mostly resolved with ValidateReferences. Callers need to check/use the returned references though.

[elwint]

@adamdecaf I think the vulnerability is still present and should be properly mitigated, the deprecated Validate function still exists, which should be removed (also in the readme). The references returned by ValidateReferences are not always checked/used by callers, so perhaps something can be implemented to mitigate the attack. Can you also create a GitHub security advisory to inform users about the vulnerability?

[adamdecaf]

I've removed references to .Validate() in the readme and tests, but I am seeing one case where ValidateReferences() returns a ref I don't think it should. Do you have a better example for checking references?

We can create a security advisory on pre-1.0 releases. We're planning a 1.0 release soon.

[rowland66]

@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?

[elwint]

@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.

[elwint]

Because a signature validation bypass is possible when the deprecated Validate function is used, it should be removed entirely. This may be a breaking change, but at least it cannot be used anymore.

[adamdecaf]

Because a signature validation bypass is possible when the deprecated Validate function is used, it should be removed entirely. This may be a breaking change, but at least it cannot be used anymore.

Agreed.