Loader: ELF non-zero based PIC binaries support #67

symflood · 2017-06-19T01:36:53Z

Watch these fixes please.
I think they should resolve an issue #65. Unfortunately, the tests from angr/angrop will not pass. There some work left, but not so much. Don't know if the solution will fit. Didn't find anything less invasive.

symflood · 2017-06-19T01:59:23Z

That's bad. Didn't even passed cle tests.

rhelmot · 2017-06-19T02:07:58Z

The CLE tests passed. The problem is you have code quality regressions so linting failed. You should make your changes pass pylint with the pylintrc in the angr-dev repository.

ltfish · 2017-06-19T05:26:31Z

@amttr Can you please rename address.py to address_translator.py? Other than that and the linting fixes, the code looks perfect to me. Thank you :-)

symflood · 2017-06-19T19:50:51Z

Travis failed to build, despite that everything goes ok on a local machine

    'No matching distribution found for %s' % req
DistributionNotFound: No matching distribution found for identifier (from patcherex==1.1)
[!!] pip failure (-e patcherex). Check /tmp/setup-10851 for details, or read it here:

The command "( curl https://raw.githubusercontent.com/angr/angr-dev/$TRAVIS_BRANCH/tests/travis-install.sh | grep -v 404 || curl https://raw.githubusercontent.com/angr/angr-dev/master/tests/travis-install.sh ) | bash" failed and exited with 1 during .

ltfish · 2017-06-20T07:26:21Z

@amttr That is our CI issue. Someone did not remove identifier as a testing dependency for Patcherex. We will be able to merge this PR once our CI is fixed.

I appreciate your effort in making this PR!

zardus · 2017-06-22T06:02:43Z

@fish, that specific issue is ours, but the CI fails hardcore with this fix. Errors and failures across almost every repo. I'm re-trying the tests, but something seems broken for sure, @amttr :-(

ltfish · 2017-06-22T06:08:24Z

but the CI fails hardcore with this fix

Are you referring to our GitLab CI?

symflood · 2017-06-22T06:29:46Z

@zardus, unfortunately this PR is only the part of the solution, I'm working on angr fixes now, and as I saw angrop is also needed to be fixed.
I've provided this incomplete PR just to show the additional interface for CLE to manipulate (translate) addresses inside binaries. As for now any project, which is using .rebase_addr or .requested_base directly, will fail. Because the semantics is changed. I am sorry)

zardus · 2017-06-22T06:37:08Z

@ltfish, I was referring to github. Look at the travis logs.

@amttr, no worries about the failures. I was just making sure @ltfish was aware of them, so he wouldn't merge it before it's ready :-)

ltfish · 2017-06-22T06:37:52Z

(Unlike someone), I'm not going to merge in such a high-stake PR without thorough testing!

@amttr Take you time :-)

symflood · 2017-06-27T11:57:41Z

Ok, one more thing: there is an inconsistency between PE and ELF formats such that PEs mostly use RVAs internally, which is more convenient in case of rebasing, but ELFs use LVAs.
As an example - PE sections addresses (PE:Section Table (Section Headers):VirtualAddress) versus ELF segments addresses (ELF:Program Header:v_addr) and also entry points addresses.

Eventually, it can lead to situations when CLE user doesn't know what type of address he have got from CLE, for example, when user requested some information indirectly from pefile/pyelftools via CLE.
Is that correct?

P.S.: As I understand by design CLE should provide only flat virtual addresses which can be applied directly to the loader.memory without any modification, but the other side is that there are accesses to Region.min_addr/max_addr everywhere.

P.P.S.: Think CLE should be fixed in such way that angr has no need to fix the addresses, it can be done by Regions initialization with already fixed base addresses, but maybe that's not correct in general. Will wait for some comments.

rhelmot · 2017-06-27T18:54:44Z

