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

Verify FTL binary integrity #2072

Merged
merged 6 commits into from
Oct 1, 2024
Merged

Verify FTL binary integrity #2072

merged 6 commits into from
Oct 1, 2024

Conversation

DL6ER
Copy link
Member

@DL6ER DL6ER commented Sep 26, 2024

What does this implement/fix?

This PR adds automatic binary verification to pihole-FTL. Technically, this works by modifying CMake to append the ahs265sum checksum after the end of the binary. During startup (or when manually calling pihole-FTL verify), FTL computes its own checksum (skipping the final 256 bit!) and compares it to the checksum stored in said last 256 bit.

On error, we also log a message to the diagnosis system:
image

Unfortunately, the automatic test has to be run at a rather late time in binary startup (when the database is already initialized), or we could not add the message. However, this is hopefully early enough.

Otherwise, the manual pihole-FTL verify command allows for easy testing without having to manually downloading the (possibly already outdated/already overwritten) branch-wise sha files from the FTL webserver:

image


Related issue or feature (if applicable): N/A

Pull request in docs with documentation (if applicable): N/A


By submitting this pull request, I confirm the following:

  1. I have read and understood the contributors guide, as well as this entire template. I understand which branch to base my commits and Pull Requests against.
  2. I have commented my proposed changes within the code.
  3. I am willing to help maintain this change if there are issues with it later.
  4. It is compatible with the EUPL 1.2 license
  5. I have squashed any insignificant commits. (git rebase)

Checklist:

  • The code change is tested and works locally.
  • I based my code and PRs against the repositories developmental branch.
  • I signed off all commits. Pi-hole enforces the DCO for all contributions
  • I signed all my commits. Pi-hole requires signatures to verify authorship
  • I have read the above and my PR is ready for review.

src/args.c Fixed Show fixed Hide fixed
Copy link
Member

@yubiuser yubiuser left a comment

Choose a reason for hiding this comment

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

Shouldn't this be better part of the build.sh instead?

@DL6ER
Copy link
Member Author

DL6ER commented Sep 26, 2024

Which part do you mean? This test is mostly intended towards corruption caused by SD card issues and similar happenings. The build script is never run on user systems.

@yubiuser
Copy link
Member

Ok, I think I misinterpreted the use and scope of this PR. It's probably designed for such things: https://discourse.pi-hole.net/t/ftl-crashed/72441

Copy link
Member

@yubiuser yubiuser left a comment

Choose a reason for hiding this comment

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

Do we really need this --skip-end code? How many binaries do you know that append their checksum at the end? How many users do you expect to use FTLto check another binary?


Are you logging checksum mismatches during startup also to the logfiles or only to the database?

@rdwebdesign
Copy link
Member

Are you logging checksum mismatches during startup also to the logfiles or only to the database?

Apparently the message is logged to FTL.log and the database:

FTL/src/files.c

Lines 832 to 833 in cdba28e

if(!verbose) // during startup
log_verify_message(expected_hex, actual_hex);

void log_verify_message(const char *expected, const char *actual)
{
// Create message
char buf[2048];
snprintf(buf, sizeof(buf), "Corrupt binary detected - this may lead to unexpected behaviour!");
// Log to FTL.log
log_crit("%s", buf);
// Log to database
add_message(VERIFY_MESSAGE, buf, expected, actual, GIT_HASH, FTL_ARCH);
}

@DL6ER
Copy link
Member Author

DL6ER commented Sep 27, 2024

How many binaries do you know that append their checksum at the end?

Well, this is a fair point. It needed to get the "real" SHA256 checksum of the FTL binary but this may be irrelevant as we have (a) the new pihole-FTL verify tool and (b) any external sha256sum (like those hosted on ftl.pi-hole.net) are different anyway as they include the full binary (including the checksum at the end).

We could use pihole-FTL sha256sum --skip-end to have the same checksums everywhere:

$ pihole-FTL verify
[✓] SHA256 checksum matches

$ od -tx --endian=big pihole-FTL | tail
121722640    00000000 00000000 80334501 00000000
121722660    aa660200 00000000 00000000 00000000
121722700    01000000 00000000 00000000 00000000
121722720    11000000 03000000 00000000 00000000
121722740    00000000 00000000 2a9a4701 00000000
121722760    a4010000 00000000 00000000 00000000
121723000    01000000 00000000 00000000 00000000
121723020 -> 6c1a0968 3c059e9c 1d5ae4fe 613b290f <-
121723040 -> 3c852b65 c31a28dc 70d95dc5 59a3f95d <-

$ sha256sum pihole-FTL
8c33200a695906a47fa5f02bf60450b71032a8b7d91747e8455b233e984dd238  pihole-FTL

$ pihole-FTL sha256sum pihole-FTL
8c33200a695906a47fa5f02bf60450b71032a8b7d91747e8455b233e984dd238  pihole-FTL

