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.

Sign up for GitHub

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

Jump to bottom

Make Core Files Great Again 🇺🇸🇩🇰 #839

Merged
merged 1 commit into from
Feb 5, 2017
Merged

Make Core Files Great Again 🇺🇸🇩🇰 #839

merged 1 commit into from
Feb 5, 2017

Conversation

zachriggle
Copy link
Member

@zachriggle zachriggle commented Jan 10, 2017 •
edited
Loading

Adds a large amount of functionality around Corefiles, to support smarter exploitation

Adds a ".corefile" property to both local and remote (ssh) processes. See the
documentation for the corefile module for extensive examples and tests.

Additionally, context.delete_corefiles and context.rename_corefiles have been added,
to control the behavior of the ".corefile" property.

@zachriggle zachriggle added this to the Someday milestone Jan 10, 2017
@zachriggle zachriggle self-assigned this Jan 10, 2017
@zachriggle zachriggle force-pushed the corefile-doctests branch 3 times, most recently from eef18e0 to d646046 Compare January 10, 2017 04:39
@zachriggle
Copy link
Member Author

LOL, Travis doesn't generate core files...

@zachriggle
Copy link
Member Author

LMAO @ Travis.

The ubuntu default for /proc/sys/kernel/core_pattern is |/usr/share/apport/apport %p %s %c %P, but /usr/share/apport/apport is not installed inside the container.

@zachriggle
Copy link
Member Author

Sigh, it's not actually possible to get core-dumps on Travis CI.

Setting up apport (2.0.1-0ubuntu17.15) ...
start: Job failed to start
invoke-rc.d: initscript apport, action "start" failed.

@zachriggle
Copy link
Member Author

This is blocked by travis-ci/travis-ci#7135

@zachriggle zachriggle force-pushed the corefile-doctests branch 7 times, most recently from d6161aa to ddae726 Compare January 11, 2017 05:15
@zachriggle
Copy link
Member Author

There's really, really no way to get core dumps to work appropriately inside Linux Containers, unless there is a writable-from-the-container-user-outside-the-container directory which is mapped into the container.

@zachriggle zachriggle force-pushed the corefile-doctests branch 10 times, most recently from 1b6086a to 19b02e4 Compare January 25, 2017 23:15
@zachriggle zachriggle mentioned this pull request Feb 2, 2017
Closed
@zachriggle zachriggle force-pushed the corefile-doctests branch 2 times, most recently from 272b080 to 7f67e34 Compare February 3, 2017 21:32
@zachriggle zachriggle force-pushed the dev branch 2 times, most recently from 19b7132 to f3968af Compare February 3, 2017 22:56
TethysSvensson
TethysSvensson approved these changes Feb 4, 2017
View reviewed changes
Copy link
Contributor

@TethysSvensson TethysSvensson left a comment

Choose a reason for hiding this comment

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

This change is pretty huge and hard to review. This is made worse by the fact that it contains a bunch of (seemingly?) unrelated changes (e.g. dockerfile, flat).

I trust you enough that I don't really care if you put this in dev, but I haven't actually reviewed it in detail. From a surface point of view, this looks really cool though, though there is a lot of the logic I don't have the knowledge to understand in detail.

Please let me know if you actually wanted a thorough review, in which case I can come back to this.

pwnlib/elf/corefile.py Outdated

if not (self.start <= start <= stop <= self.stop):
import pdb
pdb.set_trace()
Copy link
Contributor

Choose a reason for hiding this comment

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

I am guessing that this is a leftover from debugging?

pwnlib/elf/elf.py Outdated
@@ -956,9 +966,13 @@ def read(self, address, count):
memory.chop(stop, None)

if memory.begin() != start:
import pdb
pdb.set_trace()
Copy link
Contributor

Choose a reason for hiding this comment

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

Same for these

@zachriggle
Copy link
Member Author

