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-xq3w-v528-46rv] Denial of Service attack on windows app using netty #5032

Conversation

AB-xdev
Copy link

@AB-xdev AB-xdev commented Nov 22, 2024

Updates

  • CVSS v4
  • Severity

Comments
Removed CVSSv4 as it was never declared or used in the original report and contains incorrect values.
Used CVSSv3 instead which should have the correct values.

netty/netty#14473

@github
Copy link
Collaborator

github commented Nov 22, 2024

Hi there @normanmaurer! 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

@github-actions github-actions bot changed the base branch from main to AB-xdev/advisory-improvement-5032 November 22, 2024 08:11
@darakian
Copy link
Contributor

Hey @AB-xdev, would it make more sense to update the cvss 4 score? I think I agree with you about the confidentiality and integrity scores and it looks like an error on our side when adding the v4 score.

@AB-xdev
Copy link
Author

AB-xdev commented Dec 2, 2024

@darakian

Hey @AB-xdev, would it make more sense to update the cvss 4 score?

No, a CVSS v4 score was not declared in the original advisory as it uses CVSS v3, so the CVSS v4 was obviously made-up.
I have no intention to fix the made-up score when there is a correct one already present.

Feel free to re-add a valid CVSSv4 score afterwards.

@darakian
Copy link
Contributor

darakian commented Dec 2, 2024

@AB-xdev well... we added the cvss 4 score 😄

@AB-xdev
Copy link
Author

AB-xdev commented Dec 3, 2024

@AB-xdev well... we added the cvss 4 score 😄

Okay, I'm a bit confused now.
Could you please explain why you are adding a (incorrect/bogus) CVSS v4 score, when a valid CVSS v3 score is present in the first place?
And why only here? There are other advisories (e.g. GHSA-5jfw-gq64-q45f) that only use a CVSS v3 score and are totally fine.
Also what's the problem with removing the incorrect CVSS v4 score for now so that there are at least no made-up values here?

The current situation makes 0 sense to me.

@darakian
Copy link
Contributor

darakian commented Dec 3, 2024

Well the goal isn't for them to be incorrect. Like I mentioned above I think we made a mistake here. We try to enrich data as we ingest it and some customers like having cvss 4 scores hence the v4 score.

Also what's the problem with removing the incorrect CVSS v4 score for now so that there are at least no made-up values here?

Well, I'd rather simply correct the score since we're working on it now :)
Re-reading the issue I think CVSS:4.0/AV:N/AC:L/AT:P/PR:L/UI:N/VC:N/VI:N/VA:H/SC:N/SI:N/SA:N might be a fair v4 score. What do you think?

@AB-xdev
Copy link
Author

AB-xdev commented Dec 3, 2024

Re-reading the issue I think CVSS:4.0/AV:N/AC:L/AT:P/PR:L/UI:N/VC:N/VI:N/VA:H/SC:N/SI:N/SA:N might be a fair v4 score.

Sorry, but I have to object:

Metric Old value Your proposed value My proposal Note
Attack Vector (AV) Local ↑ Network ❌ Don't change at all The CVE talks about custom files that must be present on the system for the DoS to happen. There is 0 chance that you can create custom files via network without a previous security problem.
Attack requirement (AT) None ↓ Present ❌ Don't change at all "The successful attack depends on the presence of specific deployment and execution conditions of the vulnerable system that enable the attack. These include: A race condition must be won to successfully exploit the vulnerability. ..." There is no requirement/condition for this CVE. You simply create a big file on Windows and trigger an OOM.
Subsequent System Confidentiality (SC) High ↓ None ✔️ None
Subsequent System Integrity (SI) High ↓ None ✔️ None
Subsequent System Availability (SA) Low ↓ None ❌ Don't change at all I think the original is fine: The java application crashes but the rest of the machine will generally still work. If e.g. a another (web) service requires the crashed app that service might fail. If the java application is installed as a Windows service with a restart policy it might enter a reboot-loop liekly causing excessive CPU usage.

We try to enrich data as we ingest it and some customers like having cvss 4 scores hence the v4 score.

Okay.... I still see multiple problems here:

  • This is only done for certain CVEs? Why not all? Why only those?
  • The process how these values are calculated is not transparent... How are you creating these values/What's the workflow? What's your calculation basis? Do you inform the actual CVE creators?
  • Your values (CVSSv4) do not match the CVE descriptions (it only contains explanation for CVSS v3) leading to confusion.
  • Your values are not marked as "computed from CVSSv3 by GitHub" or something similar anywhere.
  • Why isn't the original CVSS score used for computing the severity?

I think you guys should stop doing this immediately before more harm can be done by incorrect CVE scores.

