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

Addition of generic NIST-DSA PKEY and ASN1 to support ML-DSA #1963

Merged
merged 28 commits into from
Nov 18, 2024
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
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
9 changes: 5 additions & 4 deletions crypto/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -323,9 +323,10 @@ if(ENABLE_DILITHIUM)
set(
DILITHIUM_SOURCES

dilithium/p_dilithium3.c
dilithium/p_dilithium3_asn1.c
dilithium/sig_dilithium3.c
dilithium/pqdsa.c
dilithium/p_pqdsa.c
Copy link
Contributor

Choose a reason for hiding this comment

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

np: indent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in 7435f9b

dilithium/p_pqdsa_asn1.c
dilithium/ml_dsa.c
)
endif()

Expand Down Expand Up @@ -774,7 +775,7 @@ if(BUILD_TESTING)
ecdh_extra/ecdh_test.cc
dh_extra/dh_test.cc
digest_extra/digest_test.cc
dilithium/p_dilithium_test.cc
dilithium/p_pqdsa_test.cc
Copy link
Contributor

Choose a reason for hiding this comment

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

np: indent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in 7435f9b

dsa/dsa_test.cc
des/des_test.cc
endian_test.cc
Expand Down
70 changes: 70 additions & 0 deletions crypto/dilithium/internal.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved.
// SPDX-License-Identifier: Apache-2.0 OR ISC

#ifndef AWSLC_HEADER_SIG_INTERNAL_H
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#ifndef AWSLC_HEADER_SIG_INTERNAL_H
#ifndef AWSLC_HEADER_PQDSA_INTERNAL_H

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in 7435f9b

#define AWSLC_HEADER_SIG_INTERNAL_H

#include <openssl/base.h>

#if defined(__cplusplus)
extern "C" {
#endif

// PQDSA_METHOD structure and helper functions.
typedef struct {
dkostic marked this conversation as resolved.
Show resolved Hide resolved
int (*keygen)(uint8_t *public_key,
dkostic marked this conversation as resolved.
Show resolved Hide resolved
uint8_t *secret_key);

int (*sign)(const uint8_t *secret_key,
uint8_t *sig,
size_t *sig_len,
const uint8_t *message,
size_t message_len,
const uint8_t *pre,
size_t pre_len);

int (*verify)(const uint8_t *public_key,
const uint8_t *sig,
size_t sig_len,
const uint8_t *message,
size_t message_len,
const uint8_t *pre,
size_t pre_len);

} PQDSA_METHOD;

// PQDSA structure and helper functions.
typedef struct {
int nid;
const uint8_t *oid;
uint8_t oid_len;
const char *comment;
size_t public_key_len;
size_t secret_key_len;
size_t signature_len;
size_t keygen_seed_len;
size_t sign_seed_len;
const PQDSA_METHOD *method;
} PQDSA;

// PQDSA_KEY structure and helper functions.
struct pqdsa_key_st {
const PQDSA *pqdsa;
uint8_t *public_key;
uint8_t *secret_key;
};

int PQDSA_KEY_init(PQDSA_KEY *key, const PQDSA *pqdsa);
const PQDSA * PQDSA_find_dsa_by_nid(int nid);
const PQDSA *PQDSA_KEY_get0_dsa(PQDSA_KEY* key);
PQDSA_KEY *PQDSA_KEY_new(void);
void PQDSA_KEY_free(PQDSA_KEY *key);
int EVP_PKEY_pqdsa_set_params(EVP_PKEY *pkey, int nid);

int PQDSA_KEY_set_raw_public_key(PQDSA_KEY *key, const uint8_t *in);
int PQDSA_KEY_set_raw_secret_key(PQDSA_KEY *key, const uint8_t *in);
#if defined(__cplusplus)
} // extern C
#endif

#endif // AWSLC_HEADER_DSA_TEST_INTERNAL_H
39 changes: 20 additions & 19 deletions crypto/dilithium/sig_dilithium3.c → crypto/dilithium/ml_dsa.c
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
// SPDX-License-Identifier: Apache-2.0 OR ISC

#include "../evp_extra/internal.h"
#include "../fipsmodule/evp/internal.h"
#include "sig_dilithium.h"
#include "ml_dsa.h"
#include "pqcrystals_dilithium_ref_common/sign.h"
#include "pqcrystals_dilithium_ref_common/params.h"

