-
Notifications
You must be signed in to change notification settings - Fork 484
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
Add XMSS Serialize/Deserialize #1542
Add XMSS Serialize/Deserialize #1542
Conversation
Fix use of uninitialized variable.
OQS_STATUS OQS_SECRET_KEY_XMSS_deserialize_key(OQS_SIG_STFL_SECRET_KEY *sk, const size_t sk_len, const uint8_t *sk_buf) { | ||
if (sk == NULL || sk_buf == NULL || (sk_len == 0) || (sk_len != sk->length_secret_key)) { | ||
return OQS_ERROR; | ||
} | ||
|
||
if (sk->secret_key_data) { | ||
// Key data already present | ||
// We dont want to trample over data | ||
return OQS_ERROR; | ||
} | ||
|
||
memcpy(sk->secret_key_data, sk_buf, sk_len); | ||
|
||
return OQS_SUCCESS; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I have this piece of code that we discuss in the meeting:
So we check if the address of array secret_key_data
is either NULL or not. If it's not NULL, we assume it's already initialized, thus we return error.
To reach line 39, sk->secret_key_data
must be NULL. Which make the memcpy
is SEGFAULT.
I'm thinking of creating static once = 1
as static variable to make sure the initialization happen only once.
But this lead to:
- We will have 2 ways to allocate memory for
sk->secret_key_data
, either indeserialize
, and in key generation. - If I use
static
variable, I don't know in the case we multithread/fork process, would it become complicate to make sure thesk->secret_key_data
happen only once withstatic
variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After a while I change the logic of the code to:
/* Deserialize XMSS byte string into an XMSS secret key data */
OQS_STATUS OQS_SECRET_KEY_XMSS_deserialize_key(OQS_SIG_STFL_SECRET_KEY *sk, const size_t sk_len, const uint8_t *sk_buf) {
if (sk == NULL || sk_buf == NULL || (sk_len != sk->length_secret_key)) {
return OQS_ERROR;
}
if (sk->secret_key_data != NULL) {
// Key data already present
// We dont want to trample over data
return OQS_ERROR;
}
// Assume key data is not present
sk->secret_key_data = malloc(sk_len);
if (sk->secret_key_data == NULL) {
return OQS_ERROR;
}
memcpy(sk->secret_key_data, sk_buf, sk_len);
return OQS_SUCCESS;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Committed in 7e4f2c9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks fine to me.
It might be good to walk through the use-case where it is necessary to call "deserialize' on a working secret key object. Those use-cases would help determine the desired behaviors for each expected outcome.
This looks good to me. I'll merge this PR. |
* Add serialize and deserialize to XMSS --------- Co-authored-by: Norman Ashley <nashley@cisco.com>
* Add serialize and deserialize to XMSS --------- Co-authored-by: Norman Ashley <nashley@cisco.com>
* Add serialize and deserialize to XMSS --------- Co-authored-by: Norman Ashley <nashley@cisco.com>
* Add serialize and deserialize to XMSS --------- Co-authored-by: Norman Ashley <nashley@cisco.com>
* Add serialize and deserialize to XMSS --------- Co-authored-by: Norman Ashley <nashley@cisco.com>
commit 244288f Add XMSS parameter xmss_sha256_h10 (#1482) commit a7e26d9 Add 12 XMSS and 16 XMSSMT parameters. (#1489) commit 4694fc3 Add secret key object to XMSS (#1530) commit 99067be Add XMSS Serialize/Deserialize (#1542) commit 2dbfc40 Update XMSS secret key object APIs, sync with LMS (#1588) commit 47740ad Enforce idx from unsigned int to uint32_t. (#1611) commit 9610576 Fix windows-x86 and arm compiling error. (#1634) commit bb658b7 Address stateful-sigs comments in #1650 (#1656) commit 7db8ddf Update `sig_stfl.h` document for #1650 (#1655) commit c3e5750 Add Apache 2.0 and MIT License to XMSS (#1662) commit e1f02b2 Change XMSS License from `(Apache 2.0 AND MIT)` to `(Apache 2.0 OR MIT) AND CC0-1.0` (#1697) commit 17c12c3 Add return status for XMSS lock/unlock functions. (#1712) commit 1941636 Add return check for lock/unlock function (#1727) commit b45415c Use `abort()` instead of exit to get the trace log. (#1728) commit ba63672 Reduce number of `malloc/free` call in `XMSS/external` (#1724) Signed-off-by: Duc Tri Nguyen <dnguye69@gmu.edu>
LMS
This PR slightly modify #1533
I tried to keep the same logic as possible. Please let me know if there is something wrong @ashman-p
XMSS
Add serialize and deserialize function to XMSS.