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

CVE-2022-0487 and CVE-2013-2140 #203

Open
wants to merge 17 commits into
base: dev
Choose a base branch
from

Conversation

nolan-white
Copy link

@nolan-white nolan-white marked this pull request as ready for review November 8, 2023 04:22
No automated tests were found in the code surrounding the fix.
fix: false
fix_answer: |
No automated tests were updated in this comming, but Konrad Wilk does

Choose a reason for hiding this comment

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

Should "comming" be "commit"?

Copy link
Author

Choose a reason for hiding this comment

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

Oops, yes that should be "commit".

description of how the vulnerability was discovered, but at that time Wilk
was the director of Xen, the project this driver is part of. Being more
involved with project requirements may have alerted him to this issue since
he wrote the function originally, but this is just speculation.

Choose a reason for hiding this comment

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

I'm not sure if this type of document is the place for speculation. I personally might remove this part of the answer.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah I was on the fence about keeping this in. I'll remove it and replace it with a bit more a description of where I looked.

nickname_instructions: |
A catchy name for this vulnerability that would draw attention it.
If the report mentions a nickname, use that.
Must be under 30 characters. Optional.
nickname:
nickname: Virtual Insanity

Choose a reason for hiding this comment

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

Like the reference!

Copy link
Author

Choose a reason for hiding this comment

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

Thanks!

- commit: ba9d5f83735fc00297e39ba5cd9ece1c61eb3995
note: Discovered automatically by archeogit.
- commit: 6b28f2c4da7e196062d84b143294cf6d86f6e02c
note: Manually confirmed.
upvotes_instructions: |

Choose a reason for hiding this comment

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

Ill give this 4 upvotes! Very interesting

Copy link
Author

@nolan-white nolan-white Nov 14, 2023

Choose a reason for hiding this comment

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

@SwiftWindz Are the upvotes for CVE-2022-0487 or CVE-2013-2140? Just wanted to clarify since all of your other comments were on CVE-2013-2140.

Thanks for reviewing my work! I'll get those changes pushed by tomorrow.

Copy link
Author

Choose a reason for hiding this comment

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

Clarified offline that the upvotes are for CVE-2013-2140.

performing the operation. If an admin provided a guest user read only access
to the disk, the user could still use the discard operation to delete arbitrary
data on the actual disk. This could impact not only the operating system using
the virtual disk, but other operating systems using Xen to access the actual disk.
Copy link

Choose a reason for hiding this comment

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

Very good and concise description of the issue, I like it.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks!

It seems that this vulnerability was the result of overlooking error
paths. There was no evidence prior to the fix that the code was attempting
to prevent sectors being discarded on a read only device, nor was it
checking that the sectors being discarded actually existed.
Copy link

Choose a reason for hiding this comment

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

Seems like the developers of the code that caused the vulnerability made a lot of big assumptions that caused security issues.

@@ -270,8 +295,10 @@ sandbox:
Answer should be true or false
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.

Can't the user potentially erase data stored for another user on the physical disk? Wouldn't that violate sandboxing?

Copy link
Author

Choose a reason for hiding this comment

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

That's a good point. The instructions seem to indicate we should limit this to Linux kernel sandboxing features being violated, so I said no because it isn't violating a sandboxing feature that the Linux kernel provides. That being said, it does seem like it would be violating them in the Xen Project. @andymeneely Since you're overseeing the project, what are your thoughts on this?

nickname_instructions: |
A catchy name for this vulnerability that would draw attention it.
If the report mentions a nickname, use that.
Must be under 30 characters. Optional.
nickname:
nickname: Virtual Insanity
Copy link

Choose a reason for hiding this comment

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

Pretty interesting overall; I enjoyed reading this. 4 upvotes.

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