Skip to content

Conversation

aawsome
Copy link
Member

@aawsome aawsome commented Jun 28, 2025

see e.g. rustic-rs/rustic#1439
This also introduces the repository config option use-pack-padding which allows to disable the padding.

As a side-effect currently wrong statistics (data added to blobs in stats was without pack header) has been corrected.

depends on #409

@aawsome aawsome changed the title Add padding blob to data packs to mitigate chunking attacks WIP: Add padding blob to data packs to mitigate chunking attacks Jun 28, 2025
@aawsome aawsome added the A-security Area: Security related label Jun 28, 2025
@aawsome aawsome changed the title WIP: Add padding blob to data packs to mitigate chunking attacks Add padding blob to data packs to mitigate chunking attacks Jun 30, 2025
@aawsome aawsome added the S-blocked Status: Blocked from merging/working on due to some issue label Jul 2, 2025
Copy link

codecov bot commented Jul 2, 2025

Codecov Report

Attention: Patch coverage is 64.22287% with 122 lines in your changes missing coverage. Please review.

Project coverage is 44.0%. Comparing base (64d052d) to head (7738b70).

Files with missing lines Patch % Lines
crates/core/src/blob/repopacker.rs 61.3% 53 Missing ⚠️
crates/core/src/commands/prune.rs 64.4% 32 Missing ⚠️
crates/core/src/blob/packer.rs 72.7% 12 Missing ⚠️
crates/core/src/blob/pack_sizer.rs 65.0% 7 Missing ⚠️
crates/core/src/commands/repair/index.rs 0.0% 6 Missing ⚠️
crates/core/src/index/indexer.rs 89.2% 3 Missing ⚠️
crates/core/src/backend/decrypt.rs 33.3% 2 Missing ⚠️
crates/core/src/commands/copy.rs 0.0% 2 Missing ⚠️
crates/core/src/commands/merge.rs 0.0% 2 Missing ⚠️
crates/core/src/commands/repair/snapshots.rs 0.0% 2 Missing ⚠️
... and 1 more
Additional details and impacted files
Files with missing lines Coverage Δ
crates/core/src/archiver/file_archiver.rs 63.1% <100.0%> (ø)
crates/core/src/archiver/tree_archiver.rs 69.7% <100.0%> (ø)
crates/core/src/backend.rs 54.6% <ø> (+1.3%) ⬆️
crates/core/src/blob.rs 81.8% <ø> (+1.8%) ⬆️
crates/core/src/chunker.rs 50.5% <ø> (ø)
crates/core/src/commands/config.rs 35.8% <100.0%> (+0.8%) ⬆️
crates/core/src/repofile/configfile.rs 47.0% <100.0%> (-5.9%) ⬇️
crates/core/src/repofile/packfile.rs 63.4% <ø> (-5.7%) ⬇️
crates/core/src/archiver.rs 57.6% <66.6%> (-2.1%) ⬇️
crates/core/src/backend/decrypt.rs 48.1% <33.3%> (+0.2%) ⬆️
... and 9 more

... and 19 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@aawsome aawsome added the S-waiting-for-review Status: PRs waiting for review label Jul 2, 2025
source: err,
})?;
let data_len_packed: u64 = len.into();
self.stats.data_packed += data_len_packed;
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about just self.stats.data_packed += len?

// Add a padding blob
fn add_padding_blob(&mut self) -> RusticResult<()> {
pub(super) const KB: u32 = 1024;
pub(super) const MAX_PADDING: u32 = 64 * KB;
Copy link
Contributor

Choose a reason for hiding this comment

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

redundant definition of these conts (defined above in constants inlined module)

Comment on lines +361 to +366
let data = vec![
0;
padding_size
.try_into()
.expect("u32 should convert to usize")
];
Copy link
Contributor

Choose a reason for hiding this comment

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

As your "except" message says, to me there will never be an issue with converting padding_size to usize:

Suggested change
let data = vec![
0;
padding_size
.try_into()
.expect("u32 should convert to usize")
];
let data = vec![0; padding_size as usize];

Comment on lines 379 to 387
fn padding_size(size: u32) -> u32 {
// compute padding size. Note that we don't add zero-sized blobs here, i.e. padding_size is in 1..=MAX_PADDING.
let padding = constants::MAX_PADDING - size % constants::MAX_PADDING;
if padding == 0 {
constants::MAX_PADDING
} else {
padding
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I took a look at the restic PR for padding attack resilience, it seems they implemented the "padmé" padding "size" algorithm for this.

They reference this blogpost https://lbarman.ch/blog/padme/ and it seems interesting (you probably already have seen it since you participated in the restic issue on chunking attacks).

What's your opinion on that?
I am not strongly opinionated on this, but it seems that padmé is a thought through padding algorithm with a good balance between security and overhead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-security Area: Security related S-blocked Status: Blocked from merging/working on due to some issue S-waiting-for-review Status: PRs waiting for review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants