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 "ioctl" target #155

Merged
merged 9 commits into from
May 9, 2023
Merged

add "ioctl" target #155

merged 9 commits into from
May 9, 2023

Conversation

1ndahous3
Copy link
Contributor

Universal code for fuzzing any driver from the state captured on ntdll!NtDeviceIoControlFile().

There are 2 modes (switchable at compile time via MutateIoctl flag):

  1. Mutate only the buffer - IOCTL from the state does not change.
  2. Mutate buffer and IOCTL - the IOCTL is captured from the first 4 bytes of the input buffer).

@0vercl0k
Copy link
Owner

0vercl0k commented Mar 7, 2023 via email

@0vercl0k
Copy link
Owner

@1ndahous3 I took the liberty to push some changes on your branch; basically I reworked the code to be a generic ioctl fuzzer as you wanted but without forcing the user to take their breakpoint at ntdll!NtDeviceIoControlFile. Now they can grab the crash-dump anywhere they'd like and the testcase is inserted when the NtDeviceIoControlFile is hit.

I also switched to nt!NtDeviceIoControlFile instead of ntdll!NtDeviceIoControlFile to be able to work fine w/ WoW64 targets.

Give it a shot and let me know what you think. I will probably change a little bit the GetArg / GetArgGva change because I don't like it but the rest should stay mostly the same!

Cheers

@1ndahous3
Copy link
Contributor Author

@0vercl0k very interesting rework, awesome.

I also switched to nt!NtDeviceIoControlFile instead of ntdll!NtDeviceIoControlFile to be able to work fine w/ WoW64 targets.

Definitely a good change!

I reworked the code to be a generic ioctl fuzzer as you wanted but without forcing the user to take their breakpoint at ntdll!NtDeviceIoControlFile

Well, I understand the new idea and algorithm, but I don't fully understand the flow: what additional states can now be dumped?

My reasoning is that if we allow a random state to be dumped, the following things can happen:

  1. cr3 in case of usermode code far from NtDeviceIoControlFile (this is true for any fuzzing target).
  2. The next time NtDeviceIoControlFile is called, the handle might be for a different device than the desired driver (even on the same thread). In practice, this can be a random call from a random dll from any call inside WinAPI).

The second scenario is more dangerous: visually, when passing a corpus for a specific driver, no errors will occur for this state, but in practice, our buffers will not be passed to the ioctl handler of the desired driver.

My opinion: if you mean that the "more general" variant allows to dump the state quite close prior to calling NtDeviceIoControlFile with the desired handle, and if we know for sure that the buffer size will be sufficient for our corpus, I think these tips should be described in the comments. Otherwise, the code can be simplified to the original version, because having hit the right call from the user mode, it is easy to hit the next NtDeviceIoControlFile call and be sure of all the arguments in the captured dump.

@1ndahous3
Copy link
Contributor Author

Why are you adding src/wtf/fuzzer_* to .gitignore? I could be wrong, but it's more convenient to see changes in targets and be able to push than to ignore these sources.

@0vercl0k
Copy link
Owner

Okay yes you are right that it might not be ideal to mutate EVERY call to NtDeviceIoControlFile; and yeah this also messes up the return breakpoint potentially.

What do you think if we add a parameter that specifies the handle? If the handle doesn't match an hardcoded value, we don't do anything with the call; if it does, we mutate? Do you think that'd work?

I am adding fuzzer_* to the .gitignore because it prevents mistake when you are working on private fuzzers that you don't want to be tracked by git. Bypassing this is easy, you can just do git add fuzzer_foo.c the first time you create the file and the changes will be tracked on this file going forward.

Cheers

@1ndahous3
Copy link
Contributor Author

What do you think if we add a parameter that specifies the handle? If the handle doesn't match an hardcoded value, we don't do anything with the call; if it does, we mutate? Do you think that'd work?

I don't think this is a good idea, because with such dumps (when calls to NtDeviceIoControlFile with other handles occur), fuzzing performance will be greatly reduced, and there is also a chance to catch a cr3 switch in the usermode before the desired NtDeviceIoControlFile. Also, to determine the right handle (to set it in the code or pass as a parameter to the target), you still need to step on the call and validate (!handle, !fileobj in WinDbg). In this case, it is more reliable to create an exactly correct dump, where the handle will already be written.

@0vercl0k
Copy link
Owner

OK sounds fair - I'll revert that code. In that case, isn't the this fuzzer nearly equivalent to fuzzer_hevd.cc? Maybe it makes more sense to add the ioctl code mutation to fuzzer_hevd.cc and rename it to fuzzer_ioctl.cc. What do you think?

Cheers

@1ndahous3
Copy link
Contributor Author

1ndahous3 commented Mar 16, 2023

@0vercl0k I agree that the "hevd" target is a special case of "ioctl" in terms of fuzzing. But HEVD is a training kit (driver, usermode application, target), i.e. with a controlled IOCTL and buffer in usermode function that is easy to catch in a dump. A kernel32!DeviceIoControl dump is expected there, not nt!NtDeviceIoControlFile (which you rightly suggested and implemented).

So maybe it's better to just leave the "hevd" target unchanged as part of a special simplified training kit?

This does not exclude the possibility of using new "ioctl" target for fuzzing HEVD.

@0vercl0k
Copy link
Owner

Okay sounds good - I reverted the changes. I need to generate a dump to try it out (hopefully this weekend); let me know if you see anything wrong!

Cheers

@0vercl0k
Copy link
Owner

0vercl0k commented May 9, 2023

Sorry for procrastinating on this - I finally grabbed a dump to be able to test this and was able to fix a few bugs. I will rework this GetArgGva(x, &ptr) thing I added tomorrow though, I'm not happy with the way it looks / used.

Anyways, hopefully this will land tomorrow or the day after :)

Cheers

@0vercl0k 0vercl0k merged commit 62031ef into 0vercl0k:main May 9, 2023
@1ndahous3 1ndahous3 deleted the ioctl_target branch September 17, 2023 20:29
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