Skip to content

add error check of aes_encrypt() #15576

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 31, 2024
Merged

add error check of aes_encrypt() #15576

merged 1 commit into from
Jan 31, 2024

Conversation

Hxinrong
Copy link

@Hxinrong Hxinrong commented Dec 8, 2020 •
edited
Loading

Contribution description

sys/random/fortuna/fortuna.c:add error check of aes_encrypt()
I'm not sure which exact value should be returned when aes_encrypt() is not on success. I just return the return value of it.

Issues/PRs references

Addresses parts of #15524

@Hxinrong Hxinrong requested a review from basilfx as a code owner December 8, 2020 03:20
@benpicco benpicco added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: security Area: Security-related libraries and subsystems labels Dec 8, 2020
benpicco
benpicco approved these changes Dec 9, 2020
View reviewed changes
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

this looks good too :)

@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Dec 9, 2020
basilfx
basilfx previously requested changes Dec 17, 2020
View reviewed changes
sys/random/fortuna/fortuna.c Outdated
aes_encrypt(&cipher, state->gen.counter.bytes, out + (i * 16));
res = aes_encrypt(&cipher, state->gen.counter.bytes, out + (i * 16));
if (res != 1) {
return res;
Copy link
Member

Choose a reason for hiding this comment

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

I wrote this implementation some time ago. I think the return code should be negative, and I'm not sure about res being negative.

Copy link
Member

Choose a reason for hiding this comment

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

Frme the doc:

1 on success
A negative value if the cipher key cannot be expanded with the AES key schedule

fortuna_generate_blocks is an internal function so it should be fine to return the negative value. But maybe an "internal" documentation of that function would be beneficial.

@fjmolinas
Copy link
Contributor

ping @Hxinrong! :)

@jeandudey jeandudey linked an issue Mar 20, 2021 that may be closed by this pull request
Missing error check of several functions #15524
Closed
@jeandudey jeandudey mentioned this pull request Mar 20, 2021
Closed
@MrKevinWeiss MrKevinWeiss added this to the Release 2021.07 milestone Jun 21, 2021
@MrKevinWeiss MrKevinWeiss removed this from the Release 2021.07 milestone Jul 15, 2021
@PeterKietzmann
Copy link
Member

@basilfx you put a change request on this PR. What did you want changed?

@basilfx
Copy link
Member

basilfx commented Jun 29, 2022

@PeterKietzmann I think that it should not return res but return -3 (or another negative value) if aes_encrypt != 1.

The documentation states that aes_encrypt either returns 1 or a negative value. Returning res could be a problem if aes_encrypt would return something else (due to an API change, another implementation, etc).

@github-actions github-actions bot added the Area: sys Area: System label Jan 25, 2024
@Teufelchen1
Copy link
Contributor

@basilfx could you give it another look?

@maribu maribu dismissed basilfx’s stale review January 30, 2024 17:10

Review is stale

benpicco
benpicco approved these changes Jan 30, 2024
View reviewed changes
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

I think this should just blow an assert() since nothing about this should depend on the run-time state.

But then again I don't expect anyone to use this over any of the more 'slim' RNGs, so it probably doesn't matter ¯\(ツ)/¯

@Teufelchen1 Teufelchen1 enabled auto-merge January 30, 2024 18:15
@riot-ci
Copy link

riot-ci commented Jan 30, 2024

Murdock results

✔️ PASSED

bf3c038 sys/random/fortuna/fortuna.c:add error check of aes_encrypt()

Success Failures Total Runtime
8629 0 8629 11m:10s

Artifacts

@Teufelchen1 Teufelchen1 added this pull request to the merge queue Jan 30, 2024
Merged via the queue into RIOT-OS:master with commit 0d04b73 Jan 31, 2024
@MrKevinWeiss MrKevinWeiss added this to the Release 2024.04 milestone Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benpicco benpicco benpicco approved these changes

@basilfx basilfx basilfx left review comments

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann

Assignees
No one assigned
Labels
Area: security Area: Security-related libraries and subsystems Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Milestone
Release 2024.04
Development

Successfully merging this pull request may close these issues.

Missing error check of several functions
8 participants
@Hxinrong @fjmolinas @PeterKietzmann @basilfx @Teufelchen1 @riot-ci @benpicco @MrKevinWeiss