0vercl0k / wtf

wtf is a distributed, code-coverage guided, customizable, cross-platform snapshot-based fuzzer designed for attacking user and / or kernel-mode targets running on Microsoft Windows and Linux user-mode (experimental!).
MIT License
1.47k stars 132 forks source link

Explicitly pass the size of the requested arguments #188

Closed 1ndahous3 closed 11 months ago

1ndahous3 commented 1 year ago

Due to refactoring in #155 the argument size bug was introduced.

Initially the code was like this:

  // ntdll!NtDeviceIoControlFile()
  // Ioctl:              @rsp + 0x30
  // InputBuffer:        @rsp + 0x38
  // InputBufferLength:  @rsp + 0x40

  const Gva_t Stack = Gva_t(g_Backend->Rsp());

  const Gva_t InputBufferLengthPtr = Stack + Gva_t(0x40);
  uint32_t CurrentIoctlBufferSize = g_Backend->VirtRead4(InputBufferLengthPtr);

But after refactoring it became like this: https://github.com/0vercl0k/wtf/blob/9823579ef764b0b3c0af2f71b61d5aa47fb3de51/src/wtf/fuzzer_ioctl.cc#L77-L89

So 8 bytes are read for the InputBufferSize argument, and we got an invalid value - only one uint32_t(InputBufferSize) cast is made, but then the same cast is not made during addition operation.

This patch replaces one argument getter function with two: GetArg4() for 4-byte values and GetArg8() for 8-byte values. Also, many additional casts to uint32_t have been removed, which are no longer needed.

0vercl0k commented 1 year ago

Oops! Thanks for the PR - I am currently traveling so will take a look at this in December; I hope it's ok!

Cheers

On Sat, Nov 4, 2023 at 3:46 AM Roman @.***> wrote:

Due to refactoring in #155 https://github.com/0vercl0k/wtf/pull/155 the argument size bug was introduced.

Initially the code was like this:

https://github.com/1ndahous3/wtf/blob/a874ebaf71da4f94df1dba85fb3ca18441e96f42/src/wtf/fuzzer_ioctl.cc#L48-L56

But after refactoring it became like this:

https://github.com/0vercl0k/wtf/blob/9823579ef764b0b3c0af2f71b61d5aa47fb3de51/src/wtf/fuzzer_ioctl.cc#L77-L89

So 8 bytes are read for the InputBufferSize argument, and we got an invalid value - only one uint32_t(InputBufferSize) cast is made, but then the same cast is not made during addition operation.

This patch replaces one argument getter function with two: GetArg4() for 4-byte values and GetArg8() for 8-byte values. Also, many additional casts to uint32_t have been removed, which are no longer needed.

You can view, comment on, or merge this pull request online at:

https://github.com/0vercl0k/wtf/pull/188 Commit Summary

File Changes

(5 files https://github.com/0vercl0k/wtf/pull/188/files)

Patch Links:

— Reply to this email directly, view it on GitHub https://github.com/0vercl0k/wtf/pull/188, or unsubscribe https://github.com/notifications/unsubscribe-auth/AALIORP4VZ2PMYNL5NFNDBLYCWUCFAVCNFSM6AAAAAA65IBC3GVHI2DSMVQWIX3LMV43ASLTON2WKOZRHE3TOMJTGA3DGMA . You are receiving this because you are subscribed to this thread.Message ID: @.***>

0vercl0k commented 11 months ago

Looking into this..

0vercl0k commented 11 months ago

@1ndahous3 what do you think of the new changes?

Cheers

1ndahous3 commented 11 months ago

@0vercl0k looks good, I think it makes no difference for GetArg4() to read 4 bytes or read 8 bytes with garbage and trim it through a uint32_t cast.

In my opinion, it would be nice to rename GetArg() to GetArg8() to show the size not only by the type of the return value. This will not give a chance to make a bug when writing code without noticing the GetArg4() pair function.

0vercl0k commented 11 months ago

I do agree, but it also means it breaks everybody that is using GetArg which is not desirable. What I can do is to introduce a GetArg8 version though to be consistent - wdyt?

Cheers

1ndahous3 commented 11 months ago

What I can do is to introduce a GetArg8 version though to be consistent - wdyt?

Sounds like a good compromise.

0vercl0k commented 11 months ago

All right - take this for a spin and let me know 🫡

Cheers

1ndahous3 commented 11 months ago

AFAIR the BugCheckCode from KeBugCheck2() is of ULONG type, so we need to get the uint32_t value.

0vercl0k commented 11 months ago

Ha yeah I forgot to update it - will do that tonight!

Cheers

On Tue, Nov 28, 2023 at 7:03 AM Roman @.***> wrote:

AFAIR the BugCheckCode from KeBugCheck2() is of ULONG type, so we need to get the uint32_t value.

— Reply to this email directly, view it on GitHub https://github.com/0vercl0k/wtf/pull/188#issuecomment-1829174614, or unsubscribe https://github.com/notifications/unsubscribe-auth/AALIORLO6RYMMOPFJDTN44LYGV5BZAVCNFSM6AAAAAA65IBC3GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMRZGE3TINRRGQ . You are receiving this because you were mentioned.Message ID: @.***>

0vercl0k commented 11 months ago

Done!

1ndahous3 commented 11 months ago

I can only suggest adding a [[deprecated]] attribute to the GetArg() function.

The code is ok.

0vercl0k commented 11 months ago

Neat, TIL [[deprecated]]!

1ndahous3 commented 11 months ago

I don't have any additional suggestions or notes, everything looks great!

0vercl0k commented 11 months ago

All right, will merge this today - thank you!

Cheers

On Fri, Dec 1, 2023 at 11:01 PM Roman @.***> wrote:

I don't have any additional suggestions or notes, everything looks great!

— Reply to this email directly, view it on GitHub https://github.com/0vercl0k/wtf/pull/188#issuecomment-1836836044, or unsubscribe https://github.com/notifications/unsubscribe-auth/AALIORL4SOCEFD2MI43DEE3YHJHSVAVCNFSM6AAAAAA65IBC3GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMZWHAZTMMBUGQ . You are receiving this because you were mentioned.Message ID: @.***>