Skip to content

[VAULT-43467] saml: resolve CVE-2026-33487, GHSA-hwqm-qvj9-4jr2, and GHSA-pcgw-qcv5-h8ch#180

Merged
ryancragun merged 5 commits into
mainfrom
ryan-VAULT-43467
Mar 27, 2026
Merged

[VAULT-43467] saml: resolve CVE-2026-33487, GHSA-hwqm-qvj9-4jr2, and GHSA-pcgw-qcv5-h8ch#180
ryancragun merged 5 commits into
mainfrom
ryan-VAULT-43467

Conversation

@ryancragun

@ryancragun ryancragun commented Mar 24, 2026

Copy link
Copy Markdown
Collaborator

Resolve GHSA-hwqm-qvj9-4jr20, GHSA-pcgw-qcv5-h8ch1, and CVE-2026-334872 by upgrading github.com/russellhaering/gosaml2 to v0.11.0 and github.com/russellhaering/goxmldsig to v1.6.0.

Due to behavior changes in the upstream libraries we slightly modify the meaning of the saml.ValidateAssertionSignature() option to saml.ParseResponse(). In prior versions of gosaml2, when decoding and validating the response assertions, it would mark the SignatureVerified field as true after validating assertion signatures. That is no longer the case when the response itself is signed so we cannot rely on that field to determine both the presence of and a valid assertion signature for each assertion. Therefore, our validation behavior is slightly different depending on the overall response. When the response envelope is signed we can no longer enforce that each assertion is signed, only that the payload is signed and that any assertions that are also signed are valid. When the response envelope is not signed it falls back to similar path as before where it marks the SignatureVerified field when validating the assertion signatures. In this path our prior behavior is preserved.

Ultimately, these changes ought not yield less secure validation as either the response envelope and all assertions are correctly signed, or in cases where the envelope is not signed all assertions are. For more details please refer to the decoder and validator implementations in 3, 4, and 5.

Along with these updates I've also added a test to verify wrap attacked assertions are caught as invalid as one would expect.

PCI review checklist

  • I have documented a clear reason for, and description of, the change I am making.

  • If applicable, I've documented a plan to revert these changes if they require more than reverting the pull request.

  • If applicable, I've documented the impact of any changes to security controls.

    Examples of changes to security controls include using new access control methods, adding or removing logging pipelines, etc.

…HSA-pcgw-qcv5-h8ch

Resolve GHSA-hwqm-qvj9-4jr2[0], GHSA-pcgw-qcv5-h8ch[1], and
CVE-2026-33487[2] by upgrading github.com/russellhaering/gosaml2 to
v0.11.0 and github.com/russellhaering/goxmldsig to v1.6.0.

Due to behavior changes in the upstream libraries we slightly modify
the meaning of the saml.ValidateAssertionSignature() option to
saml.ParseResponse(). In prior versions of gosaml2, when decoding and
validating the responsse assertions, it would mark the SignatureVerified
field as true after validating assertion signatures. That is no longer
the case when the response itself is signed so we cannot rely on that
field to determine both the presensence of and a valid assertion
signature. Therefore, our validation behavior is slightly different
depending on the overall response. When the response envelope is signed
we can no longer enforce that each assertion is signed, only that the
payload is signed and that any assertions that are also signed are valid.
When the response envelope is not signed it falls back to similar path
as before where it marks the SignatureVerified field when validating the
assertion signatures. In this path our prior behavior is preserved.

Ultimately, these changes ought not yield less secure validation as
either the response envelope and all assertions are correctly signed, or
in cases where the envelope is not signed all assertions are. For more
details please refer to the decoder and validator implementations in [3],
[4], and [5].

Along with these updates I've also added a test to verify wrap attacked
assertions are caught as invalid as one would expect.

[0]: GHSA-hwqm-qvj9-4jr2
[1]: GHSA-pcgw-qcv5-h8ch
[2]: GHSA-479m-364c-43vc
[3]: https://github.com/russellhaering/gosaml2/blob/636e7dda202a4d669644e72404a82616ffcbe004/decode_response.go#L255-L289
[4]: https://github.com/russellhaering/gosaml2/blob/636e7dda202a4d669644e72404a82616ffcbe004/decode_response.go#L422
[5]: https://github.com/russellhaering/goxmldsig/blob/878c8c615feb628064040115d00e105a137fcfa7/validate.go#L568-L584

Signed-off-by: Ryan Cragun <me@ryan.ec>
@ryancragun ryancragun requested a review from a team as a code owner March 24, 2026 16:39

@jimlambrt jimlambrt left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really want to change the strict behaviour of the existing Response.ValidateAssertionSignature()? Would it be better to preserve the strict behaviour and provide a new option for short circuiting when the response envelope is signed?

