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 Windows Kernel structs #1219

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from

Conversation

HackingFrogWithSunglasses
Copy link
Contributor

As per #1217 this is the WIndows kernel structures PR

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 will 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


Copy link
Member

@elicn elicn left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution.
As mentioned on the previous PR you opened, the additional structures are not defined in a correct way. Please see BaseStruct documentation and how other Windows structures are defined to get the idea.

@@ -7,7 +7,7 @@
from typing import Any, Dict, MutableMapping, NamedTuple, Optional, Mapping, Sequence, Tuple, Union

from unicorn import UcError
from unicorn.x86_const import UC_X86_REG_CR4, UC_X86_REG_CR8
from unicorn.x86_const import UC_X86_REG_CR4, UC_X86_REG_CR8, UC_X86_REG_GS
Copy link
Member

Choose a reason for hiding this comment

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

UC_X86_REG_GS is imported but never used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this should be used to write the kernel structures pointers to the GS register with their correct offset but I didn't manage to get it working so I wrote to the value manually.

kpcr_obj.Prcb = kprcb_addr # Writes _KPRCB pointer to Prcb field

# Writes KPCR pointer into GS:[0x18]
self.ql.mem.write_ptr(0x6000018, kpcr_addr)
Copy link
Member

Choose a reason for hiding this comment

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

Please refrain from using magic numbers; there is a constant for GS that can be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not a magic number. That is the exact offset for the KPRCB pointer in Windows KM. I agree that the constant + offset should be used, though.

Copy link
Member

Choose a reason for hiding this comment

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

Great, that's what I meant.

self.ql.mem.map(kthread_addr, self.ql.mem.align_up(kthread_struct.sizeof()), info='[kthread]')

# Initialize an instance with key fields
kthread_obj = kthread_struct.volatile_ref(self.ql.mem, kthread_addr)
Copy link
Member

Choose a reason for hiding this comment

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

This structure is initialized with magic numbers whose origin is unclear.
Is there a place [a Qilign object] we can take the values from? Consistency is key.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure which numbers you're referring to are magic here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh if you mean the 0x188 for GS, then same as above. Should be changed to constant + offset

Copy link
Member

Choose a reason for hiding this comment

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

I actually referred to the kthread_obj fields values: 11, 0x1000, 0x1500, 0x2000?

kthread_obj.InitialStack = 0x1000
kthread_obj.StackBase = 0x1500
kthread_obj.StackLimit = 0x2000
kthread_obj.MiscFlags |= 0x400
Copy link
Member

Choose a reason for hiding this comment

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

The value of MiscFlags is OR-ed with the existing one, but what is it...? That field was never initialized.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

MiscFlags can be removed in this instance, I was using this for my own purpose and it won't be useful for Qiling generically.

# Writes KPRCB pointer into GS:[0x20]
self.ql.mem.write_ptr(0x6000020, kprcb_addr)

# @NOTE: Tests for writing structure pointers.
Copy link
Member

Choose a reason for hiding this comment

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

What this is about? Why is it so important it shouldn't be removed?
Also, Qiling debugging info should be logged as debug rather than warn.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can remove these debug statements, I just had those there for my local branch and forgot to remove them when I submitted PR.

('NestingLevel', ctypes.c_char),
('ClockOwner', ctypes.c_char),
('PendingTickFlags', ctypes.c_char),
('PendingTick', ctypes.c_void_p), # POS 0 : BIT 1
Copy link
Member

Choose a reason for hiding this comment

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

This is not how bit fields are defined.
To keep it simple, just keep the encapsulating field and do not define bit fields.

pointer_type = native_type