There were some bits and pieces that were unrelated which made it in because I was doing development locally for some challenges. Should be cleaner now.

@zachriggle zachriggle force-pushed the corefile-doctests branch 3 times, most recently from a5ceaac to ff4ce97 Compare February 4, 2017 21:04
TethysSvensson
TethysSvensson reviewed Feb 5, 2017
View reviewed changes
pwnlib/elf/corefile.py
>>> if os.path.exists('core'): os.unlink('core')

Let's build an example binary which should eat ``R0=0xdeadbeef``
and ``PC=0xcafebabe``.
Copy link
Contributor

Choose a reason for hiding this comment

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

eat?

TethysSvensson
TethysSvensson reviewed Feb 5, 2017
View reviewed changes
pwnlib/elf/corefile.py
return super(Corefile, self).__getattribute__(attribute)

class Core(Corefile):
"""Alias for :class:`.Corefile`"""
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems ugly to have a diffent class for this. Would it be possible to make it actua alias and hack the docs somehow?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the only way to have the class documented

TethysSvensson
TethysSvensson approved these changes Feb 5, 2017
View reviewed changes
Copy link
Contributor

@TethysSvensson TethysSvensson left a comment

Choose a reason for hiding this comment

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

Much easier to review. I might have missed something, especially in the logic in CorefileFinder.

Is it possible to have tests for that part of the code? The logic seems sufficiently convoluted that I'm sure some bugs are hiding in there.

@zachriggle
Copy link
Member Author

Re: Testing CorefileFinder explicitly, it's difficult for reasons surrounding the ability to change kernel.core_pattern inside Travis. 99% of the logic is straightforward, except the bit on extraction from Apport crashes, which will never happen in the standard case -- and I don't care enough to embed a corefile in the doctests.

It is tested indirectly via two mechanisms in the doctests for Corefile, via process.corefile, which uses CorefileFinder. If you look at the coverage, you should see that most of the code gets hit.

@TethysSvensson
Copy link
Contributor

This sounds good to make. SHIP IT.

@zachriggle zachriggle force-pushed the corefile-doctests branch 4 times, most recently from 4dccc05 to 5623ba8 Compare February 5, 2017 14:17
@zachriggle
Make corefiles great again
0980528 
Adds a large amount of functionality around Corefiles, to support smarter exploitation

Adds a ".corefile" property to both local and remote (ssh) processes.  See the
documentation for the corefile module for extensive examples and tests.

Additionally, context.delete_corefiles and context.rename_corefiles have been added,
to control the behavior of the ".corefile" property.
@zachriggle
Copy link
Member Author

CI failure is unrelated, i think it's due to Binutils versions, merging

@zachriggle zachriggle merged commit d160f9b into Gallopsled:dev Feb 5, 2017
@zachriggle zachriggle deleted the corefile-doctests branch February 5, 2017 15:49
@TethysSvensson TethysSvensson modified the milestones: 3.5.0, Someday Feb 17, 2017
@TethysSvensson
Copy link
Contributor

Is it possible to have tests for that part of the code? The logic seems sufficiently convoluted that I'm sure some bugs are hiding in there.

Well, the commit certainly did! Specifically #1164 😄

Kyle-Kyle pushed a commit to Kyle-Kyle/pwntools that referenced this pull request Apr 25, 2021
@takimata @disconnect3d
Compact, [big-endian] hexdump (Gallopsled#839)
f096be7 
* Add options for compact hexdump

* Minor cleanup

* Update pwndbg/commands/hexdump.py

* Update pwndbg/commands/hexdump.py

Co-authored-by: git <[email protected]>
Co-authored-by: Disconnect3d <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TethysSvensson TethysSvensson TethysSvensson approved these changes

Assignees

@zachriggle zachriggle

Labels
docs enhancement
Projects
None yet
Milestone
3.5.0
Development

Successfully merging this pull request may close these issues.
2 participants
@zachriggle @TethysSvensson