[GHSA-j5g2-q29x-cw3h] SimpleSAMLphp vulnerable to XXE in parsing SAML messages

[sandermarechal]

Updates

• Affected products
• Source code location

Comments
The vulnerability exists in the simplesamlphp/saml2 package, not in the simplesamlphp/simplesamlphp package. It is possible to update simplesamlphp/saml2 without updating simplesamlphp/simplesamlphp. A simple composer update will work and pull in the updated simplesamlphp/saml2. In that case even older versions of simplesamlphp/simplesamlphp are safe.

[github]

Hi there @tvdijen! A community member has suggested an improvement to your security advisory. If approved, this change will affect the global advisory listed at github.com/advisories. It will not affect the version listed in your project repository.

This change will be reviewed by our Security Curation Team. If you have thoughts or feedback, please share them in a comment here! If this PR has already been closed, you can start a new community contribution for this advisory

[tvdijen]

The SimpleSAMLphp developers do not agree with this change.

You cannot assume everyone uses composer. Many of our users rely on our pre-built tarball and it is crucial that they update. We created this advisory deliberately like this.

Please close this PR.

[sandermarechal]

The problem is that this advisory generates composer audit errors for people who do use composer, and also creates conflicts for people who use roave/security-advisories (https://github.com/Roave/SecurityAdvisories). They both use this advisory. I am forced to remove roave/security-advisories from my dependencies because of the conflict with simplesamlphp <2.0.15, even though I am not vulnerable at all!

[tvdijen]

So you're running an old an unsupported version of SimpleSAMLphp and backport security-fixes by updating the dependencies? Even if you do use composer, it's generally bad practise to bypass the lock-file and composer update. It will leave you with an untested set of dependencies and is likely break things sooner or later. SimpleSAMLphp may follow semantic versioning, but there's no guarantee that all our secondary and tertiary dependencies do.

[sandermarechal]

We require SimpleSAMLphp as a package in our own application. Composer always ignores lockfiles from dependencies. If that is not a supported use-case then you should probably not publish simplesamlphp/simplesamlphp as a package on packagist.

That said, if I have an updated simplesamlphp/saml2 then simplesamlphp/simplesamlphp <2.0.15 is not vulnerable to this XXE, right? Then this security advisory is not valid as it is.

[tvdijen]

I wouldn't know, because that combination of versions is not tested. Suit yourself if you want to take a risk!
I can't just ignore our tarball-users on a vulnerability like this.

[shelbyc]

👋 Hi @sandermarechal, thank you for your concern about GHSA-j5g2-q29x-cw3h, but I'm going to leave the advisory as it currently is. It's valid for the maintainers of https://github.com/simplesamlphp/simplesamlphp to want to warn users about a vulnerability for which they may need to take action.

[shelbyc]

@sandermarechal and @tvdijen I showed this thread to some of my colleagues, and I came to realize that I was too hasty in closing the pull request. I'm commenting in the thread to continue the conversation and explore options for alerting users of a possible vulnerable dependency while minimizing unnecessary alerts. There are a few courses of action that could occur, each with pros and cons, and I want to get your input on each of them.

1. Change the advisory to list simplesamlphp/saml2 as the affected product, as @sandermarechal suggested. This is slightly more accurate, but simplesamlphp/saml2 is already listed as affected in GHSA-2x65-fpch-2fcm, so listing simplesamlphp/saml2 in GHSA-j5g2-q29x-cw3h is unnecessary.
2. Withdraw the global advisory for GHSA-j5g2-q29x-cw3h with a message that the underlying information is accurate, but the alerts on the Composer package are not accurate because the vulnerability primarily affects users of the SimpleSAMLphp tarball. This option doesn't affect the repository advisory (GHSA-j5g2-q29x-cw3h) at all but withdraws the alerts from the simplesamlphp/simplesamlphp Composer package. Some users might be confused about why the advisory was withdrawn if the underlying issue is valid, but the GitHub Advisory Database issues similar withdrawals for legitimate advisories under other circumstances, e.g. when an advisory is valid but duplicates another advisory.

I'm curious to hear what both of you think of these options and which one is the best for reaching users of SimpleSAMLphp who need to see information about CVE-2024-52596 while minimizing unnecessary alerts.

[tvdijen]

I never asked for the advisory to be published in the GitHub Advisory Database or for Dependabot-alerts to be sent in the first place.. It's out of my control.

So option 3: It would be helpful if this behaviour was configurable when creating a repository advisory.
This way I can inform my tarball-users and I don't have to bother anyone with unnecessary alerts.

[sandermarechal]

@shelbyc @tvdijen Another option may be to use a different "ecosystem" tag. Right now the ecosystem is listed as "Composer", which is why composer, packagist and roave/security-advisors picked this up automatically. I tried suggesting a different ecosustem using the github suggestion UI, but "ecosystem" is a required select box, and "Composer" is the only option that comes even close. Maybe ecosystem should not be a required field? Or maybe "tarball" or "download" or "Github release" should be an ecosystem?

[tvdijen]

General consensus: we need more flexibility on different levels

[shelbyc]

GHSA-j5g2-q29x-cw3h has been withdrawn with a message that, although the underlying information is valid, those who are affected are users of the tarball, not users of the Composer package.

@sandermarechal I'm not sure if Another option may be to use a different "ecosystem" tag. was directed to me, but for the global GitHub Advisory Database we limit our scope to our supported ecosystems. Using a different ecosystem tag is a good approach for repository advisories, and we see examples of this in repository advisories that can't be published to the global database.

Also, do you still want credit on the global advisory? If so, I can get help from colleagues to make that happen.

@tvdijen I'm happy to pass along your feedback about wanting greater flexibility in the repository advisory publication process. For now, it is possible to do something similar to what @sandermarechal suggested, and use the Other ecosystem to specify something like Tarball in the repository advisory. Here is an example of a repository advisory that clarifies which parts of the software are affected: GHSA-fgrf-2886-4q7m

[tvdijen]

Thanks @shelbyc ! I wasn't aware of the fact that selecting composer would have these consequences. Now that I'm aware of this, I will update the repository advisory as well!
If this would have been more obvious, this may not have happened at all.
Thanks for your help and for passing on my feedback!