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

Add ML-DSA post-quantum signatures to _CryptoExtras #267

Open
wants to merge 43 commits into
base: main
Choose a base branch
from

Conversation

fpseverino
Copy link
Contributor

@fpseverino fpseverino commented Oct 2, 2024

Add support for ML-DSA post-quantum digital signatures inside _CryptoExtras.

Checklist

  • I've run tests to see all new and existing tests pass
  • I've followed the code style of the rest of the project
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary

If you've made changes to gyb files

  • I've run .script/generate_boilerplate_files_with_gyb and included updated generated files in a commit of this pull request

Motivation:

With the advent of quantum computing, the mathematical foundations on which the cryptographic protocols in use today are based have been questioned, as they can easily be circumvented and violated by quantum computers.

While waiting for the creation of quantum computers that work at full capacity, and to protect network communications from "Harvest Now, Decrypt Later" attacks, the cryptographic community is working on post-quantum cryptography algorithms, which work on the traditional computers we use today, but are resistant to future attacks by quantum computers.

One of these algorithms is ML-DSA (AKA Dilithium), a module lattice-based signature scheme standardized by NIST in FIPS 204, that is available inside BoringSSL.

By including ML-DSA inside Swift Crypto, we can get closer to normalizing quantum secure algorithms and start implementing them into our apps and libraries to make them quantum-proof.

Modifications:

Added a MLDSA65 enum inside the _CryptoExtras module with corresponding PrivateKey, PublicKey and Signature structs that use BoringSSL methods to produce and verify ML-DSA-65 digital signatures, with the code style of other signature schemes in the library.

Added tests that cover use cases of the ML-DSA scheme, including test vectors taken from the BoringSSL repo (extracted from a .txt file and encoded in JSON).

Result:

ML-DSA-65 digital signatures can be created and verified with Swift Crypto.

Sources/_CryptoExtras/MLDSA/MLDSA_boring.swift Outdated Show resolved Hide resolved
Sources/_CryptoExtras/MLDSA/MLDSA_boring.swift Outdated Show resolved Hide resolved
Sources/_CryptoExtras/MLDSA/MLDSA_boring.swift Outdated Show resolved Hide resolved
Sources/_CryptoExtras/MLDSA/MLDSA_boring.swift Outdated Show resolved Hide resolved
Sources/_CryptoExtras/MLDSA/MLDSA_boring.swift Outdated Show resolved Hide resolved
Sources/_CryptoExtras/MLDSA/MLDSA_boring.swift Outdated Show resolved Hide resolved
Sources/_CryptoExtras/MLDSA/MLDSA_boring.swift Outdated Show resolved Hide resolved
Sources/_CryptoExtras/MLDSA/MLDSA_boring.swift Outdated Show resolved Hide resolved
Sources/_CryptoExtras/MLDSA/MLDSA_boring.swift Outdated Show resolved Hide resolved
Sources/_CryptoExtras/Util/Error.swift Outdated Show resolved Hide resolved
@fpseverino
Copy link
Contributor Author

fpseverino commented Oct 3, 2024

Hi @Lukasa, thank you very much for the quick feedback and directions.

I think I fixed most of your requested changes, but I have some problems with the DER and PEM representations.
Unlike RSA, where there are clear BoringSSL methods for generating and parsing them, I can't find any for ML-DSA. Maybe there are some generic ones? Or a pure Swift implementation (perhaps with swift-asn1) is possible/required?

Please let me know if the other changes I made are valid and how I could fix the PEM/DER issue.
Thanks again for the help!

@fpseverino fpseverino requested a review from Lukasa October 3, 2024 17:54
Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

Thanks for your fixes, I've left some more notes.

Regarding PEM/DER, right now there is no PEM/DER format for these keys. The only format that is in a final standard is the "raw" format, which is defined in NIST spec FIPS 204 § 7.2. This is what I believe BoringSSL uses today, and that is roughly analogous to the "raw" representation Crypto uses for EC keys.

The most likely source of a spec for DER formatted keys is going to be draft-ietf-lamps-dilithium-certificates. This has defined a tentative private key format and profile for an SPKI public key as well as matching signature format. However, this is non-final, so I don't think we need to implement them at this time. We can stick with the raw representation until the IETF is closer to an answer, or until there's an interoperability need.

If there does become an interoperability need, we can use swift-asn1 to provide the representation, as we do for the EC keys in Crypto.

Sources/_CryptoExtras/MLDSA/MLDSA_boring.swift Outdated Show resolved Hide resolved
Sources/_CryptoExtras/MLDSA/MLDSA_boring.swift Outdated Show resolved Hide resolved
Sources/_CryptoExtras/MLDSA/MLDSA_boring.swift Outdated Show resolved Hide resolved
Sources/_CryptoExtras/MLDSA/MLDSA_boring.swift Outdated Show resolved Hide resolved
Sources/_CryptoExtras/MLDSA/MLDSA_boring.swift Outdated Show resolved Hide resolved
func signature(for data: some DataProtocol, context: [UInt8]? = nil) throws -> Signature {
let output = try Array<UInt8>(unsafeUninitializedCapacity: Signature.bytesCount) { bufferPtr, length in
let result = data.regions.first!.withUnsafeBytes { dataPtr in
if let context {
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be nice to hide this branch in a helper function, something like:

extension Optional where Wrapped == ContiguousBytes {
    func withUnsafeBytes<ReturnValue>(_ body: (UnsafeRawBufferPointer) throws -> ReturnValue) rethrows -> ReturnValue {
        if let self {
            return try self.withUnsafeBytes { try body($0) }
        } else {
            return try body(UnsafeRawBufferPointer(start: nil, count: 0))
        }
    }
}

This lets us get a single call to MLDSA65_sign which makes this code nicer to follow and ensures we don't mix up arguments.

Copy link
Contributor Author

@fpseverino fpseverino Oct 18, 2024

Choose a reason for hiding this comment

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

Would this be ok? (Also using your withUnsafeBytes function)

context.map { Data($0) }.withUnsafeBytes { contextPtr in
    CCryptoBoringSSL_MLDSA65_verify(
        self.pointer,
        signaturePtr.baseAddress,
        signaturePtr.count,
        dataPtr.baseAddress,
        dataPtr.count,
        contextPtr.baseAddress,
        contextPtr.count
    )
}

Sources/_CryptoExtras/MLDSA/MLDSA_boring.swift Outdated Show resolved Hide resolved
static let bytesCount = Backing.bytesCount

fileprivate final class Backing {
let pointer: UnsafeMutablePointer<MLDSA65_private_key>
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be private. It's not acceptable for us to access the raw pointer directly, we need to use a with method to do it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So should I add a method to PrivateKey.Backing like this one?

func withUnsafePointer<T>(_ body: (UnsafePointer<MLDSA65_private_key>) throws -> T) rethrows -> T {
    try body(self.pointer)
}

Sources/_CryptoExtras/MLDSA/MLDSA_boring.swift Outdated Show resolved Hide resolved
@fpseverino fpseverino requested a review from Lukasa October 18, 2024 20:36
Sources/_CryptoExtras/MLDSA/MLDSA_boring.swift Outdated Show resolved Hide resolved

self.pointer = UnsafeMutablePointer<MLDSA65_private_key>.allocate(capacity: 1)

try rawRepresentation.regions.flatMap { $0 }.withUnsafeBufferPointer { buffer in
Copy link
Contributor

Choose a reason for hiding this comment

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

We can use the rawRepresentation.regions.count == 1 dance here too, instead of the flatMap.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I rewrite it like

let bytes: ContiguousBytes = rawRepresentation.regions.count == 1 ? rawRepresentation.regions.first! : Array(rawRepresentation)
try bytes.withUnsafeBytes { buffer in
    var cbs = CBS(data: buffer.baseAddress, len: buffer.count)
    guard CCryptoBoringSSL_MLDSA65_parse_private_key(self.pointer, &cbs) == 1 else {
        throw CryptoKitError.internalBoringSSLError()
    }
}

an error on the CBS init appears, on buffer.baseAddress: Cannot convert value of type 'UnsafeRawPointer?' to expected argument type 'UnsafePointer<UInt8>?' because initializer 'init(data:len:)' was not imported from C header.

Changing it to CBS(data: buffer.baseAddress?.assumingMemoryBound(to: UInt8.self), len: buffer.count) seems to work, is it fine?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is not, but we don't need it. We want to use a withMemoryRebound(to:) instead. This dance should probably also be wrapped up in a helper function to avoid distracting in the flow here.

Sources/_CryptoExtras/MLDSA/MLDSA_boring.swift Outdated Show resolved Hide resolved
Sources/_CryptoExtras/MLDSA/MLDSA_boring.swift Outdated Show resolved Hide resolved
Sources/_CryptoExtras/MLDSA/MLDSA_boring.swift Outdated Show resolved Hide resolved
Sources/_CryptoExtras/MLDSA/MLDSA_boring.swift Outdated Show resolved Hide resolved
Sources/_CryptoExtras/MLDSA/MLDSA_boring.swift Outdated Show resolved Hide resolved

/// A lattice-based digital signature algorithm that provides security against quantum computing attacks.
/// A module lattice-based digital signature algorithm that provides security against quantum computing attacks.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want module here?

Copy link
Contributor Author

@fpseverino fpseverino Oct 30, 2024

Choose a reason for hiding this comment

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

I added it just to complete the ML-DSA acronym, maybe I could add an hyphen between it and lattice to make it clearer, as in the FIPS title

self.pointer
) == 1 else {
throw CryptoKitError.internalBoringSSLError()
(self.key, self.seed) = try withUnsafeTemporaryAllocation(of: MLDSA65_private_key.self, capacity: 1) { privateKeyPtr in
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need an unsafeTemporaryAllocation. We can just zero-initialize the value self.key = .init() and then take a pointer to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. I also tried to remove the seed's unsafeTemporaryAllocation by passing to the BoringSSL function an empty [UInt8] var and then initializing the Data object from it, but I encountered some seg faults in testing

Sources/_CryptoExtras/MLDSA/MLDSA_boring.swift Outdated Show resolved Hide resolved
Sources/_CryptoExtras/MLDSA/MLDSA_boring.swift Outdated Show resolved Hide resolved
@fpseverino fpseverino requested a review from Lukasa October 30, 2024 16:41
Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

Cool, looking really good here. One note in the diff.


guard CCryptoBoringSSL_MLDSA65_private_key_from_seed(
&self.key,
Array(seed.prefix(MLDSA.seedSizeInBytes)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not create an array here, nor should we use prefix. We know the seed size is right, and we can use self.seed.withUnsafeBytes to get the interior pointer there.

@fpseverino fpseverino requested a review from Lukasa November 8, 2024 13:15
Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

Great, I think this is in good shape. Let's get it merged!

@Lukasa
Copy link
Contributor

Lukasa commented Nov 8, 2024

Just a few CI jobs to fix up. Missing license headers, and a need to run the cmake script in the scripts directory.

@fpseverino
Copy link
Contributor Author

The license headers are there, but I think the CI wants a CONTRIBUTORS.txt, whereas in this repo there is a CONTRIBUTORS.md

@fpseverino
Copy link
Contributor Author

Regarding the CMake script, if I run sh scripts/update_cmakelists.sh it removes all the libraries string from the CMakeLists.txt files. I suppose that's not the intended outcome.

Here's the output:

Finding source files (*.c *.swift) under /Users/francescopaoloseverino/Documents/GitHub/swift-crypto/Sources/CCryptoBoringSSL
scripts/update_cmakelists.sh: line 58: gfind: command not found

Updated /Users/francescopaoloseverino/Documents/GitHub/swift-crypto/Sources/CCryptoBoringSSL/CMakeLists.txt
Finding source files (*.c *.swift) under /Users/francescopaoloseverino/Documents/GitHub/swift-crypto/Sources/CCryptoBoringSSLShims
scripts/update_cmakelists.sh: line 58: gfind: command not found

Updated /Users/francescopaoloseverino/Documents/GitHub/swift-crypto/Sources/CCryptoBoringSSLShims/CMakeLists.txt
Finding source files (*.c *.swift) under /Users/francescopaoloseverino/Documents/GitHub/swift-crypto/Sources/CryptoBoringWrapper
scripts/update_cmakelists.sh: line 58: gfind: command not found

Updated /Users/francescopaoloseverino/Documents/GitHub/swift-crypto/Sources/CryptoBoringWrapper/CMakeLists.txt
Finding source files (*.c *.swift) under /Users/francescopaoloseverino/Documents/GitHub/swift-crypto/Sources/Crypto
scripts/update_cmakelists.sh: line 58: gfind: command not found

Updated /Users/francescopaoloseverino/Documents/GitHub/swift-crypto/Sources/Crypto/CMakeLists.txt
Finding source files (*.c *.swift) under /Users/francescopaoloseverino/Documents/GitHub/swift-crypto/Sources/_CryptoExtras
Excluding source paths (*/AES/*.swift) under /Users/francescopaoloseverino/Documents/GitHub/swift-crypto/Sources/_CryptoExtras
scripts/update_cmakelists.sh: line 58: gfind: command not found

Updated /Users/francescopaoloseverino/Documents/GitHub/swift-crypto/Sources/_CryptoExtras/CMakeLists.txt
Finding assembly files (.S) under /Users/francescopaoloseverino/Documents/GitHub/swift-crypto/Sources/CCryptoBoringSSL
scripts/update_cmakelists.sh: line 72: gfind: command not found
scripts/update_cmakelists.sh: line 73: gfind: command not found
scripts/update_cmakelists.sh: line 74: gfind: command not found
scripts/update_cmakelists.sh: line 75: gfind: command not found




Updated /Users/francescopaoloseverino/Documents/GitHub/swift-crypto/Sources/CCryptoBoringSSL/CMakeLists.txt

P.S. I do have the find command installed on my computer

@Lukasa
Copy link
Contributor

Lukasa commented Nov 20, 2024

We expect GNU find. You can get it by running brew install findutils

@fpseverino
Copy link
Contributor Author

@Lukasa all done, now the jobs should pass

@fpseverino
Copy link
Contributor Author

fpseverino commented Nov 27, 2024

@Lukasa I saw that the BoringSSL team added another parameter set to ML-KEM. Considering that they might do this for ML-DSA too, to avoid breaking the API, should we put the current implementation inside a namespace that identifies the parameter set?
If so, I'm open to suggestions for names, as the following (which are the most obvious) are not valid.

enum MLDSA {
    enum 65 {
        // Implementation proposed in this PR
    }
    
    // Possible future ones
    enum 44 {}
    enum 87 {}
}

P.S. Each parameter set uses completely different C++ functions and structures

EDIT: As suggested by Cory I went with MLDSA65.PrivateKey etc.

@fpseverino fpseverino marked this pull request as draft December 4, 2024 15:47
@fpseverino fpseverino marked this pull request as ready for review December 12, 2024 14:07
@fpseverino
Copy link
Contributor Author

Hi @Lukasa,
Thanks again for reviewing and approving this PR!
Just checking if it’s possible to get this merged when you have a moment.
Once this is merged, it would be great to start looking at reviewing #278 and #314.
Let me know if there’s anything else I need to do 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🆕 semver/minor Adds new public API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants