Skip to content

CVE-2016-9754 and CVE-2022-0516#225

Open
evstod wants to merge 20 commits intoVulnerabilityHistoryProject:devfrom
evstod:round1
Open

CVE-2016-9754 and CVE-2022-0516#225
evstod wants to merge 20 commits intoVulnerabilityHistoryProject:devfrom
evstod:round1

Conversation

@evstod
Copy link

@evstod evstod commented Nov 10, 2023

No description provided.

@andymeneely
Copy link
Contributor

Looks like this needs to be worked on. Keep going!

Copy link

@mineo333 mineo333 left a comment

Choose a reason for hiding this comment

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

Good work overall. Just a few comments.

Choose a reason for hiding this comment

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

This is not a buffer overflow, but rather an integer overflow - https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=59643d1535eb220668692a5359de22545af579f6

This, under certain situations, can still be found by certain automated testers such as syzkaller

Copy link
Author

@evstod evstod Nov 13, 2023

Choose a reason for hiding this comment

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

You're right. The integer in question controls the buffer size for the ring buffer, which caused me to misidentify the issue. Fixed the note.

Choose a reason for hiding this comment

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

Same comment as above. This is an integer overflow.

Copy link
Author

Choose a reason for hiding this comment

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

You're right. The integer in question controls the buffer size for the ring buffer, which caused me to misidentify the issue. Fixed the note.

answer:
note:
answer: true
note: 'The exploit involves user input to modify a file controlling buffer size.'

Choose a reason for hiding this comment

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

How did you come to this conclusion?

This seems to be a vulnerability in a kernel data structure.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, but the exploit noted in the fix commit involves editing a file that controls the buffer size used in the ring buffer, /sys/kernel/debug/tracing/buffer_size_kb. This is also noted in the vulnerability description on NVD.

Choose a reason for hiding this comment

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

There does not seem to be a buffer involved here.

Copy link
Author

Choose a reason for hiding this comment

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

I meant the integer that controls the buffer size. Fixed to clarify that better.

Choose a reason for hiding this comment

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

2 upvotes

- commit: 83f40318dab00e3298a1f6d0b12ac025e84e478d
note: Discovered automatically by archeogit.
note: 'Discovered automatically by archeogit. Adds the capability to remove pages from a ring buffer without destroying any existing data in it.'
upvotes_instructions: |

Choose a reason for hiding this comment

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

1 upvote

Comment on lines +154 to +157
answer: 'The issue was reported by a Red Hat employee'
automated: false
contest: false
developer: false
Copy link

Choose a reason for hiding this comment

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

Was the method of discovery mentioned by the employee?

Copy link
Author

Choose a reason for hiding this comment

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

It was not. I assume it was a manual find.

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: 7a8e76a3829f1067b70f715771ff88baf2fbf3c3
note: Discovered automatically by archeogit.
note: 'Discovered automatically by archeogit. Introduces the ring buffer.'
- commit: 83f40318dab00e3298a1f6d0b12ac025e84e478d
Copy link

Choose a reason for hiding this comment

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

This bug was in the system for 4 years before being detected. I had a similar CVE with very old code containing a bug. Who knows how many undiscovered bugs are tucked away in large systems. Very interesting.

Comment on lines +239 to +246
- commit: a43b80b782c9f56b3bcc2e5e51261dc3980839ec
note: |
There is 2 years worth of work on this subsystem and even file from the origin
to the fix, but none of them really did anything with the Guest users functionality
except this one. This one adds a global for debugging, KVM_GUESTDBG_VALID_MASK.
Though it has no effect on the specific function of the bug, it is interesting that
permissions debugging systems were made in the bug's subsystem, and yet the bug
remained elusive.
Copy link

Choose a reason for hiding this comment

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

Do you think they were aware there might have been a bug but didn't know the exact location and couldn't replicate it? Or was this preemptive yet still didn't locate it. Either way its unfortunate they walked right over it without seeing the bug.

Copy link

Choose a reason for hiding this comment

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

6 upvotes

Comment on lines +467 to +472
answer: |
This is likely a simple lapse. The function used for making the check on the virtual cpu was created
and intnded to be used, but the check was never placed in along with the rest. The fix was simply adding the check in.
CWE 280 recommends using "safe areas" surrounded by trust boundaries in the design of the subsystem. These were assumably
already in place given the specificness of the other checks in the permissions functionality. The CWE also recommends always
checking that a resource was properlly and successfully accessed. This is now followed since such a check was added.
Copy link

Choose a reason for hiding this comment

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

Was the function to make the check used anywhere after its implementation?

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure. It was not used anywhere in the subsystem at least.

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.

Great work overall, and definitely an interesting vulnerability. Just added a couple minor suggestions to help with consistency and readability, looks like the majority of your information is laid out quite well otherwise though!

Also please add 2 upvotes to CVE-2022-0516

  • I found it interesting how long it took them to finally fix the issue, even after they knew they had a bug in their system

Place any notes you would like to make in the notes field.
vccs:
- commit: 19e1227768863a1469797c13ef8fea1af7beac2c
- commit: '19e1227768863a1469797c13ef8fea1af7beac2c'

Choose a reason for hiding this comment

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

The quotes are unnecessary here since it's a hash value. Not sure if it makes a difference but they can be removed both here and for the hash on line 91

code: false
code_answer: 'There were no surrounding tests. Since this subsystem surrounds permissions. Unit tests handling this type of defect would be difficult.'
fix: false
fix_answer: 'No tests were added. Since this subsystem surrounds permissions. Unit tests handling this type of defect would be difficult.'

Choose a reason for hiding this comment

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

Could be refactored into paragraph format just to stay under the 80 char line limit. Same can be said for lines 320, 334, 350, etc.

and intnded to be used, but the check was never placed in along with the rest. The fix was simply adding the check in.
CWE 280 recommends using "safe areas" surrounded by trust boundaries in the design of the subsystem. These were assumably
already in place given the specificness of the other checks in the permissions functionality. The CWE also recommends always
checking that a resource was properlly and successfully accessed. This is now followed since such a check was added.

Choose a reason for hiding this comment

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

Just a couple grammatical updates here as well that could help with readability, intended and properly

Choose a reason for hiding this comment

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

There are some typos here:
"misculaculation" -> "miscalculation"
"ineger rounding" -> "integer rounding"

Choose a reason for hiding this comment

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

Typo:
"buffer suze" -> "buffer size"

Choose a reason for hiding this comment

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

Typo: "repriduction" -> "reproduction"

Copy link

@karisanno11 karisanno11 Nov 15, 2023

Choose a reason for hiding this comment

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

Typo:
"bahvior" -> "behavior"

Choose a reason for hiding this comment

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

"learly understanding"? Maybe it was supposed to be "clearly understanding"

Choose a reason for hiding this comment

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

Overall great! Your explanations are clear and easy to understand. The minor errors are consistency in capitalization and punctuation across the document.
I would give this a 5.

defense_in_depth:
applies:
applies: false
note:

Choose a reason for hiding this comment

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

Consider adding a note for why it's false. Other documents I've reviewed included this, so it might be good to have it.

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.

6 participants

Comments