Skip to content

Conversation

Patrick-Sorensen
Copy link

Initial PR, will be updated over the course of the project

Copy link

@aisgbnok aisgbnok left a comment

Choose a reason for hiding this comment

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

This is good work! 👍 Hopefully I was not too harsh. While I did not review everything for perfect accuracy, if I did not comment on something I most likely think it is fine.

Overall, I think you just need to expand on some of your notes. Specifically I think the description and mistakes answer are the most important for readers. The description explains what the CVE actually is, and the mistake answer explains why it happened and how not to have it happen to you. Take some extra time to craft those notes.

I have requested some changes and provided some thoughts. You do not have to accept all of my suggestions and comments. I only ask that you take them into consideration.

Additional Notes

  • Add 2 upvotes for CVE-2013-0290
  • Add 5 upvotes for CVE-2015-8787
    • I found it interesting that it was a repeated vulnerability.
  • Consider changing the Discovered automatically by archeogit. for the vcc group to Manually Confirmed.
  • Change the cwe note to Manually Confirmed.
  • Ensure you are putting periods at the end of sentences.

Unfortunately, I cannot comment on unchanged lines. (github/roadmap#347)

@@ -75,7 +78,7 @@ bugs_instructions: |
* Mentioned in mailing list discussions
* References from NVD entry
* Various other places
bugs: []
bugs: [911473]

Choose a reason for hiding this comment

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

This is not a kernel bug: https://bugzilla.kernel.org/show_bug.cgi?id=911473

It looks like this is from the RedHat bugzilla.

Suggested change
bugs: [911473]
bugs:
- https://bugzilla.redhat.com/show_bug.cgi?id=911473

Comment on lines 151 to 153
answer: |
The vulnerability was discovered by a developer,
Tommi Rantala <[email protected]>, testing the code with a fuzzer

Choose a reason for hiding this comment

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

I think this could be refined a bit, consider:

Suggested change
answer: |
The vulnerability was discovered by a developer,
Tommi Rantala <[email protected]>, testing the code with a fuzzer
answer: |
The vulnerability was discovered by Tommi Rantala <[email protected]>
by testing the code using Trinity, a Linux system call fuzzer.

I am assuming the trinity in the commit torvalds/linux@3f518bf message is this: https://github.com/kernelslacker/trinity

Tommi Rantala <[email protected]>, testing the code with a fuzzer
automated: false
contest: false
developer: true

Choose a reason for hiding this comment

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

This threw me off too in my work. I think this is false. I can't seem to find information on Tommi Rantala being a Linux Kernel developer or Google developer. I think he was just a contributor. His email is just standard gmail. Confirm before you make the change.

Suggested change
developer: true
developer: false

Edit: I see it was signed off by Eric Dumazet, a Google developer. So I'm not sure what to do for this one...

Comment on lines 173 to 176
note: |
The vulnerability was caused by the system waiting in an infinite loop on a
packet with no payload. The issue was reported by a developer who used a
fuzzer and discovered the issue

Choose a reason for hiding this comment

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

This is a good start, but consider stating that yes this can be discovered through automated means because it was discovered through automated means. Or/and explain a little more why this type of vulnerability is autodiscoverable.

For one of my CVEs i put:

  note: |
    The vulnerability was discovered using Google's syzkaller fuzzer. This
    demonstrates that it's not only possible, but proven, that automated tools
    can be used to uncover similar vulnerabilities.

I think even I could expand on what similar vulnerabilities. means.

Comment on lines 247 to 250
- commit: 3f518bf745cbd6007d8069100fb9cb09e960c872
note: |
Interesting the initial commit which created the issue was made almost
exactly a year before the vulnerability was fixed

Choose a reason for hiding this comment

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

I chose not to put my vulnerability-contributing commits as interesting commits, but I think this is fine.

I would definitely reword that note. It's hard to understand. Consider:

Suggested change
- commit: 3f518bf745cbd6007d8069100fb9cb09e960c872
note: |
Interesting the initial commit which created the issue was made almost
exactly a year before the vulnerability was fixed
commits:
- commit: 3f518bf745cbd6007d8069100fb9cb09e960c872
note: |
The commit that first introduced the issue. It was made approximately one
year prior to the discovery and resolution of the vulnerability.

answer: true
note: |
Both the original commit as well as the fix commit had several developers
sign off on them'

Choose a reason for hiding this comment

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

The wording could be improved, but at least add a period.

Suggested change
sign off on them'
sign off on them.

note:
any_stacktraces: true
stacktrace_with_fix: true
note: 'The stack trace was used to find and fix the issue'

Choose a reason for hiding this comment

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

Suggested change
note: 'The stack trace was used to find and fix the issue'
note: 'The stack trace was used to find and fix the issue.'

Comment on lines 381 to 383
note: |
The vulnerability was caused by a removal of a NULL check, which was then
updated in the fix

Choose a reason for hiding this comment

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

Rewrite this. What does "updated in the fix" mean, how does it relate to a forgotten check?

answer:
note:
answer: false
note: 'The fix did not involve a change in the order of operations'

Choose a reason for hiding this comment

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

Suggested change
note: 'The fix did not involve a change in the order of operations'
note: 'The fix did not involve a change in the order of operations.'

Comment on lines 483 to 487
answer: |
A NULL check was removed,
'if (indev != NULL) {' in favor of 'if (indev && indev->ifa_list) {'
which allowed for a NULL value to be accepted. A similar issue occurred in
2003 but the 2015 commit was not properly reviewed.
Copy link

@aisgbnok aisgbnok Nov 13, 2023

Choose a reason for hiding this comment

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

Expand on this a bit more. Was this a planning issue? A mistake? And how was the 2015 commit not properly reviewed? Remember those reading this won't know all of the context you know from your research.

Comment on lines 247 to 250
- commit: 3f518bf745cbd6007d8069100fb9cb09e960c872
note: |
Interesting the initial commit which created the issue was made almost
exactly a year before the vulnerability was fixed
Copy link

Choose a reason for hiding this comment

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

Although its interesting that there was about a year between discovery and resolution, it doesn't add anything to describing the commit. What was this initial commit for?

Comment on lines 58 to 61
description: |
The __skb_recv_datagram function in net/core/datagram.c in the Linux kernel
didn't handle an MSG_PEEK flag with zero-length data. This locked the system
in an infinite loop and could result in a denial of service.
Copy link

Choose a reason for hiding this comment

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

2 Upvotes

Comment on lines +193 to +197
note: |
TCP specification violation, vulnerability in the socket buffers being
passed into the system. When the SKB is of zero-length it should be skipped
but the missing check results in a DOS.
answer: true
Copy link

Choose a reason for hiding this comment

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

Did you come to this conclusion after researching the CVE or was this mentioned in a bug report or commit?

@@ -473,5 +508,5 @@ nickname_instructions: |
A catchy name for this vulnerability that would draw attention it.
Copy link

Choose a reason for hiding this comment

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

Can't comment unchanged code, so putting this below it:

CWE_note is not filled out. Make sure you look over CWE 20 and confirm it.

@@ -473,5 +509,5 @@ nickname_instructions: |
A catchy name for this vulnerability that would draw attention it.
Copy link

Choose a reason for hiding this comment

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

Again, Make sure to verify the CWE above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants