Skip to content

add error check of aes_encrypt()#15576

Merged
Teufelchen1 merged 1 commit intoRIOT-OS:masterfrom
Hxinrong:aes_encryptBranch
Jan 31, 2024
Merged

add error check of aes_encrypt()#15576
Teufelchen1 merged 1 commit intoRIOT-OS:masterfrom
Hxinrong:aes_encryptBranch

Conversation

@Hxinrong
Copy link
Copy Markdown

@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
Copy Markdown
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
Copy Markdown
Member

@basilfx basilfx Dec 17, 2020

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
Copy Markdown
Member

@PeterKietzmann PeterKietzmann May 31, 2022

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
Copy Markdown
Contributor

fjmolinas commented Jan 20, 2021

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
Copy Markdown
Member

PeterKietzmann commented Jun 28, 2022

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

@basilfx
Copy link
Copy Markdown
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
Copy Markdown
Contributor

Teufelchen1 commented Jan 30, 2024

@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
Copy Markdown
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
Copy Markdown

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