@darakian
Copy link
Contributor

darakian commented Dec 3, 2024

Ah you're very right about attack vector. On the attack requirements I took that to be access to either a local account or ability to get a config file into the local environment. AV:L is a better approach. Also apologies on Subsequent System Availability. That should be Vulnerable System Availability 🤦

To your series of questions. We do this as the advisories come up and we have these conversations in public as needed. This is a very manual process and its a balancing act to be sure. With respect to this advisory we're now talking about the cvss v4 score of
CVSS:4.0/AV:L/AC:L/AT:N/PR:N/UI:N/VC:N/VI:N/VA:L/SC:N/SI:N/SA:N
which scores as a medium and is in line with the cvss 3 score on record. Does this seem reasonable?

@AB-xdev
Copy link
Author

AB-xdev commented Dec 4, 2024

We do this as the advisories come up and we have these conversations in public as needed. This is a very manual process and its a balancing act to be sure.

It looks like this process is extremely nontransparent, which completely erodes (my) trust in this - security critical - system.
I see no public conversations or documentation anywhere, please link one if these exist.

CVSS:4.0/AV:L/AC:L/AT:N/PR:N/UI:N/VC:N/VI:N/VA:L/SC:N/SI:N/SA:N

Again you are trying to change some unrelated metrics:

  • Priviledges Required (PR) is now NONE. Why? You need to have access to the system to create files -> Leave it as it was: LOW
  • Availability Impact to the Vulnerable System (VA) was changed to LOW? Low = Performance is reduced... High = There is a total loss of availability.... If the app dies due to OOM it's dead and not "reduced" -> Leave it as it was: HIGH
  • Availability Impact to the Subsequent System (SA) was changed to NONE? See my comment above.

Did you read my original linked issue?

... has a few unexpected values that result in conflicts:

  • Subsequent System Confidentiality is HIGH. This should likely be NONE as only a DoS occurs.
  • Subsequent System Integrity is also set to HIGH. This should also likely be NONE (see above)

Change this and NOTHING else, as only these values are obviously incorrect - if you want to reintroduce CVSS v4 at all costs.

I applied the changes myself (took me a few seconds to re-select 2 metrics on a website) and I get CVSS:4.0/AV:L/AC:L/AT:N/PR:L/UI:N/VC:N/VI:N/VA:H/SC:N/SI:N/SA:L/E:P with a score of 5.4 instead of 7.

@irene221b
Copy link

I've just submitted a PR questioning your CVSS 4 score in a similar manner, but this PR is much better, I hope you merge it ASAP.

@advisory-database advisory-database bot merged commit fd84ffd into AB-xdev/advisory-improvement-5032 Dec 4, 2024
2 checks passed
@advisory-database
Copy link
Contributor

Hi @AB-xdev! 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!

@advisory-database advisory-database bot deleted the AB-xdev-GHSA-xq3w-v528-46rv branch December 4, 2024 18:06
@darakian
Copy link
Contributor

darakian commented Dec 4, 2024

Ok, so in the interest of brevity we've gone ahead and merged an update. I think this aligns with how you see the severity as well as with the v3 score from the source document.

It looks like this process is extremely nontransparent, which completely erodes (my) trust in this - security critical - system.
I see no public conversations or documentation anywhere, please link one if these exist.

We're having a public conversation right now :)
This repo is the public record of changes to the database. Feel free to dig around. As for documentation; we've got
https://docs.github.com/en/code-security/security-advisories/working-with-global-security-advisories-from-the-github-advisory-database/about-the-github-advisory-database#github-reviewed-advisories
and
https://docs.github.com/en/code-security/security-advisories/working-with-global-security-advisories-from-the-github-advisory-database/editing-security-advisories-in-the-github-advisory-database
which may address some of your concerns

You are correct on the privileges. Setting that to none was an error on my part in copy+pasting things around and I would encourage you to give us a little empathy. We deal with a lot advisories on a daily basis as well as users telling us we're doing it wrong. We do our best and I'm glad we've done well enough to be regarded as security critical :)

So, with respect to the availability in both the primary and subsequent systems. The cvss v4 doc
https://www.first.org/cvss/v4.0/specification-document
Subsequent systems are an undefined term. This leaves the term up to the interpretation of the analyst and could be a separate process running in a single kernel, a separate machine or anything in between. The way I read your comments I think you see things from the perspective of a single process. I can see that viewpoint though my initial perspective was to see the OS as the system with the death of a process then being a partial availability issue. It does come down to a matter of interpretation though.

@AB-xdev
Copy link
Author

AB-xdev commented Dec 5, 2024