The other choice would be to change the behaviour and provide a new strict option?

Comment thread saml/response.go Outdated
Co-authored-by: Jim <jlambert@hashicorp.com>
@ryancragun

Copy link
Copy Markdown
Collaborator Author

Do we really want to change the strict behaviour of the existing Response.ValidateAssertionSignature()? Would it be better to preserve the strict behaviour and provide a new option for short circuiting when the response envelope is signed?

The other choice would be to change the behaviour and provide a new strict option?

I had hoped to keep the behavior the same but the upstream changes make that difficult. If the entire response is signed the assertion signature validation happens with a different validator implementation than it previously did and it doesn't keep track of assertion signature validations anymore unless the entire payload isn't signed. If we wanted to keep the strict mode we'd have to implement our own validator which seems like quite a significant change.

@ryancragun

Copy link
Copy Markdown
Collaborator Author

More detail:
First the upstream changed how assertion signatures are validated here: russellhaering/gosaml2@9957448

But that change led to one of the advisories that was fixed later, in that it wasn't validating all of the assertions signatures in a signed response leading to wrap attacks. That was fixed by changing the validation in instances where the response is signed: russellhaering/gosaml2@4ddcc82

If we want to enforce that all assertions are signed and valid, not just that the response and any assertions are valid, we'll have to take on responsibility without the upstream. Unfortunately, the machinery to do that is not exposed in the upstream library as public functions so it will either take exposing that or forking the upstream.

explict about the changes in behavior when signatures
are validated on SAML responses.
@jimlambrt

Copy link
Copy Markdown
Collaborator

Mind adding this to CHANGE.log?

Signed-off-by: Ryan Cragun <me@ryan.ec>
@ryancragun ryancragun requested a review from jimlambrt March 27, 2026 18:15

@jimlambrt jimlambrt left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

everything else looks great.

Comment thread CHANGELOG.md Outdated
Signed-off-by: Ryan Cragun <me@ryan.ec>
@ryancragun ryancragun requested a review from jimlambrt March 27, 2026 18:18

@jimlambrt jimlambrt left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ty.

@ryancragun ryancragun merged commit 2026dd0 into main Mar 27, 2026
7 checks passed
@ryancragun ryancragun deleted the ryan-VAULT-43467 branch March 27, 2026 18:53
suraj-simha pushed a commit that referenced this pull request Apr 15, 2026
…HSA-pcgw-qcv5-h8ch (#180)

* [VAULT-43467] saml: resolve CVE-2026-33487, GHSA-hwqm-qvj9-4jr2, and GHSA-pcgw-qcv5-h8ch

Resolve GHSA-hwqm-qvj9-4jr2[0], GHSA-pcgw-qcv5-h8ch[1], and
CVE-2026-33487[2] by upgrading github.com/russellhaering/gosaml2 to
v0.11.0 and github.com/russellhaering/goxmldsig to v1.6.0.

Due to behavior changes in the upstream libraries we slightly modify
the meaning of the saml.ValidateAssertionSignature() option to
saml.ParseResponse(). In prior versions of gosaml2, when decoding and
validating the responsse assertions, it would mark the SignatureVerified
field as true after validating assertion signatures. That is no longer
the case when the response itself is signed so we cannot rely on that
field to determine both the presensence of and a valid assertion
signature. Therefore, our validation behavior is slightly different
depending on the overall response. When the response envelope is signed
we can no longer enforce that each assertion is signed, only that the
payload is signed and that any assertions that are also signed are valid.
When the response envelope is not signed it falls back to similar path
as before where it marks the SignatureVerified field when validating the
assertion signatures. In this path our prior behavior is preserved.

Ultimately, these changes ought not yield less secure validation as
either the response envelope and all assertions are correctly signed, or
in cases where the envelope is not signed all assertions are. For more
details please refer to the decoder and validator implementations in [3],
[4], and [5].

Along with these updates I've also added a test to verify wrap attacked
assertions are caught as invalid as one would expect.

[0]: GHSA-hwqm-qvj9-4jr2
[1]: GHSA-pcgw-qcv5-h8ch
[2]: GHSA-479m-364c-43vc
[3]: https://github.com/russellhaering/gosaml2/blob/636e7dda202a4d669644e72404a82616ffcbe004/decode_response.go#L255-L289
[4]: https://github.com/russellhaering/gosaml2/blob/636e7dda202a4d669644e72404a82616ffcbe004/decode_response.go#L422
[5]: https://github.com/russellhaering/goxmldsig/blob/878c8c615feb628064040115d00e105a137fcfa7/validate.go#L568-L584

Signed-off-by: Ryan Cragun <me@ryan.ec>
Co-authored-by: Jim <jlambert@hashicorp.com>
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

Successfully merging this pull request may close these issues.

3 participants