binarly-io / fwhunt-scan

Tools for analyzing UEFI firmware and checking UEFI modules with FwHunt rules
GNU General Public License v3.0
214 stars 30 forks source link

Help message says to use non-existing "scan" command #62

Closed kerneis-anssi closed 8 months ago

kerneis-anssi commented 8 months ago

Steps to reproduce: run fwhunt_scan_analyzer.py scan-firmware -d FwHunt/rules dump.bin

Result: [I] Specify volume_guids in IntelAlderLakeLeak or use scan command

Expected result: running with scan instead of scan-firmware works.

Actual result: no such scan command.

Work-around: use a combination of extract and analyze_module on every extracted module, but it's tedious to do manually.

Expected fix: either provide an actual scan command, fix scan-firmware to do the right thing, or update the warning to avoid mentioning a non-existing solution.

yeggor commented 8 months ago

Hi. We don't have scan command, but we do have scan-firmware and scan-module commands.

scan-module is intended to be used only on separate modules (no volume_guids required). If you want to scan the whole firmware, the rules you use must fulfil the following requirements:

I checked current state of fwhunt-scan with rules from binarly-io/FwHunt and don't see any problems with it.

image
kerneis-anssi commented 8 months ago

The error/information message I quoted is misleading. My suggestion based on your explanation would be to:

Currently, it's unclear based on the output whether the rule didn't run or if it ran but didn't trigger because the firmware is safe.

yeggor commented 8 months ago

oh, I get it. You're right, it should be fixed

yeggor commented 8 months ago

@kerneis-anssi it's updated now, thanks for highlighting this inconsistency I also updates one rule in FwHunt, so it will be applied only with scan-module command: https://github.com/binarly-io/FwHunt/commit/08779032da70eda856504873bd9bd86ffdc9ebc8.

yeggor commented 8 months ago

fwhunt_scan_analyzer output after update:

image
kerneis-anssi commented 8 months ago

Thanks for the update, https://github.com/binarly-io/fwhunt-scan/commit/7ae817287bc60c27fd7817eda3262b53eee2ef0f is definitely the right fix. I don't understand https://github.com/binarly-io/FwHunt/commit/08779032da70eda856504873bd9bd86ffdc9ebc8 though: it looks like the warning message is a good thing in the case of ESPecter, since it won't detect anything unless scan-module is used. Why specify a dummy volume guid to hide it?

yeggor commented 8 months ago

this is not to hide, but to make it more convenient to use fwhunt-scan via the API. The point is that when volume guids are not specified, it means that the rule should be applied to all modules. So we are using dummy guids for 2 rules:

Assuming that these rules will be applied in a targeted manner, rather than on every module.

kerneis-anssi commented 8 months ago

I'm sorry but I still don't get it: fwhunt_scan_analyzer.py doesn't run rules that have no volume guids, does it? With the dummy guids, the information message is never triggered to suggest targetted analysis.

yeggor commented 8 months ago

With the dummy guids, the information message is never triggered to suggest targetted analysis.

It will if someone uses custom rules and there is a mismatch.

Again, if volume guids are not specified in the rule, it is assumed that the rule must be applied to each firmware module. In case of BlackLotusBootkit and ESPecter it's not the case (since the rules apply to a boot loader that is not part of the UEFI firmware). This is the reason why I specified dummy GUIDs for these rules (which are more of an exception to the normal fwhunt usage scenario).

kerneis-anssi commented 8 months ago

First, I want to thank you for taking the time to engage and trying to clarify the situation. I think I've identified the root of our misunderstanding.

if volume guids are not specified in the rule, it is assumed that the rule must be applied to each firmware module

This is the part that I don't understand:

I see a few options here:

  1. keep existing behaviour of fwhunt-scan
    • update the specification to clarify that if no volume_guid is specified and target is not "firmware", then the rule should be run manually against specific modules
    • update rules to remove the dummy guids (which will restore the warning messages)
    • (optional) introduce a new type of target: esp instead for those rules, if you prefer warning saying that the rule is incompatible with scan-firmware instead of a warning that the rule must be run on targetted modules (https://github.com/binarly-io/fwhunt-scan/blob/master/fwhunt_scan_analyzer.py#L149)
  2. change behaviour of fwhunt-scan
    • update the specification to clarify that, if no volume guid is specified, the rule is attempted on every module
    • update https://github.com/binarly-io/fwhunt-scan/blob/master/fwhunt_scan_analyzer.py#L154 to match this behaviour
    • introduce a new type of target: esp instead of the dummy guids, to get the warning saying that those rules are incompatible with scan-firmware (and maybe update the warning to say that you can use scan-module instead)

There may be other options that I'm missing. There may also be some other reasons why you need those rules to have the dummy volume_guid (maybe some code that is not published on githtub but shares the rules), but it's not obvious why looking only at your public code.

yeggor commented 8 months ago

Thank you very much for your input @kerneis-anssi. We have merged 2 PRs that we think solve the problem:

In the first one, we introduced documentation for target: bootloader and introduced descriptions for the target and volume guids fields. In the second one, we have implemented the appropriate changes in fwhunt-scan.

I hope this resolves any inconsistencies. Let me know what you think about it.

kerneis-anssi commented 8 months ago

The documentation is much clearer now, and the target: bootloader a welcome addition. I only looked at the code (didn't test it), but I think the current code doesn't match your specification: if a rule has no volume_guid, fwhunt-scan scan-firmware is going to skip it instead of testing it against all submodules.

yeggor commented 8 months ago

It's true. The reason for this is that the public fwhunt-scan implementation is too slow to be used at scale with rules without specified volume guids.

So this is an attempt to warn the user against using such rules.

However, I think we can allow such scans to be used with an additional parameter in the scan-firmware.

kerneis-anssi commented 8 months ago

However, I think we can allow such scans to be used with an additional parameter in the scan-firmware.

Sounds like a good idea.

We've wandered quite far from my original report so you can close this issue if you wish as far as I'm concerned. Thanks again for your tool.

yeggor commented 8 months ago

@kerneis-anssi thanks again for your input. I've added the --force parameter to the scan-firmware command, which allows rules without volume guids to be applied to all extracted modules.

I'm closing the issue