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

Add paragraph on ROP gadgets and variable length instructions #245

Merged
merged 1 commit into from
May 14, 2024

Conversation

wanders
Copy link
Contributor

@wanders wanders commented May 6, 2024

This change is Reviewable

Copy link
Member

@kbeyls kbeyls left a comment

Choose a reason for hiding this comment

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

Thank you!
This looks good, just have left a few nit picks.

Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, all discussions resolved


book.md line 630 at r1 (raw file):

that should be protected and have been missed.

On architectures such as x86 that has variable length instruction encoding

s/has/have/


book.md line 631 at r1 (raw file):

On architectures such as x86 that has variable length instruction encoding
where instructions doesn't have any alignment requirements there will also

s/doesn't/do not/


book.md line 632 at r1 (raw file):

On architectures such as x86 that has variable length instruction encoding
where instructions doesn't have any alignment requirements there will also
exist gadgets that the compiler has not emitted as instructions. For example,

maybe s/exist/be/? But this is a matter of taste, so if you prefer "exist", let's keep "exist".

@wanders
Copy link
Contributor Author

wanders commented May 7, 2024

book.md line 632 at r1 (raw file):
I think I prefer "be". But maybe even

On architectures such as x86 that has variable length instruction encoding
where instructions doesn't have any alignment requirements it is possible
to find gadgets that the compiler has not emitted as instructions.

Copy link
Member

@kbeyls kbeyls left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, all discussions resolved


book.md line 632 at r1 (raw file):

Previously, wanders (Anders Waldenborg) wrote…

I think I prefer "be". But maybe even

On architectures such as x86 that has variable length instruction encoding
where instructions doesn't have any alignment requirements it is possible
to find gadgets that the compiler has not emitted as instructions.

Oh yes, that's better!

@g-kouv g-kouv requested a review from kbeyls May 7, 2024 13:09
Copy link
Collaborator

@g-kouv g-kouv left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @kbeyls and @wanders)


book.md line 632 at r1 (raw file):

On architectures such as x86 that has variable length instruction encoding
where instructions doesn't have any alignment requirements there will also
exist gadgets that the compiler has not emitted as instructions. For example,

If we're mentioning variable-length instructions, it might also be worth mentioning that this can also happen in other cases, e.g. literal pools embedded in the code, read-only data that gets mapped in the same segment as code etc. This kind of thing is particularly interesting in JITs when the input code (and therefore these values) is attacker controlled.

@wanders
Copy link
Contributor Author

wanders commented May 7, 2024

book.md line 632 at r1 (raw file):

Previously, g-kouv (Georgia Kouveli) wrote…

If we're mentioning variable-length instructions, it might also be worth mentioning that this can also happen in other cases, e.g. literal pools embedded in the code, read-only data that gets mapped in the same segment as code etc. This kind of thing is particularly interesting in JITs when the input code (and therefore these values) is attacker controlled.

Good point. I'll see if I can cover that too.

Copy link
Member

@kbeyls kbeyls left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed (commit messages unreviewed), 1 unresolved discussion (waiting on @g-kouv)


book.md line 632 at r1 (raw file):

Previously, wanders (Anders Waldenborg) wrote…

Good point. I'll see if I can cover that too.

I like the text, it reads well and explains the idea well.
Georgia, what do you think?


book.md line 649 at r2 (raw file):

`66 b8 5f c3`. If the attacker can divert execution two bytes into that 4-byte
instruction it will execute `5f c3`. Those bytes corresponds to the two single
byte instructions `pop %rdi; ret` which is an useful ROP gadget.

s/an useful/a useful/?

Copy link
Member

@kbeyls kbeyls left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @g-kouv)


book.md line 632 at r1 (raw file):

Previously, kbeyls (Kristof Beyls) wrote…

I like the text, it reads well and explains the idea well.
Georgia, what do you think?

Georgia may not be available to do another review in the near future.
Let's land this improvement to the text.
We can always adapt it further in the future.

@kbeyls kbeyls merged commit 23cd930 into llsoftsec:main May 14, 2024
1 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants