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

Change dynamic scope to call for atomic operations #952

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jorik-utwente
Copy link

To reduce false positives, we change the dynamic scope of a couple of operations to call.
E.g., writing to the registry is always done in one Windows API call, so we change the scope from thread to call.

Original discussion: #951

Copy link

google-cla bot commented Oct 31, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@jorik-utwente jorik-utwente force-pushed the narrow-dyn-scope-atomic-operations branch from 1fdde58 to 4449de5 Compare October 31, 2024 09:07
- or:
- number: 0x40000000 = GENERIC_WRITE
- number: 0x2 = FILE_WRITE_DATA
- match: create or open file
Copy link
Collaborator

Choose a reason for hiding this comment

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

here's where things can get tricky: the scope of create or open file is (fortunately) also call, but in some other rules is a larger scope (like thread), which is impossible to match.

so, i think this change is fine.

but we should be aware that in other scenarios, there's some tricky things we might have to fix.

@jorik-utwente jorik-utwente force-pushed the narrow-dyn-scope-atomic-operations branch from 4449de5 to c9a586d Compare October 31, 2024 12:27
@jorik-utwente jorik-utwente force-pushed the narrow-dyn-scope-atomic-operations branch from c9a586d to 29dc617 Compare October 31, 2024 12:30
Copy link
Collaborator

@williballenthin williballenthin left a comment

Choose a reason for hiding this comment

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

thank you!

@@ -7,7 +7,7 @@ rule:
- [email protected]
scopes:
static: function
dynamic: thread
dynamic: call
Copy link
Collaborator

Choose a reason for hiding this comment

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

our rule parser doesn't like a call subscope in a file-wide call scope: call subscope supported only for the process and thread scopes - that's why the rule was as is, I assume

Copy link
Collaborator

Choose a reason for hiding this comment

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

this works here:

rule:
  meta:
    name: copy file
    namespace: host-interaction/file-system/copy
    authors:
      - [email protected]
      - [email protected]
    scopes:
      static: function
      dynamic: call
    mbc:
      - File System::Copy File [C0045]
    examples:
      - Practical Malware Analysis Lab 01-01.exe_:0x401440
  features:
    - or:
      - api: kernel32.CopyFile
      - api: kernel32.CopyFileEx
      - api: CopyFile2
      - api: CopyFileTransacted
      - api: LZCopy
      - api: System.IO.FileInfo::CopyTo
      - api: System.IO.File::Copy
      # static
      - basic block:
        - and:
          - number: 2 = FO_COPY
          - or:
            - api: kernel32.SHFileOperation
      # dynamic
      - and:
        - number: 2 = FO_COPY
        - or:
          - api: kernel32.SHFileOperation

Copy link
Author

Choose a reason for hiding this comment

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

Hmm okay. The problem with this fix is that for static analysis the dynamic part will also match, and doesn't have the basic block scope (as pointed out here: #952 (comment) ). There is no way to make part of the features only apply to dynamic analysis, is there?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, yes, that's not what we want.
I think we'd have to change the rule parser to properly handle this. @williballenthin, what do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This makes sense to me. +1

Copy link
Collaborator

Choose a reason for hiding this comment

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

@mr-tz
Copy link
Collaborator

mr-tz commented Nov 11, 2024

Thanks for all your suggestions, @jorik-utwente! I'm back this week to work on getting the improvements merged.

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.

3 participants