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

fix: off by one error in domain seperation #226

Closed

Conversation

bgorlick
Copy link

Looked like there was a fixed length assumption for version bytes causing off-by-one errors with an inaccurate tag length calculation potentially leading to hash collisions. The static domain tag construction is not reflecting actual content variations.

Hopefully this addresses it:
We can dynamically construct the domain tags based on actual content, ensuring uniqueness. This should accurately calculate full the tag length to prevent inconsistencies, and then from there we can update the digest with exact length bytes and then the constructed tag. My thoughts is it's safer to converts version and label bytes to strings safely using String::from_utf8. this version implements expect for error handling to clearly identify conversion failures. I looked over the broader class and it looks like the approach is to panic, so I kept it consistent instead of returning String, which I had initially considered.

Tested with build and cargo bench, and everything passes.

@SWvheerden
Copy link
Contributor

Thanks for the PR,
Can you please run fmt by running:

Run cargo +nightly-2024-02-01 fmt --all -- --check

@SWvheerden SWvheerden changed the title Refactor domain tag creation to dynamically construct with accurate l… fix: off by one error in domain seperation Apr 15, 2024
Comment on lines +108 to +109
String::from_utf8(label.to_vec())
.expect("Label conversion failed: invalid UTF-8")
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not a big fan of this approach.
S is a str, now we convert it to bytes, only to convert it back to a string, then down below, convert the whole string back to bytes again?

Copy link
Author

Choose a reason for hiding this comment

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

You raise a valid point, I will share a variation. My main mo was to get something initially to address the issue. Will refactor.

@AaronFeickert
Copy link
Contributor

Can you provide an example of how the off-by-one error occurs? It's not immediately clear to me.

@AaronFeickert
Copy link
Contributor

Superseded by #229. Closing.

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.

4 participants