_fields_ = (
('MxCsr', ctypes.c_ulong),
Copy link
Member

Choose a reason for hiding this comment

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

From the same reason mentioned above (avoiding structures definitions getting affected by the hosting arch), we should use only known size types, like c_uint32 and c_uint16 instead of long and short.

@@ -1,13 +1,17 @@
[OS64]
heap_address = 0x500000000
heap_address = 0xfffff76000000000
Copy link
Member

Choose a reason for hiding this comment

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

Is there any specific reason for this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No specific reason relevant to Qiling. Was for my own testing purposes.

stack_size = 0x40000
image_address = 0x400000
dll_address = 0x7ffff0000000
entry_point = 0x140000000
# KI_USER_SHARED_DATA = 0xfffff78000000000
KI_USER_SHARED_DATA = 0x7ffe0000
KI_USER_SHARED_DATA = 0xfffff78000000000
Copy link
Member

Choose a reason for hiding this comment

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

Is there any specific reason you removed the existing value and took the commented one?
Qiling doesn't have a concept of virtual and physical addresses, and this is the main reason why the value was changed in the first place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No specific reason, again for my own testing.

qiling/profiles/windows.ql Outdated Show resolved Hide resolved
Copy link
Contributor Author

@HackingFrogWithSunglasses HackingFrogWithSunglasses left a comment

Choose a reason for hiding this comment

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

I agree with the requested changes.

@@ -3,10 +3,12 @@
# Cross Platform and Multi Architecture Advanced Binary Emulation Framework
#

from calendar import c
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, can be removed.

Apply fixes per discussion on PR.
Apply fixes per discussion on PR.
Copy link
Contributor Author

@HackingFrogWithSunglasses HackingFrogWithSunglasses left a comment

Choose a reason for hiding this comment

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

👍

import ctypes

from enum import IntEnum
from functools import lru_cache
from pickletools import uint1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, can be removed.

@@ -115,6 +117,35 @@ class PEB(Struct):

return PEB

class NT_TIB(struct.BaseStruct):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The output is not needed but please be aware that with certain kernel structures you cannot just have a "link" to them because they are undocumented. In this case the output can be deleted though.

@@ -7,7 +7,7 @@
from typing import Any, Dict, MutableMapping, NamedTuple, Optional, Mapping, Sequence, Tuple, Union

from unicorn import UcError
from unicorn.x86_const import UC_X86_REG_CR4, UC_X86_REG_CR8
from unicorn.x86_const import UC_X86_REG_CR4, UC_X86_REG_CR8, UC_X86_REG_GS
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this should be used to write the kernel structures pointers to the GS register with their correct offset but I didn't manage to get it working so I wrote to the value manually.

self.ql.mem.map(kthread_addr, self.ql.mem.align_up(kthread_struct.sizeof()), info='[kthread]')

# Initialize an instance with key fields
kthread_obj = kthread_struct.volatile_ref(self.ql.mem, kthread_addr)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure which numbers you're referring to are magic here.

self.ql.mem.map(kthread_addr, self.ql.mem.align_up(kthread_struct.sizeof()), info='[kthread]')

# Initialize an instance with key fields
kthread_obj = kthread_struct.volatile_ref(self.ql.mem, kthread_addr)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh if you mean the 0x188 for GS, then same as above. Should be changed to constant + offset

Apply fixes per discussion on PR.
Apply fixes per discussion on PR.
Copy link
Member

@elicn elicn left a comment

Choose a reason for hiding this comment

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

I took the liberty to edit some of the files and apply the fixes myself.
Also added a few responses.

self.ql.mem.map(kthread_addr, self.ql.mem.align_up(kthread_struct.sizeof()), info='[kthread]')

# Initialize an instance with key fields
kthread_obj = kthread_struct.volatile_ref(self.ql.mem, kthread_addr)
Copy link
Member

Choose a reason for hiding this comment

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

I actually referred to the kthread_obj fields values: 11, 0x1000, 0x1500, 0x2000?

@@ -115,6 +117,35 @@ class PEB(Struct):

return PEB

class NT_TIB(struct.BaseStruct):
Copy link
Member

Choose a reason for hiding this comment

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

Yes, I am aware.. if there is an unformal documentation you can link to, that's perfectly fine.

@HackingFrogWithSunglasses
Copy link
Contributor Author

That's absolutely fine as I don't have time myself to maintain this. Thanks!

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.

2 participants