-
Notifications
You must be signed in to change notification settings - Fork 412
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
kernelCTF: added CVE-2024-1086 lts mitigation #96
Conversation
Apparently docs/exploit.md got corrupted locally in between git commits. It seems like I'll need to rewrite a large part of docs/exploit.md again. I hope I can get it done before you're done evaluating the older entries 👍 |
All documentation is completed now and the PR is ready for merge in my eyes. Force-push above was required to correct git email in a commit (required due to CLA). The only thing left to fix is the attached files (due to static glibc compiles causing trouble), as spoken with KT, but they would look into it. Feel free to give feedback :-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made a few suggestions, please take a look!
|
||
### Setup for `CONFIG_FREELIST_HARDENED` naive DF detection bypass | ||
|
||
In order to bypass `CONFIG_FREELIST_HARDENED`'s naive double-free detection, we simply allocate an skb (remove from the freelist) before the double-free, which we free (append to the freelist) in between the double-free. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add references like [1], [2], [3] into this text and the code below which lines do the followings:
- "we simply allocate an skb (remove from the freelist) [1] before"
- "the double-free [2]"
- "we free (append to the freelist) [3] in between the double-free."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in af171cb 👍
I tried to prevent boilerplate in the write-up by just linking the other double-free code. Is this acceptable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you use relative paths, e.g. ../exploit/lts-6.1.72/src/exploit.c#L362
instead of https://github.com/Notselwyn/security-research/blob/a4da96327b8bd940e4d5ae0c565d060b11e54ce7/pocs/linux/kernelctf/CVE-2024-1086_lts_mitigation/exploit/lts-6.1.72/src/exploit.c#L362, GH should be able to handle this link format after we merge the change.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in e79a21a 👍
pocs/linux/kernelctf/CVE-2024-1086_lts_mitigation/docs/exploit.md
Outdated
Show resolved
Hide resolved
It would be useful to link to your blog post in the writeup (https://pwning.tech/nftables/) because some techniques like dirty pagedirectory are explained in more detail there. |
Great feedback, thanks! I'll try to fix it before the end of the week |
Thanks for the feedback. I believe my explanation may have accidentally misled you: it does not implement recursive pagetable-like mechanisms. It simply causes an arbitrary Page Upper Directory (PUD) and arbitrary Page Middle Directory (PMD) to be overlapped (called "PUD+PMD" in the writeups). Hence no recursion happens, as the overlapping PMD is not a child of the overlapped PUD, but is the child of a normal PUD. Do you have ideas for I could clear this up in the write-ups? I tried to convey this in the original diagram by showing that the table index used differs between the PUDs |
Ah I see, yes now the diagram makes more sense. Yeah I don't really know how to make that clearer, maybe you could add the note from your latest comment to the writeup. |
Done 👍 @koczkatamas can I resolve the changes you requested? |
Thanks! I've made one comment. The review will be continued by @matrizzo, but he is out-of-office this week, so probably next week earliest. |
@matrizzo This seems to have fallen through the cracks. :^) |
Yeah, I think it would be great if we could continue with the review :-) |
Hey! Sorry for the long process... Meanwhile you could please take a look at another nftables submissions (we got a ton) and how did they solve that they don't include the Linux kernel header files? This also needs to be resolved before we can merge the commit. I will ping @matrizzo to try to finish the review this week. |
@@ -0,0 +1,205 @@ | |||
# novel-techniques | |||
|
|||
TODO: increase description granularity for techniques proven to be novel. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see there is still a TODO here, do you want to make any changes before we merge this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for picking it up. If you believe the explanations are detailed enough, I'm fine with the way it currently is :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please still look into if you can exclude the header files?
There are several other nftables submissions in the repo and in the PRs. Is there anything different in your submission which does not make this possible?
@koczkatamas @matrizzo it took a couple of tries but I got rid of the headers 👍 |
Looks good, thanks for the submission |
No description provided.