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

PLUGX: make more restrictive to fix FP #843

Merged
merged 3 commits into from
Nov 15, 2023
Merged

Conversation

williballenthin
Copy link
Collaborator

closes #842

@williballenthin
Copy link
Collaborator Author

@Still34 FYI

@williballenthin williballenthin added bug Something isn't working false positive False positive rule hit labels Nov 13, 2023
@williballenthin
Copy link
Collaborator Author

looks like i need to review the linter failure before this is merged. please review the intent of the logic change in the meantime.

@Still34
Copy link
Contributor

Still34 commented Nov 13, 2023

Shoot, I'll take a look in a bit

Copy link
Collaborator

@mr-tz mr-tz left a comment

Choose a reason for hiding this comment

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

logic LGTM

@Still34
Copy link
Contributor

Still34 commented Nov 13, 2023

Looks perfectly fine to me as well

@williballenthin
Copy link
Collaborator Author

williballenthin commented Nov 14, 2023

when I proposed the changes here I imagined (without looking, sorry) that there'd be a large command ID dispatcher that had a bunch of the command IDs referenced in the same function, using a case/switch statement. turns out this is not the case for plugx. instead, we have individual functions that handle each command, parsing or creating the protocol, like:

image

note that we see:

mov [base+0], TIMESTAMP
mov [base+4], COMMAND

also: along the way i did find a FP in the same sample: see how 0x4000 is used as a bitmask in a related function, not as the command ID:

image

so, i'll propose updating the rule to:

- and:
  - instruction:
    - mnemonic: mov
    - operand[0].offset: 0
    - or:
      - operand[1].number: TIMESTAMP1
      - operand[1].number: TIMESTAMP2
  - instruction:
    - mnemonic: mov
    - operand[0].offset: 4
    - or:
      - operand[1].number: COMMAND1
      - operand[1].number: COMMAND2

i think its appropriate to set the scope to basic block, too.

@williballenthin
Copy link
Collaborator Author

image

@williballenthin
Copy link
Collaborator Author

@Still34 @mr-tz would you take another peek, please?

@Still34
Copy link
Contributor

Still34 commented Nov 14, 2023

Throw this timestamp in there as well 0x20140613, also I could not get the match to pass with sample edfff708808ef3a5453dc7713d306b20 @ 0x1000A258 (and yes the timestamp is added to the local rule).

@mr-tz
Copy link
Collaborator

mr-tz commented Nov 15, 2023

with the added value it matches for me:
image

@williballenthin
Copy link
Collaborator Author

@Still34

also I could not get the match to pass

would you double check using -vv?

it looks like today we're filtering out malware family rules from the default display. im not quite sure why we're doing that. do you remember @mr-tz?

@williballenthin williballenthin merged commit 1b8b2db into master Nov 15, 2023
3 checks passed
@williballenthin williballenthin deleted the fix/issue-842 branch November 15, 2023 09:24
@mr-tz
Copy link
Collaborator

mr-tz commented Nov 15, 2023

I'm not sure why. Maybe we wanted to render it separately...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working false positive False positive rule hit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PLUGX rule is too loose
3 participants