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

[GHSA-36xx-7vf6-7mv3] Silverstripe Framework: Members with no password can be created and bypass custom login forms #2575

Conversation

maxime-rainville
Copy link

Updates

  • CVSS
  • Severity

Comments
As indicated in the notice, there's no confidentiality, integrity or availability impact on a vanilla install using the built in authenticator. The problem will only be present if a project has implemented a custom authenticator doesn't validate if a password is present.

GHSA-36xx-7vf6-7mv3

@github
Copy link
Collaborator

github commented Aug 4, 2023

Hi there @emteknetnz and @maxime-rainville! 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 highly-trained 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

@github-actions github-actions bot changed the base branch from main to maxime-rainville/advisory-improvement-2575 August 4, 2023 01:46
@GuySartorelli
Copy link

@darakian
Copy link
Contributor

darakian commented Aug 4, 2023

Hey 👋 , the cvss scoring system is used to determine severity should an attack be pulled off. Section 2.3.3 reads as

2.3.3. Assume Vulnerable Configurations
The explanation of Attack Complexity in CVSS v3.0 considers “the presence of certain system configuration settings”. This text has been removed from CVSS v3.1. If a specific configuration is required for an attack to succeed, the vulnerable component should be scored assuming it is in that configuration, providing it is a reasonable configuration. Unreasonable configurations are those that deliberately place the target in a vulnerable state, e.g., by disabling security features, or that conflict with documented configuration guidance, e.g., by using a non-default configuration that a product vendor explicitly states should never be used.

https://www.first.org/cvss/v3.1/user-guide

Hence our score on this issue.

@GuySartorelli
Copy link

GuySartorelli commented Aug 4, 2023

@darakian

the vulnerable component should be scored assuming it is in that configuration, providing it is a reasonable configuration.

A configuration that allows an empty password to be used to authenticate a user is not a reasonable configuration.

But beyond that, implementing custom authentication logic goes beyond "a configuration". The security of that authentication logic is the responsibility of the person Iimplementing it. There is no vulnerable logic within the application that this CVE was raised for. Any vulnerable logic is being introduced by custom implementations beyond the scope of this repository (which does come with its own secure authentication logic)

We would appreciate if you would reconsider the cvss score with that in mind.

@darakian
Copy link
Contributor

darakian commented Aug 7, 2023

Ok fair. For this one I can go ahead change the impact down to C:N,I:N,A:N, but I do want to point out that it's a technical abuse of the cvss system. CVEs are supposed to have some impact. In the future if you want to make an advisory like this I would suggest not requesting a CVE at all for it. A GHSA alone will still come through us and we will still send out dependabot alerts, but requesting a CVE requires us to rescore the severity and places different requirements on us.

Also, given that I suspect readers will be a bit confused by a zero impact advisory do you think you can flesh out some text on what a user would need to do to be affected? I can add that in to the description as well 😄

@GuySartorelli
Copy link

GuySartorelli commented Aug 7, 2023

Thank you.

Here is our information about severity ratings, including the (non) impact of a CVSS of 0: https://docs.silverstripe.org/en/5/contributing/release_process/#severity-rating

Please see also https://nvd.nist.gov/vuln-metrics/cvss which documents that CVSS scores of 0 are valid according to NIST, and https://docs.veracode.com/r/review_severity_exploitability which shows that other organisations are also using CVSS scores of 0 (or at least consider them to be valid).

If you have some clear documentation showing CVSS scores of 0 being an "abuse" of the CVSS system we'd be happy to review it to help improve our processes.

@advisory-database advisory-database bot merged commit 42c220a into maxime-rainville/advisory-improvement-2575 Aug 7, 2023
@advisory-database advisory-database bot deleted the maxime-rainville-GHSA-36xx-7vf6-7mv3 branch August 7, 2023 21:54
@advisory-database
Copy link
Contributor

Hi @maxime-rainville! Thank you so much for contributing to the GitHub Advisory Database. This database is free, open, and accessible to all, and it's people like you who make it great. Thanks for choosing to help others. We hope you send in more contributions in the future!

@darakian
Copy link
Contributor

darakian commented Aug 7, 2023

If you have some clear documentation showing CVSS scores of 0 being an "abuse" of the CVSS system we'd be happy to review it to help improve our processes.

I don't. You're right that other orgs do use 0.0 and it's an active talking point in the CVE world as to if it's a correct use of the system or not. Again I'll say that out GHSA system has no such requirement.

