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

sys/crypto: fix OCB mode #20745

Merged
merged 1 commit into from
Jun 14, 2024
Merged

sys/crypto: fix OCB mode #20745

merged 1 commit into from
Jun 14, 2024

Conversation

LP-HAW
Copy link
Contributor

@LP-HAW LP-HAW commented Jun 11, 2024

Contribution description

This PR fixes the Offset_0 calculation in OCB mode. There was a failed attempt to discard the upper two bits of nonce_padded[15]. The tests did not catch this because all nonces have zeros in those positions.

Perhaps we should just remove OCB mode, as it has apparently never been used by anyone (including its authors) since it was merged 5 years ago.

Testing procedure

make -C tests/sys/crypto all test

Issues/PRs references

None

@github-actions github-actions bot added Area: tests Area: tests and testing framework Area: sys Area: System labels Jun 11, 2024
@benpicco benpicco requested a review from mguetschow June 13, 2024 15:17
@benpicco benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Jun 13, 2024
@riot-ci
Copy link

riot-ci commented Jun 13, 2024

Murdock results

✔️ PASSED

b51d8e3 sys/crypto: fix OCB mode

Success Failures Total Runtime
10178 0 10178 17m:10s

Artifacts

Copy link
Contributor

@mguetschow mguetschow left a comment

Choose a reason for hiding this comment

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

Thanks for finding and fixing, and special kudos for adding a dedicated test for it, well done! :)

Is the reason of the failed attempt that the left- and right-shift operations got optimized away by the compiler? If so, is this line another instance of the same problem? Just found it by searching for the same pattern.

https:/RIOT-OS/RIOT/blob/master/drivers/my9221/my9221.c#L126

@mguetschow mguetschow added this pull request to the merge queue Jun 14, 2024
@LP-HAW
Copy link
Contributor Author

LP-HAW commented Jun 14, 2024

Is the reason of the failed attempt that the left- and right-shift operations got optimized away by the compiler?

Its because of integer promotion. Before the shift operations, nonce_padded[15] is converted to an int which is either 16 or 32 bit depending on the platform.

If so, is this line another instance of the same problem? Just found it by searching for the same pattern.

I think the code in my9221.c should be fine because after the left shift, the result is cast to an uint16_t, which should cut off the upper bits. We could add braces to make it clear that the cast is applied after the shift left and before the shift right.

@mguetschow
Copy link
Contributor

I see, thanks for the explanation :)

Merged via the queue into RIOT-OS:master with commit b879bbd Jun 14, 2024
28 checks passed
@mguetschow mguetschow added this to the Release 2024.07 milestone Jul 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: sys Area: System Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants