Skip to content

Conversation

JoeWesn
Copy link

@JoeWesn JoeWesn commented Nov 9, 2023

No description provided.

Copy link

@Chsalinetti Chsalinetti left a comment

Choose a reason for hiding this comment

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

Overall pretty good, I would just try and go more in depth on some things.

Comment on lines +446 to 447
answer: lapse - Forgot to check if socket was RAW
CWE_instructions: |

Choose a reason for hiding this comment

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

Try and go more in depth about the specifics of this issue.

- commit: 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2
note: Discovered automatically by archeogit.
note: Manually verified
upvotes_instructions: |

Choose a reason for hiding this comment

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

For this vulnerability, I'd give it a 2.

answer:
note:
answer: false
note: Out of order

Choose a reason for hiding this comment

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

What do you mean by out of order?

Write a thoughtful entry here that people in the software engineering
industry would find interesting.
answer:
answer: The main mistake was a lack of critical thinking about order of operations.

Choose a reason for hiding this comment

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

Again, try and go more in depth here with specifics.

- commit: 6bc235a2e24a5ef677daee3fd4f74f6cd643e23c
note: Discovered automatically by archeogit.
note: Manually verified
upvotes_instructions: |

Choose a reason for hiding this comment

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

This vulnerability actually seems pretty interesting given its about USB ports and existed for so long. I'll give it a 6

answer:
note:
answer: false
note: n/a
Copy link

Choose a reason for hiding this comment

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

Check the commit message. I see two acknowledgements of people that signed off.

answer:
note:
answer: false
note: Not part of IPC
Copy link

Choose a reason for hiding this comment

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

How so? It is my understanding that sockets are inherently inter-process.

answer:
answer: lapse - Forgot to check if socket was RAW
CWE_instructions: |
Please go to http://cwe.mitre.org and find the most specific, appropriate CWE
Copy link

Choose a reason for hiding this comment

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

CWE 264 is a category. It may be worth looking for a specific CWE in that category.

note:
distrust_input:
applies:
applies: false
Copy link

Choose a reason for hiding this comment

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

The socket type being sent by the user/process counts as input. This input is not being checked fully. I would propose this to count under distrust_input.

answer:
note:
answer: false
note: Was in drivers.
Copy link

Choose a reason for hiding this comment

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

Expand. The subsystem it's in does not define whether or not it is ipc.

Write a note about how you came to the conclusions you did, regardless of what your answer was.
answer:
note:
answer: false
Copy link

Choose a reason for hiding this comment

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

I see one acknowledgement of a person who signed-off in the commit message

- commit: 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2
note: Discovered automatically by archeogit.
note: Manually verified
upvotes_instructions: |
Copy link

Choose a reason for hiding this comment

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

2 upvotes

- commit: 6bc235a2e24a5ef677daee3fd4f74f6cd643e23c
note: Discovered automatically by archeogit.
note: Manually verified
upvotes_instructions: |
Copy link

Choose a reason for hiding this comment

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

This taught me a lot about the interesting parts of driver handling. 4 upvotes.

Copy link

@Patrick-Sorensen Patrick-Sorensen left a comment

Choose a reason for hiding this comment

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

Overall good work! I made a couple suggestions but for the most part if I didn't comment on something I most likely think it's fine as is.

There's a couple sections you could expand upon, especially the description and mistakes answers. These are most important for readers to understand what the vulnerability was and how to avoid it in the future.

My comments are only suggestions, you don't have to accept all of them but please take them into consideration.

Additionally please add 3 upvotes to CVE-2012-6657

  • I found it interesting that a local user could use RAW sockets in this way, as well as how such a small oversight could crash the whole system

Add 5 upvotes to CVE-2019-15216

  • I found it interesting how long it lasted for before it was found, and the fact a fuzzer was able to finally detect it

security
description:
description: The function sock_setsockopt does not check the type of socket before running tcp_set_keepalive
on it, users could use a RAW socket to crash the system.

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 going into more detail here. Especially could explain a bit better:

  • What is sock_setsockopt and what was it used for?
  • What is tcp_set_keepalive and why is it a problem if the socket type is not checked?
  • What is a RAW socket and how would it result in a system crash?

From the question description, this section should be explained as if you don't have a background in security. From just reading this response it's not quite clear what the vulnerability was. Adding details in layman's terms describing what this issue actually was would be more helpful for future readers.

automated:
contest:
developer:
answer: Discovered by a developer, no used of tools were mentioned.

Choose a reason for hiding this comment

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

Small rewording change could be made here for clarification

Suggested change
answer: Discovered by a developer, no used of tools were mentioned.
answer: Discovered by a developer and no use of automated tools was mentioned.

answer:
note:
answer: false
note: Not part of IPC

Choose a reason for hiding this comment

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

I believe this should instead be true, as the vulnerability was caused by an improper passing of sockets which is an IPC mechanism

answer:
note:
answer: false
note: n/a

Choose a reason for hiding this comment

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

I believe this would be true, as the fix commit has three developers who reviewed and signed off on it
torvalds/linux@3e10986

answer: Discovered by a developer, no used of tools were mentioned.
automated: false
contest: false
developer: false

Choose a reason for hiding this comment

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

The fix commit was signed off by a google employee so the developer flag should be true

nickname:
CVSS:
nickname: Socket type non check.
CVSS:

Choose a reason for hiding this comment

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

Looks like this vulnerability has not yet been provided a 3.1 CVSS score, but instead you could use the 2.0 CVSS from "https://nvd.nist.gov/vuln/detail/CVE-2012-6657"

Suggested change
CVSS:
CVSS: CVSS:2.0AV:L/AC:L/Au:N/C:N/I:N/A:C

why you come to that conclusion.
note:
answer:
note: Yes, a fuzzer found the issue in this case.

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 expanding on it a bit. For instance, you could emphasize that yes this vulnerability can be discovered through automated means because it actually was discovered this way. You could also give a bit more detail on why this type of vulnerability is autodiscoverable.

security
description:
description: When a USB device is unplugged, the system attempts to log a message
using the device's name AFTER it has been unregistered and name deallocated, causing an error.

Choose a reason for hiding this comment

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

This is a good description that is easy to read and understand. I would just mention that this vulnerability results specifically in a NULL pointer dereference which could be exploited to cause a denial of service .

Also try to ensure you remain consistent in not going over the 80 char line limit

answer:
note:
answer: false
note: Out of order

Choose a reason for hiding this comment

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

I believe this may actually be true in your case, since the USB driver created a NULL pointer and the system wasn't able to handle it causing an error, both of which were forgotten checks that the system should have been able to catch before it became a vulnerability.

free to give it a small name and add one in the same format as these.
defense_in_depth:
applies:
applies: false

Choose a reason for hiding this comment

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

This sounds like it could be a defense in depth issue as well, as once the NULL pointer was passed the system had no deeper security to handle the error

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