As for this one. You can see the updated severity on the CVE here
https://cveawg.mitre.org/api/cve/CVE-2023-32302
Not 100% sure when that'll hit the UI or NVD, but it should be in the pipe. Our advisory on github has also been updated (hence the thread got closed), but happy to keep the conversation going here if you like 😄

@GuySartorelli
Copy link

Thanks for that @darakian. I'm satisfied with this outcome - and this is the first time we've ever had an issue we considered to be a CVSS of 0. We're not likely to have another one any time soon so I'm happy to call this concluded for now and we'll see what state things are in if/when we have another one - though @maxime-rainville may have his own questions.

@darakian
Copy link
Contributor

darakian commented Aug 7, 2023

Totally fair and ya it's a weird one for sure. If you do end up having another 0.0 feel free to reach out ahead of publication as well. Happy to help guide things or give feedback or to be someone to yell at or whatever 😄

@GuySartorelli
Copy link

Hi @darakian, we've had a new development with this and I'd like some advice if you're keen to give it.

The good folks at NVD decided that this should have a higher CVSS score, and when we had a similar discussion with them they said (among other things):

The NVD has an operational policy to not provide CVSS vector strings with no impacts and thus result in a 0.0 qualitative severity. As noted in the comments provided and our initial response, we take the position that if a vulnerability has no impacts it should not qualify as a CVE. This directs attention upstream to the CVE Program to ensure proper accountability within the program's processes and participants.

It does appear that the text at https://nvd.nist.gov/vuln-metrics/cvss could use refinement to avoid the incorrect interpretation of our position and policies. ... We will make updates to this page to include these clarifications as time and resources allow.

Based on this, our opinion has now changed on this advisory. Specifically, we will be going with the intepretation above that because this "has no impacts it should not qualify as a CVE".

The advice I'd like from you is how to go about invalidating the CVE associated with this advisory.

@darakian
Copy link
Contributor

darakian commented Oct 3, 2023

Had to refresh my memory, but looks like you all requested the CVE from us so I just need to loop in my team and we can handle it all on our end. We'll reject the CVE with mitre so that anyone reading from the CVE system will see it as REJECTED and we'll withdraw the GHSA on our side so that dependabot alerts stop. The text and all that will remain
GHSA-36xx-7vf6-7mv3
but there will be a little withdrawn tag on it.

The repo advisory is a different object altogether and edits to that are entirely under your control. You can update the text or blank it our or whatever.

So, that's the basic process if you wanna head down that route 😄

@GuySartorelli
Copy link

GuySartorelli commented Oct 3, 2023

Awesome, thank you. For clarity, we'd like to keep the advisory. We want to revoke the CVE but keep the description in the advisory system, the same as if we'd just published it in the advisory system without having ever requested a CVE in the first place.

If you could go ahead and start that process that would be great.

@darakian
Copy link
Contributor

darakian commented Oct 3, 2023

Mmmmmm I think our system will still show the CVE number, but when clicking through it should show rejected. Let me double check and get back to you in the morning (PST), this might be the first time we've ever done a reject of a CVE without also pulling the advisory. It should work though 👍

@GuySartorelli
Copy link

It's fine if it shows the CVE and says it was rejected - the main thing is that the advisory itself is still there and still "functions" (i.e. has or doesn't have the same side-effects on any systems like dependabot) the same as it would if we had never requested a CVE.

@darakian
Copy link
Contributor

darakian commented Oct 4, 2023

@GuySartorelli ok I just put through the rejection
https://cveawg.mitre.org/api/cve/CVE-2023-32302
Not sure how long that'll take to propagate out through every system that reads from mitre, but it should be processing.
On the github side our advisory still does show the CVE number and is basically unchanged 👍
GHSA-36xx-7vf6-7mv3

@GuySartorelli
Copy link

Fantastic, thank you for all your help with this.

@darakian
Copy link
Contributor

darakian commented Oct 5, 2023

Happy to help. Also, if you want this sort of behavior in the future just skip asking for a CVE. 😄

@GuySartorelli
Copy link

GuySartorelli commented Oct 5, 2023

Yup, that's the plan, now that we know what a headache that can be lol

@darakian
Copy link
Contributor

darakian commented Oct 5, 2023

I'll make sure to bring it up with them too. This is the sort of thing I hate in the system where well meaning developers essentially get punished by bureaucracy. No promises or timelines on changes, but I'll at least give them an earful 😉

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.

4 participants