Expand All @@ -25,34 +26,34 @@
// depending on platform support.

int ml_dsa_65_keypair(uint8_t *public_key /* OUT */,
uint8_t *secret_key /* OUT */) {
uint8_t *secret_key /* OUT */) {
ml_dsa_params params;
ml_dsa_65_params_init(&params);
return crypto_sign_keypair(&params, public_key, secret_key);
return (crypto_sign_keypair(&params, public_key, secret_key) == 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

how did this work so far? aren't we checking the return value? because this change inverts the return value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

crypto_sign_keypair returns 0 on success -1 for failure. This is an anti-pattern with how EVP expects 1 for success. The return values are checked within the PKEY functions:

}

int ml_dsa_65_sign(uint8_t *sig /* OUT */,
size_t *sig_len /* OUT */,
const uint8_t *message /* IN */,
size_t message_len /* IN */,
const uint8_t *ctx /* IN */,
size_t ctx_len /* IN */,
const uint8_t *secret_key /* IN */) {
int ml_dsa_65_sign(const uint8_t *secret_key /* IN */,
uint8_t *sig /* OUT */,
size_t *sig_len /* OUT */,
const uint8_t *message /* IN */,
size_t message_len /* IN */,
const uint8_t *pre /* IN */,
Copy link
Contributor

Choose a reason for hiding this comment

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

what's pre?

size_t pre_len /* IN */) {
ml_dsa_params params;
ml_dsa_65_params_init(&params);
return crypto_sign_signature(&params, sig, sig_len, message, message_len,
dkostic marked this conversation as resolved.
Show resolved Hide resolved
ctx, ctx_len, secret_key);
pre, pre_len, secret_key);
}

int ml_dsa_65_verify(const uint8_t *message /* IN */,
size_t message_len /* IN */,
const uint8_t *sig /* IN */,
size_t sig_len /* IN */,
const uint8_t *ctx /* IN */,
size_t ctx_len /* IN */,
const uint8_t *public_key /* IN */) {
int ml_dsa_65_verify(const uint8_t *public_key /* IN */,
const uint8_t *sig /* IN */,
size_t sig_len /* IN */,
const uint8_t *message /* IN */,
size_t message_len /* IN */,
const uint8_t *pre /* IN */,
size_t pre_len /* IN */) {
ml_dsa_params params;
ml_dsa_65_params_init(&params);
return crypto_sign_verify(&params, sig, sig_len, message, message_len,
ctx, ctx_len, public_key);
pre, pre_len, public_key);
}
36 changes: 36 additions & 0 deletions crypto/dilithium/ml_dsa.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
// SPDX-License-Identifier: Apache-2.0 OR ISC

#ifndef ML_DSA_H
#define ML_DSA_H

#include <stddef.h>
#include <stdint.h>
#include <openssl/base.h>
#include <openssl/evp.h>

#define MLDSA65_PUBLIC_KEY_BYTES 1952
#define MLDSA65_PRIVATE_KEY_BYTES 4032
#define MLDSA65_SIGNATURE_BYTES 3309
#define MLDSA65_KEYGEN_SEED_BYTES 32
#define MLDSA65_SIGNATURE_SEED_BYTES 32

int ml_dsa_65_keypair(uint8_t *public_key,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why were the comments preceding the functions removed? This could be why the rename considered it a new file.

Copy link
Contributor Author

@jakemas jakemas Nov 14, 2024

Choose a reason for hiding this comment

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

Dusan said they were superfluous earlier on in this review, so I removed them. (#1963 (comment))

uint8_t *secret_key);
Copy link
Contributor

Choose a reason for hiding this comment

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

np: indent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

missed it in this commit, will hit it next time!


int ml_dsa_65_sign(const uint8_t *secret_key,
uint8_t *sig,
size_t *sig_len,
const uint8_t *message,
size_t message_len,
const uint8_t *ctx,
Copy link
Contributor

Choose a reason for hiding this comment

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

can we change this to cxt_string? same for verify

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in 7435f9b

size_t ctx_len);

int ml_dsa_65_verify(const uint8_t *public_key,
const uint8_t *sig,
size_t sig_len,
const uint8_t *message,
size_t message_len,
const uint8_t *ctx,
size_t ctx_len);
#endif
125 changes: 0 additions & 125 deletions crypto/dilithium/p_dilithium3.c

This file was deleted.

Loading
Loading