I think that we should absolutely be providing a consistent API, preferably of fully rebased virtual addresses. My opinion is that it's completely ok to touch massive amounts of code in order to support this change. One API change that comes to mind is that several CLE objects have .addr and .rebased_addr, while it should probably be .local_addr (better name?) and .addr. Similarly, it would probably be the best for Region to have a backreference to the object that contains it, so that it can apply the base address to its API as well.

symflood · 2017-07-11T10:17:05Z

inb4: Currently I haven't found a simple way to make symbols <-> addresses mapping consistent with other API.

symflood · 2017-07-11T12:34:05Z

Tried to rebase, but there are many unrelated errors on testing, checked out with

./git_all.sh checkout wip/the_end_times

got:

ERROR: test_cfgaccurate.test_string_references
ERROR: test_fauxware.test_pickling('i386',)
ERROR: test_fauxware.test_pickling('x86_64',)
ERROR: test_fauxware.test_pickling('ppc',)
ERROR: test_fauxware.test_pickling('armel',)
ERROR: test_fauxware.test_pickling('mips',)
ERROR: test_identifier.test_comparison_identification
ERROR: test_vfg_path.test_vfg_paths
ERROR: test_oppologist.test_cromu_70
ERROR: test_unicorn.test_longinit_i386
ERROR: test_unicorn.test_longinit_x86_64
FAIL: test_inspect.test_inspect
FAIL: test_oppologist.test_fauxware_oppologist
FAIL: test_unicorn.test_stops
FAIL: test_unicorn.test_fauxware
FAIL: test_unicorn.test_fauxware_aggressive
FAIL: test_unicorn.test_unicorn_pickle
FAIL: test_unicorn.test_concrete_transmits
FAIL: test_veritesting.test_veritesting_b('x86_64',)
FAILED (errors=11, failures=8)

Is it ok to rebase there?

P.S.: I've fixed angrop in my fork, but it also based on master branch. Think it also should be rebased on wip/the_end_times. Not sure because tests are failing there.

rhelmot · 2017-07-11T19:06:27Z

the end times branch is in a state of extreme flux, so yeah, tests are supposed to be failing. You can compare your branch to the current state of the testcases and try to fix stuff to get it back to here.

rhelmot · 2017-07-11T19:07:16Z

Oh, it occured to me that your unicorn-related failures are probably because you haven't rebuilt angr. Just jump into angr and run python setup.py build.

symflood · 2017-07-12T15:17:43Z

I have pushed all the changes at:

angr/cle (master)
angr/angr (wip/the_end_times)
angr/angrop (wip/the_end_times)

Think its ready (for testing at least)

rhelmot

Pretty good!!!!! Mostly just style stuff that needs to be fixed, with the exception of the comment on rebase()

rhelmot · 2017-07-13T19:25:06Z

cle/backends/__init__.py

@@ -497,10 +528,14 @@ def set_arch(self, arch):
        self.memory = Clemory(arch) # Private virtual address space, without relocations

    @property
+    def image_base_delta(self):
+        return - self.linked_base + self.mapped_base


I had to blink a couple of times before I realized this wasn't a syntax error. Can you remove the space between the - and self.linked_base?

I can replace with just self.mapped_base - self.linked_base. I saw something like that in the codebase, just thought it's more clear.

rhelmot · 2017-07-13T19:25:47Z

cle/backends/__init__.py

