Skip to content

Relocate import symbol addresses for loaded PE files#886

Closed
crass wants to merge 1 commit into
qilingframework:old_devfrom
crass:relocate-imported-symbols-in-loaded-library
Closed

Relocate import symbol addresses for loaded PE files#886
crass wants to merge 1 commit into
qilingframework:old_devfrom
crass:relocate-imported-symbols-in-loaded-library

Conversation

@crass
Copy link
Copy Markdown
Contributor

@crass crass commented Aug 16, 2021

Currently only the imported symbols of the main PE file are relocated. This
mostly works because the common Windows dlls (eg. kernel32.dll and
user32.dll) normally have imported symbols which are already bound, and
thus do not need relocating. However, if a library loaded at runtime (believe at
loadtime also) has unbound import symbols, they will need to be relocated.

I have run the tests and they all succeed, but I have not added any new ones.
The binary that I'm analyzing that triggers this is 38Mb (kinda large), but it could
probably be truncated to 5Mb and still be effective as a test.

This is partly a refactor in that a code chunk is moved into qiling.loader.pe.Process.init_imports.
The moved code maintains mostly the same behavior with some notable differences.

  1. Bound symbols are ignored
  2. The library where imported symbols originate from is only loaded via load_dll once.
  3. If there is a KeyError when adding the symbol to import_address_table the loop is continued,
    which was a minor bug in the current code

Checklist

Which kind of PR do you create?

  • This PR only contains minor fixes.
  • This PR contains major feature update.
  • This PR introduces a new function/api for Qiling Framework.

Coding convention?

  • The new code conforms to Qiling Framework naming convention.
  • The imports are arranged properly.
  • Essential comments are added.
  • The reference of the new code is pointed out.

Extra tests?

  • No extra tests are needed for this PR.
  • I have added enough tests for this PR.
  • Tests may be added after some discussion and review.

Changelog?

  • This PR doesn't need to update Changelog.
  • Changelog will be updated after some proper review.
  • Changelog has been updated in my PR.

Target branch?

  • The target branch is dev branch.

One last thing


@xwings
Copy link
Copy Markdown
Member

xwings commented Aug 16, 2021

Cool, once we finish the test I will merge this PR.

again, welcome to the community!

@xwings
Copy link
Copy Markdown
Member

xwings commented Aug 16, 2021

seems like there is a missing dll

you might want to edit this in your PR and make sure it copy the dll

https://github.com/qilingframework/qiling/blob/dev/examples/scripts/dllscollector.bat

@crass crass force-pushed the relocate-imported-symbols-in-loaded-library branch from 71e6272 to 46e3402 Compare August 16, 2021 07:06
@crass
Copy link
Copy Markdown
Contributor Author

crass commented Aug 16, 2021

Thanks for the warm welcome! I don't really have a good way of testing on windows right now, but I think I've added correctly the missing dlls. Its interesting, I'm using system dlls from Win7, and the system dlls import symbols from that missing lib, but they are all bound. So that missing lib never actually gets loaded. I guess on the newer version of windows that the tests are run on, those system dlls have some unbound import symbols.

@xwings
Copy link
Copy Markdown
Member

xwings commented Aug 16, 2021

We will find out in the test. CI test acts funny sometimes. So, only CI knows what CI wansts.

@xwings
Copy link
Copy Markdown
Member

xwings commented Aug 16, 2021

@crass crass force-pushed the relocate-imported-symbols-in-loaded-library branch from 46e3402 to 1df8774 Compare August 16, 2021 08:16
@crass
Copy link
Copy Markdown
Contributor Author

crass commented Aug 16, 2021

Looks like I'm going to have to keep playing this game until it stops complaining. Here's another round

@crass
Copy link
Copy Markdown
Contributor Author

crass commented Aug 17, 2021

ping. Can I get the workflow run again, until we can get all the needed libraries added?

@xwings
Copy link
Copy Markdown
Member

xwings commented Aug 18, 2021

every code will be tested automatically

@crass crass force-pushed the relocate-imported-symbols-in-loaded-library branch from 1df8774 to 15aa189 Compare August 18, 2021 17:24
@crass
Copy link
Copy Markdown
Contributor Author

crass commented Aug 18, 2021

@xwings The workflow does not run automatically after I do a push. It always tells me

1 workflow awaiting approval
First-time contributors need a maintainer to approve running workflows

I forgot I needed to take into account x86 vs x8664 rootfs, so prior changes may have been wrong. This may mean a couple extra cycles. Please approve the workflow again.

@crass
Copy link
Copy Markdown
Contributor Author

crass commented Aug 18, 2021

Its also annoying that I can't got back and look at prior runs of the workflow. Once the PR branch is updated the workflow CI output is removed.

@xwings
Copy link
Copy Markdown
Member

xwings commented Aug 21, 2021

lets checkout the test :)

@crass crass force-pushed the relocate-imported-symbols-in-loaded-library branch 2 times, most recently from 4a26a73 to 33872bc Compare August 21, 2021 08:57
@xwings
Copy link
Copy Markdown
Member

xwings commented Aug 21, 2021

still fail :(

Before only the imported symbols of the main PE file were relocated. This
mostly worked because the common Windows dlls (eg. kernel32.dll and
user32.dll) normally have imported symbols which are already bound, and
thus do not need relocating.
@crass crass force-pushed the relocate-imported-symbols-in-loaded-library branch from 33872bc to ee69754 Compare August 21, 2021 09:04
@crass
Copy link
Copy Markdown
Contributor Author

crass commented Aug 21, 2021

It looks like one snag, is that there is no %WINDIR%\System32\downlevel\api-ms-win-core-apiquery-l1-1-0.dll, which seems to be required by X8664 windows binaries. It turns out all this extra library non-sense is due to a feature since Windows 7, called API sets (and more on reimplementing them here).

I need to read up it more, the loader might need to be updated to support this. I don't know why some of the API sets exists and some don't.

@wtdcode wtdcode deleted the branch qilingframework:old_dev October 12, 2021 08:49
@wtdcode wtdcode closed this Oct 12, 2021
@wtdcode wtdcode reopened this Oct 14, 2021
@elicn
Copy link
Copy Markdown
Member

elicn commented Apr 24, 2022

The suggested changes were included as part of #1118 .
Thanks @crass !

@elicn elicn closed this Apr 24, 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.

4 participants