Check file size of snapshot_version when unarchiving snapshot

[mooori]

Problem

The size of the snapshot_version file is not checked, which is an attack vector. See #18504, #18841.

Summary of Changes

Check the size of the file containing the snapshot_version before opening it. If the size exceeds MAX_SNAPSHOT_VERSION_FILE_SIZE, an error is returned and the file will not be opened.

Notes

• MAX_SNAPSHOT_VERSION_FILE_SIZE was chosen arbitrarily - let me know if another value makes more sense.
• If the file size is too big, an error is created and returned according to other code in snapshot_utils dealing with files that are too big (namely create_snapshot_data_file_stream).

Test

So far, there is no dedicated test for unarchive_snapshot. Setting up such a test just to verify the snapshot_version file size is checked correctly looks like overkill.

Fixes #18841

[mvines]

Oh nice. This PR lgtm but @brooksprumo can you please take a look as well for a full review

[brooksprumo]

@mooori Thank you for your submission! Please mark the PR as "Ready for review" when you feel it's completed and I will take a look.

[mooori]

@brooksprumo CI checks haven't finished yet, but since it's just a small PR I think it's good to go (status is updated).

If there should be any issues, I'll be back later. I ran cargo test, cargo fmt and cargo clippy locally inside runtime and they didn't complain.

[codecov]

Codecov Report

Merging #21925 (96c39dc) into master (56ec524) will decrease coverage by 0.0%.
The diff coverage is 100.0%.

@@            Coverage Diff            @@
##           master   #21925     +/-   ##
=========================================
- Coverage    81.3%    81.2%   -0.1%     
=========================================
  Files         516      516             
  Lines      144323   144346     +23     
=========================================
- Hits       117347   117346      -1     
- Misses      26976    27000     +24     

[brooksprumo]

Overall looks good! I'm requesting some small tweaks.

Also:

So far, there is no dedicated test for unarchive_snapshot. Setting up such a test just to verify the snapshot_version file size is checked correctly looks like overkill.

I'd like tests for the file-to-snapshot-version function (see comments below). Those tests do not necessarily need to be part of this PR though. If you'd rather not add the tests here, please create a new issue name something like "Add tests for snapshot_version_from_file" and mark it as "good first issue".

I'd like two tests:

1. Ensure that a version file with the default SnapshotVersion passes
2. Ensure that a version file with a size MAX_SNAPSHOT_VERSION_FILE_SIZE + 1 fails

[brooksprumo]

Oh, also please re-request a review from me once the changes are made. This way I'll make sure to see the review request in my GH dashboard.

Pull request has been modified.

[mooori]

Thank you for the review @brooksprumo. The suggestions have been implemented (including tests).

[brooksprumo]

I like it! Two more small things and then I think it'll be all set!

Pull request has been modified.

[brooksprumo]

Looks good! Thanks again for your submission, @mooori!

[brooksprumo]

@Mergifyio backport v1.8

[mergify]

backport v1.8

✅ Backports have been created

• #21982 Check file size of snapshot_version when unarchiving snapshot (backport #21925) has been created for branch v1.8