+        Rebase backend's regions to the new base where they were mapped by the loader
+        """
+        if self.sections:
+            self.sections._rebase(self.image_base_delta)


Can we throw an error of some sort if we try to rebase things twice? Currently it'll just keep adding the delta to each of the sections, which is... weird

rhelmot · 2017-07-13T19:31:47Z

cle/backends/elf.py

+        self.mapped_base = self.linked_base = min(
+            map(lambda x: x['p_vaddr'],
+                filter(lambda x: x.header.p_type == 'PT_LOAD', self.reader.iter_segments())) or [0]
+        )


Python's functional style is only really useful when you're using generators to prevent the whole list from ever being created. Can you break this expression into several more readable statements, like

seg_addrs = [x['p_vaddr'] for x in self.reader.iter_segments() if x.header.p_type == 'PT_LOAD'] self.mapped_base = min(seg_addrs) if seg_addrs else 0

Also take into account sections as well, if segments are not present.

Also take into account sections as well, if segments are not present.

I think there is a naming inconsistency between PE and ELF -- entities which are sections in PE are segments in case of ELF. There are also so called directories described in PE's optional header which seems equivalent to ELF sections.

Maybe I am wrong, but loadable sections (in a case of ELF) should be always a part of loadable segments, so I don't understand how I can take sections into account and think the code is correct.
If I am not then it is possible for valid ELF, that loadable sections exist without a loadable segment. Is it so? Because this is not in a case of valid PE.

From specification:

To compute the base address, one determines the memory address associated with the lowest p_vaddr value for a PT_LOAD segment. One then obtains the base address by truncating the memory address to the nearest multiple of the maximum page size.
Depending on the kind of file being loaded into memory, the memory address might or might not match the p_vaddr values.

So resulting minimum should be only additionally aligned on a page size, which is actually the same what you're doing at ELF._load_segment_memory:

def _load_segment_memory(self, seg): # see https://code.woboq.org/userspace/glibc/elf/dl-load.c.html#1066 ...

Hm.. Looks like this is an additional issue: get_min_addr/get_max_addr rely on metadata which now is not consistent with Clemory state.

rhelmot · 2017-07-13T19:36:18Z

cle/backends/pe.py

-                rebased_bytes = struct.pack('<I', rebased_value % 2**32)
-                self.owner_obj.memory.write_bytes(self.dest_addr, rebased_bytes)
+                rebased_value = AT.from_lva(org_value, self.owner_obj).to_mva()
+                rebased_bytes = struct.pack('<I', rebased_value % 2 ** 32)


I think our current style is to not put spaces around the ** operator when it's just 2 to the power something. Makes it more readable imo but if you feel strongly about it it can stay it the codebase as a statement :)

That's ok. That was PyCharm, i'll fix that

rhelmot · 2017-07-13T19:40:02Z

cle/backends/pe.py

-        self.characteristics = pe_section.Characteristics
+    def __init__(self, name, offset, vaddr, size, chars):
+        super(PESection, self).__init__(name, offset, vaddr, size)
+        self.characteristics = chars


If you feel strongly about this change, could you please update ELFSection and ELFSegment to do the same thing? Right now they both just take readelf sections and parse them out into the constructors for the normal region arguments; this class was designed to mirror that. If there are complications with that for some reason, please revert this change. The interfaces should be the same.

Think interfaces should be updated, because of vaddr semantics for sections/segments, they are not the same for PE/ELF, but should be.

Btw, Mach-O's Section/Segment interfaces are already the same )

rhelmot · 2017-07-13T19:41:14Z

cle/loader.py

@@ -119,8 +121,7 @@ def close(self):
    def __repr__(self):
        if self._main_binary_stream is None:
            return '<Loaded %s, maps [%#x:%#x]>' % (os.path.basename(self._main_binary_path), self.min_addr(), self.max_addr())
-        else:
-            return '<Loaded from stream, maps [%#x:%#x]>' % (self.min_addr(), self.max_addr())
+        return '<Loaded from stream, maps [%#x:%#x]>' % (self.min_addr(), self.max_addr())


was this really necessary

I disabled this particular pylint check (no-else-return) as soon as I upgraded my pylint, so if pylint told you to do this, grab the new pylintrc from angr-dev!

symflood · 2017-07-14T20:00:46Z

I didn't complete interface fixing )

rhelmot · 2017-07-14T20:01:16Z

😬

symflood · 2017-07-14T20:03:59Z

I can push it with separate PR, i think

rhelmot · 2017-07-14T20:08:43Z

I just realized that we broke angr:master with this, so I pushed these changes to cle:wip/the_end_times and then force-pushed to revert to before the merge on master.

Can you give me a quick overview of what still needs to be done? It's possible I can knock it out faster.

symflood · 2017-07-14T20:18:15Z

Can you give me a quick overview of what still needs to be done?

Only interface fixing is left. Tests should work without this.

rhelmot · 2017-07-14T21:37:02Z

Just a heads up - I just merged a huge refactor to angr. I made sure to rebase it on top of your changes first, but if you've got any of these interface fixes in progress, you should rebase those on top of the current wip/the_end_times.

ltfish added bug enhancement labels Jun 19, 2017

symflood added 11 commits July 12, 2017 10:10

update: Owned addresses types conversion

0c0a182

fix: minor Mach-O binding issue (logging parameters order confusion)

4d18943

fix: rebasing and valid virtual image bases for Region/Regions

676a339

fix: switch from rebase_addr/requested_base to mapped_base/linked_base

9e7b91f

fix: add_object is responsible for rebasing

40abf96

fix: mapped_base initial state for ELF/PE/Mach-O

c7ebf31

fix: Address manipulations fixes

8b2c775

update: Relocated so-files mapping test

28bcf14

fix: Tests fixes. Mapped address cannot be equal to 0x0

c5b841d

fix: PESection interface consistency fix

fd864f0

fix: pylint

ae1649b

symflood added 2 commits July 13, 2017 19:04

fix: sorted key function misuse

1071dfa

update: Regression test suite for AddressTranslator

81c972a

rhelmot requested changes Jul 13, 2017

View reviewed changes

symflood added 5 commits July 14, 2017 14:23

fix: no-else-return fixes rollback

cd406d9

fix: ** operator surrounding fix

c233e23

fix: image_base_delta style fix

9a3f172

fix: Switch functional to readable. Linked image base alignment

d45e910

update: CLEOperationError on rebasing already mapped image

6b4b94d

rhelmot approved these changes Jul 14, 2017

View reviewed changes

rhelmot merged commit e3c565b into angr:master Jul 14, 2017

symflood mentioned this pull request Sep 30, 2019

Cannot load prelinked binaries #191

Open

Loader: ELF non-zero based PIC binaries support #67

Loader: ELF non-zero based PIC binaries support #67

Conversation

symflood commented Jun 19, 2017 • edited Loading

symflood commented Jun 19, 2017

rhelmot commented Jun 19, 2017

ltfish commented Jun 19, 2017

symflood commented Jun 19, 2017

ltfish commented Jun 20, 2017

zardus commented Jun 22, 2017

ltfish commented Jun 22, 2017

symflood commented Jun 22, 2017 • edited Loading

zardus commented Jun 22, 2017

ltfish commented Jun 22, 2017

symflood commented Jun 27, 2017 • edited Loading

rhelmot commented Jun 27, 2017

symflood commented Jul 11, 2017 • edited Loading

symflood commented Jul 11, 2017 • edited Loading

rhelmot commented Jul 11, 2017

rhelmot commented Jul 11, 2017

symflood commented Jul 12, 2017 • edited Loading

rhelmot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

symflood Jul 13, 2017 • edited Loading

Choose a reason for hiding this comment

symflood Jul 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

symflood commented Jul 14, 2017

rhelmot commented Jul 14, 2017

symflood commented Jul 14, 2017

rhelmot commented Jul 14, 2017

symflood commented Jul 14, 2017 • edited Loading

rhelmot commented Jul 14, 2017

symflood commented Jun 19, 2017 •

edited

Loading

symflood commented Jun 22, 2017 •

edited

Loading

symflood commented Jun 27, 2017 •

edited

Loading

symflood commented Jul 11, 2017 •

edited

Loading

symflood commented Jul 11, 2017 •

edited

Loading

symflood commented Jul 12, 2017 •

edited

Loading

symflood Jul 13, 2017 •

edited

Loading

symflood Jul 14, 2017 •

edited

Loading

symflood commented Jul 14, 2017 •

edited

Loading