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 for txt output length of plain PQ key material #268

Merged
merged 3 commits into from
Oct 3, 2023
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions oqsprov/oqs_encode_key2any.c
Original file line number Diff line number Diff line change
Expand Up @@ -1160,7 +1160,7 @@ static int oqsx_to_text(BIO *out, const void *key, int selection)
}

if ((selection & OSSL_KEYMGMT_SELECT_PRIVATE_KEY) != 0) {
int classic_key_len = 0;
int classic_key_len = 0 - SIZE_OF_UINT32;
Copy link
Member

Choose a reason for hiding this comment

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

Initializing a length variable to a negative number, seems counterintuitive. I'll assume what you've done indeed fixes the bug, but I wonder if there's a more natural way, or at least if there should be a comment explaining this counterintuitive step.

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps you can walk me through the logic in the subsequent: does line 1169 add to classic_key_len, so that on 1171 it is now non-negative?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. The code is shared between hybrid (numkeys>1) and plain PQ keys. The classic_key_len element is (obviously) only relevant for hybrid keys (as is, err, should be, the 4 bytes storing it). This pattern is not unusual throughout provider -- but I agree that initializing a length to a negative value (to get it added back in the subsequent keytype independent subtraction for plain PQ keys) may be "surprising". Will revise the code for easier understanding -- but doing this "for real", i.e., in the complete code base, will probably require many more changes...


if (okey->numkeys > 1) {
char classic_label[200];
Expand All @@ -1178,7 +1178,7 @@ static int oqsx_to_text(BIO *out, const void *key, int selection)
return 0;
}
if ((selection & OSSL_KEYMGMT_SELECT_PUBLIC_KEY) != 0) {
int classic_key_len = 0;
int classic_key_len = 0 - SIZE_OF_UINT32;

if (okey->numkeys > 1) {
char classic_label[200];
Expand Down