Ok, so in the interest of brevity we've gone ahead and merged an update. I think this aligns with how you see the severity as well as with the v3 score from the source document.

Thank you, however they got immediately overwritten by #5056 :(

We're having a public conversation right now :)

I was referring to existing conversations.

This repo is the public record of changes to the database. Feel free to dig around.

No this repo is the database. It does not contain an easily observable change that describes the created CVSS v4 scores.

As for documentation; we've got ...

None of these links describes that you create CVSS v4 scores anywhere.

You are correct on the privileges. Setting that to none was an error on my part in copy+pasting things around and I would encourage you to give us a little empathy. We deal with a lot advisories on a daily basis as well as users telling us we're doing it wrong. We do our best and I'm glad we've done well enough to be regarded as security critical :)

I understand that and I'm also glad that this database exists. However as iterated above, please be more transparent!

So, with respect to the availability in both the primary and subsequent systems. The cvss v4 doc
https://www.first.org/cvss/v4.0/specification-document
Subsequent systems are an undefined term. This leaves the term up to the interpretation of the analyst and could be a separate process running in a single kernel, a separate machine or anything in between. The way I read your comments I think you see things from the perspective of a single process. I can see that viewpoint though my initial perspective was to see the OS as the system with the death of a process then being a partial availability issue. It does come down to a matter of interpretation though.

CVSS v4 specifies:

The Impact metrics reflect the direct consequence of a successful exploit, and represent the consequence to the “things that suffer the impact”, which may include impact on the vulnerable system and/or the downstream impact on what is formally called the “subsequent system(s)”.

But yes that sounds like a design problem of CVSS v4, as it's quite hard to say what the "subsequent system" is (e.g. other machines relying on the service, other processes on affect machine). Sounds like another reason why we should maybe stick with the original scores then or write a description why the values have been chosen ;)


Anyway I will spin off this PR into a separate issue to propose a general change.

@darakian
Copy link
Contributor

darakian commented Dec 5, 2024

Thank you, however they got immediately overwritten by #5056 :(

Did it? Looks correct to me on the main view
GHSA-xq3w-v528-46rv

I was referring to existing conversations.

Here are a few of my favorites
#57
#103
#401 <- more cvss ambiguity
#2258 <- some cool researchers sharing their work

You do need to dig for them, but everything is indexed on the GHSA number so it should be easy to find on a per advisory basis.

I understand that and I'm also glad that this database exists. However as iterated above, please be more transparent!

It's a constant effort. There aren't many of us and the operational stuff always has priority. We do have updated and expanded docs ongoing and I'll link this thread in to the issue to try and give it more urgency. For any other specifics on how to improve transparency feel free to make some issues 👍

Anyway I will spin off this PR into a separate issue to propose a general change.

Right on. Again, not many of us so big changes might take a while to give a good/complete answer to.

Also, if you feel strongly about severities it may make sense to reach out to the cvss group to iterate on the design
https://www.first.org/cvss/
I would just email them asking how to get involved since their website doesn't make that super clear.

@AB-xdev
Copy link
Author

AB-xdev commented Dec 6, 2024

Did it? Looks correct to me on the main view

  1. Open GHSA-xq3w-v528-46rv and note that it says CVSS v4. I deleted CVSS v4 here in this PR.
  2. Open merge commit fd84ffd. Notice the gigantic orange "This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository." warning
  3. Open https://github.com/github/advisory-database/blob/main/advisories/github-reviewed/2024/11/GHSA-xq3w-v528-46rv/GHSA-xq3w-v528-46rv.json and notice that the CVSS v4 ist still there

I was referring to existing conversations.

Here are a few of my favorites
#57
#103
#401 <- more cvss ambiguity
#2258 <- some cool researchers sharing their work

You do need to dig for them, but everything is indexed on the GHSA number so it should be easy to find on a per advisory basis.

You know that I'm talking about YOU CREATING CVSS v4 SCORES and not general communication or?


Rest of your statements look fine :)

@darakian
Copy link
Contributor

darakian commented Dec 6, 2024

and note that it says CVSS v4. I deleted CVSS v4 here in this PR.

Ah right. 🤦
I was going off the conversation we started having about a "correct' v4 score. Well, if (for the moment at least) the current v4 score is ok then all's well (for the moment at least)?

You know that I'm talking about YOU CREATING CVSS v4 SCORES and not general communication or?

It was ambiguous. You were talking about transparency in a fairly general way and I wanted to reassure you that we do try very hard to be transparent. It's something I personally care quite a bit about. I agree we can do better about documenting what enrichment we do independently and its valid to call out our shortcoming on that front.

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