$ pihole-FTL sha256sum --skip-en pihole-FTL
6c1a09683c059e9c1d5ae4fe613b290f3c852b65c31a28dc70d95dc559a3f95d  pihole-FTL

There is actually typo in the code so you'd need to write, e.g., --skip-en to get that feature. My next commit will be either fixing this or removing the feature, depending on what we agree on. Till then, I will make this a draft.


edit

or ...

$ cp pihole-FTL pihole-FTL.tmp

$ ls -l pihole-FTL.tmp 
-rwxrwxr-x 1 d d 21472664 Sep 27 12:13 pihole-FTL.tmp

$ truncate -s $(($(stat -c%s pihole-FTL.tmp)-32)) pihole-FTL.tmp 

$ ls -l pihole-FTL.tmp 
-rwxrwxr-x 1 d d 21472632 Sep 27 12:13 pihole-FTL.tmp

$ sha256sum pihole-FTL.tmp
6c1a09683c059e9c1d5ae4fe613b290f3c852b65c31a28dc70d95dc559a3f95d  pihole-FTL

but this is not really any prettier

@DL6ER DL6ER marked this pull request as draft September 27, 2024 10:11
@rdwebdesign
Copy link
Member

Suggestion:

If the issue is the use of pihole-FTL with 2 arguments (sha256sum --skip-end), I think we can use a different approach.

Change the function to pihole-FTL autocheck and internally autocheck will behave like sha256sum --skip-end.

Note:
autocheck is just a suggestion. We could use a different argument.

@yubiuser
Copy link
Member

Change the function to pihole-FTL autocheck and internally autocheck will behave like sha256sum --skip-end.

I think this is what verify does.


I still think we could remove the skip-end as we have verify.


Crazy. maybe even impossible: can we append the sha to the binary from the binary already including the sha? Or is this a chicken-egg-situation?

@DL6ER
Copy link
Member Author

DL6ER commented Sep 29, 2024

You are right, including a checksum in the data being used to compute the checksum creates a paradoxical situation. The issue here is called a "Circular Dependency":

When you compute a checksum, the algorithm processes all the data to generate a unique hash value that represents the data's integrity. If the checksum itself is part of the data, the value of the checksum would influence the result of the checksum computation. This creates a circular dependency: the checksum value depends on the data, which includes the checksum value. Hence, this will result in an infinite loop computing the checksum over and over.

Example Scenario:

Imagine you have a file with the following content:

Hello, World!
<not yet known checksum>

You want to compute the checksum of the entire file, including the placeholder for the checksum.
When you compute the checksum, you get a value, say ABC123.
You replace the placeholder with ABC123, but now the data has changed:

Hello, World!
ABC123

This new data will produce a different checksum, say DEF456.
Replacing the checksum again will change the data once more, leading to yet another checksum value.

To avoid this problem, the only possible solution is to store the checksum is separately from the data it verifies. For example:

  1. Separate File: Store the checksum in a separate file (...sha files as on ftl.pi-hole.net), or
  2. Appended Data: Append the checksum at the end of the data after computing it, ensuring it is not part of the data used for the checksum calculation.

By keeping the checksum separate, you ensure that the data remains consistent during the checksum computation, allowing for accurate verification of data integrity.

How about using hash collisions?

One might think that since the SHA256 algorithm is simple (enough) and since the number of collisions is infinite, there should be a way to find a modification for the given hash so the checksum over the entire file (incl. the hash) results this exact hash. But currently there is no algorithm known to find a file that produces the given hash within a practically acceptable time. This property is called pre-image resistance. By "no algorithm known" we mean no algorithm except of brute-forcing, which in case of SHA-256 would require the computing power of the whole world for a time longer than the Universe exists to date.

SHA-256 was designed to be used for cryptographic purposes and pre-image resistance is a desired property - see also here.

@DL6ER
Copy link
Member Author

DL6ER commented Sep 29, 2024

image

@DL6ER DL6ER marked this pull request as ready for review September 29, 2024 08:42
@DL6ER DL6ER requested a review from a team September 29, 2024 08:43
src/args.c Dismissed Show dismissed Hide dismissed
Signed-off-by: DL6ER <[email protected]>
Copy link
Member

@yubiuser yubiuser left a comment

Choose a reason for hiding this comment

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

2024-09-30_21-18

Should we always print the hash, not only on mismatch?

src/files.c Show resolved Hide resolved
@DL6ER
Copy link
Member Author

DL6ER commented Sep 30, 2024

Should we always print the hast, not only on mismatch?

The hash used by FTL to verify itself is a different one, though. It is the --skip-end one ;-)

edit See the bottom of #2072 (comment) how to retrieve it without --skip-end

Co-authored-by: yubiuser <[email protected]>
Signed-off-by: Dominik <[email protected]>
@DL6ER DL6ER merged commit 6c0c875 into development Oct 1, 2024
19 checks passed
@DL6ER DL6ER deleted the new/verify branch October 1, 2024 04:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants