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 11 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 *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 ctx be used via the EVP API?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The argument ctx perhaps is a confusing name, it comes directly from the FIPS 204 standard page 17 and 18.

"The signing algorithm ML-DSA.Sign (Algorithm 2) takes a private key, a message, and a context string as
input. Algorithm 2 ML-DSA.Sign(sk, M, ctx)".

This is not a PKEY, EVP, (or any other type of) ctx.

Upstream reference Dilithium call it "pre" (https://github.com/pq-crystals/dilithium/blob/master/ref/sign.c#L89) short for "prefix string". But I wanted it to be obvious from the standard what this argument was, so went with the standard name.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. I was asking whether the EVP_PKEY API will be able to accommodate that extra parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh I understand now! Currently no, there is no way to set ctx from the EVP API. Should we require that functionality, it can be added.

For additional background, the standard states: "By default, the context is the empty string, though applications may specify the use of a non-empty context string." I believe this was added as an optional meta-data field when adding in the concept of domain separation for pre-hash vs pure signing modes.

Should we want to make the ctx settable at the EVP API we'd need to make changes to the EVP API that I currently do not see an application for.

Copy link
Contributor

Choose a reason for hiding this comment

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

please document that in the code. btw, ctx is very unfortunate name, can we change it to ctx_string or something like that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! Changed to pre for prefix string as has now also been changed upstream (https://github.com/pq-crystals/dilithium/blob/master/ref/sign.c#L78). See bcbb832

size_t ctx_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 *ctx,
size_t ctx_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
35 changes: 18 additions & 17 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,32 +26,32 @@
// 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 *ctx /* IN */,
size_t ctx_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);
}

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 *ctx /* IN */,
size_t ctx_len /* IN */) {
ml_dsa_params params;
ml_dsa_65_params_init(&params);
return crypto_sign_verify(&params, sig, sig_len, message, message_len,
Expand Down
50 changes: 50 additions & 0 deletions crypto/dilithium/ml_dsa.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
// SPDX-License-Identifier: Apache-2.0 OR ISC

#ifndef SIG_DILITHIUM_H
dkostic marked this conversation as resolved.
Show resolved Hide resolved
#define SIG_DILITHIUM_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

// ml_dsa_65_keypair generates an ML-DSA-65 keypair and assigns a public key to
// |public_key| and a private key to |secret_key|. It returns 0 upon success.
Copy link
Contributor

Choose a reason for hiding this comment

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

It returns 0 upon success.

Can it fail?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it can return -1 when prefix string too long, added comment to indicate this bcbb832

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!


// ml_dsa_65_sign generates an ML-DSA-65 signature. Where |secret_key| is a
dkostic marked this conversation as resolved.
Show resolved Hide resolved
// pointer to bit-packed secret key, |sig| is a pointer to output signature,
// |sig_len| is a pointer to output length of signature, |message| is a pointer
// to message to be signed, |message_len| is the length of the message, |ctx| is
// a pointer to the context string, and |ctx_len| is the length of the context
// string (max length 255 bytes). It returns 0 upon success.
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);

// ml_dsa_65_verify generates an ML-DSA-65 signature. Where |public_key| is a
// pointer to bit-packed public key, |sig| is a pointer to input signature,
// |sig_len| is the length of the signature, |message| is a pointer to message,
// |message_len| is the length of the message, |ctx| is a pointer to the context
// string, and |ctx_len| is the length of the context string (max length 255
// bytes). Returns 0 if signature could be verified successfully and -1 otherwise.
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