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

update to liboqs 0.9.1 #266

Closed
wants to merge 1 commit into from
Closed

update to liboqs 0.9.1 #266

wants to merge 1 commit into from

Conversation

aparcar
Copy link
Contributor

@aparcar aparcar commented Mar 13, 2024

The release updated the Classic McEliece implemented and since we use it, might be a good idea to update the lib.

@koraa
Copy link
Member

koraa commented Mar 20, 2024

This is fascinating

thread 'fuzz_handle_msg' panicked at /home/runner/work/rosenpass/rosenpass/secret-memory/src/secret.rs:152:31:
source slice length (13568) does not match destination slice length (13608)
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Does the fuzzer crash reproduce when you run locally?

@koraa
Copy link
Member

koraa commented Mar 20, 2024

Oh by the way, it is possible that the updated OQS actually has new versions of McEliece & Kyber, in which case this crash would make sense

@aparcar
Copy link
Contributor Author

aparcar commented Mar 20, 2024

Oh by the way, it is possible that the updated OQS actually has new versions of McEliece & Kyber, in which case this crash would make sense

How so?

@aparcar
Copy link
Contributor Author

aparcar commented Apr 8, 2024

In other words, does this mean with a bump all deployed setups would break?

@koraa
Copy link
Member

koraa commented Apr 8, 2024

Oh by the way, it is possible that the updated OQS actually has new versions of McEliece & Kyber, in which case this crash would make sense

How so?

Well, occasionally OQS just updates their implementations

In other words, does this mean with a bump all deployed setups would break?

I would say: All deployed setups should update.

Anway. Lets fix and merge this but please add info to the commit indicating whether the algorithm versions where changed.

The release updated the Classic McEliece implemented and since we use
it, might be a good idea to update the lib.

Signed-off-by: Paul Spooren <mail@aparcar.org>
@prabhpreet
Copy link
Contributor

Looking into the cargo fuzz failure, will add any findings here

@aparcar
Copy link
Contributor Author

aparcar commented Apr 29, 2024

Great thanks

@prabhpreet
Copy link
Contributor

prabhpreet commented Apr 29, 2024

The secret key size and the cipher text length have changed in the McEliece KEM we are using.

https://github.com/open-quantum-safe/liboqs/blob/7c3a0e9aa7f9568e4dcafaf908ff8aa0008f0b71/src/kem/classic_mceliece/kem_classic_mceliece.h#L32-L33

So the slice size turns out to be smaller when copying the slice into Secret for the handle_msg.rs test

https://github.com/aparcar/rosenpass/blob/5c84df41454a36bf3a0241583273e6f694414aa0/fuzz/fuzz_targets/handle_msg.rs#L10

I've added some updates to use constants instead of manually specifying slice sizes in these tests for KEMs on this branch:

https://github.com/prabhpreet/rosenpass/tree/liboqs-910

@aparcar I don't have permissions to write to your branch. Can you add the changes from the branch and see if the pipeline passes?

@prabhpreet
Copy link
Contributor

@aparcar Tests are passing with changes- please feel free to review #292 and incorporate the changes or review that PR and close this one

@aparcar
Copy link
Contributor Author

aparcar commented May 2, 2024

Superseded by #292

@aparcar aparcar closed this May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants