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

Determine the actual canary location #918

Merged
merged 3 commits into from Nov 17, 2022
Merged

Conversation

clubby789
Copy link
Contributor

Description/Motivation/Screenshots

This PR adds arch-specific methods to find the actual canary location, as stored in $fs_base+0x28 on x86_64.
This allows printing the in-use canary location, which can help with exploits that involve overwriting the canonical canary.

I wasn't able to find a way to read the gs segment on 32-bit, so currently this PR only adds functionality to x86_64.

Against which architecture was this tested ?

"Tested" indicates that the PR works and the unit test (see docs/testing.md) run passes without issue.

  • x86-32
  • x86-64
  • ARM
  • AARCH64
  • MIPS
  • POWERPC
  • SPARC
  • RISC-V

Checklist

  • My PR was done against the dev branch, not main.
  • My code follows the code style of this project.
  • My change includes a change to the documentation, if required.
  • If my change adds new code, adequate tests have been added.
  • I have read and agree to the CONTRIBUTING document.

@clubby789
Copy link
Contributor Author

Failure looks unrelated, but my other PR passed so I'm unsure

gef.py Outdated Show resolved Hide resolved
gef.py Outdated Show resolved Hide resolved
tests/commands/canary.py Outdated Show resolved Hide resolved
@clubby789
Copy link
Contributor Author

A couple things I'm not sure how to deal with

  • I believe it would be nice to inform the user when the canary is read from the non-canonical auxv. However, this information is a bit confusing in the general case where a user just wants to know the canary value, and seems to get spammed a bit on launch
  • The cached ._canary property wasn't being used, and I've added a check that returns it if it's already been cached. However, if the canary location (e.g. spawning a thread) or the canary itself (e.g. via exploitation) changes, the cached value is now innacurate so it might be best not to cache this at all

gef.py Outdated Show resolved Hide resolved
gef.py Outdated Show resolved Hide resolved
@Grazfather
Copy link
Collaborator

The cached ._canary property wasn't being used, and I've added a check that returns it if it's already been cached. However, if the canary location (e.g. spawning a thread) or the canary itself (e.g. via exploitation) changes, the cached value is now innacurate so it might be best not to cache this at all

Yes, just noticed via your change, missed this comment. We also do this for a couple other things e.g. pagesize.

How often is this function called? We probably shouldn't cache.

@clubby789
Copy link
Contributor Author

It might be worth only caching/checking cache in the exception case - the address of AT_RANDOM isn't going to change, and it's unlikely/irrelevant that it would be overwritten there - then again, it seems to be called a few times on startup, then only when canary is invoked by the user, so caching is probably unecessary in general here

@Grazfather
Copy link
Collaborator

Let's not cache, and figure out what to do in #919.

tests/commands/canary.py Outdated Show resolved Hide resolved
@clubby789
Copy link
Contributor Author

Still no idea about the failing tests :/ It looks persistent, but dev seems to be passed fine

@Grazfather
Copy link
Collaborator

You're writing a 4 byte canary and testing for it, It'll fail if there is anything but 0s in the other 4 bytes.

[+] The canary of process 3439 is at 0x7ffff7fb8568, value is 0x7f00deadbeef"

@Grazfather
Copy link
Collaborator

The other test is failing because you very slightly lower the coverage score... @hugsy should probably fix it, it's way too stringent right now.

Copy link
Owner

@hugsy hugsy left a comment

Choose a reason for hiding this comment

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

Good idea, minor stuff to change before we can merge.

tests/commands/canary.py Outdated Show resolved Hide resolved
tests/commands/canary.py Outdated Show resolved Hide resolved
gef.py Outdated Show resolved Hide resolved
gef.py Outdated Show resolved Hide resolved
gef.py Outdated Show resolved Hide resolved
Copy link
Owner

@hugsy hugsy left a comment

Choose a reason for hiding this comment

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

LGTM

@hugsy hugsy merged commit 63ac481 into hugsy:dev Nov 17, 2022
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