Is it possible to generate an invalid PrivateKey when no secret it passed?

[charlie632]

If I don't pass a secret to secp256k1.Signing.PrivateKey I get a random one. Checking the code to see its implementation, I see that it may be possible to generate an invalid key?

https://github.com/GigaBitcoin/secp256k1.swift/blob/a69bc11b381207c1a774b8f1fb0de6e30c4f6490/Sources/zkp/secp256k1.swift#L154C1-L154C1

This will calculate a safe random number of 32 bytes, but I believe a couple of values may be invalid (see https://crypto.stackexchange.com/a/30272).

Not sure if down the line there are some checks to check the validity of this value or if when generating the PublicKey (in the same constructor) it will throw an error due to this reason.

[csjones]

Hey @charlie632 👋

Are you referring to using secp256k1_ec_seckey_verify to check that the PrivateKey is valid?

If you have a specific example using a 32 byte key, we can create a unit test for this case.

[charlie632]

Hey @csjones! Yeah, If the SecureBytes() function returns a value bigger than:

0xfffffffffffffffffffffffffffffffebaaedce6af48a03bbfd25e8cd0364142 the secp256k1_ec_seckey_verify inside the PublicKeyImplementation will raise.

Here I created a small test scenario:

   func testPrivateKeyWithInvalidKey() throws {
        let invalidPrivateKey = "0xfffffffffffffffffffffffffffffffebaaedce6af48a03bbfd25e8cd0364142"
        
        let privKeyBytes = [UInt8](Data(hexadecimalString: invalidPrivateKey)!)

        let context = secp256k1.Context.rawRepresentation
        
        
        let isValid = secp256k1_ec_seckey_verify(context, privKeyBytes) > 0
        
        XCTAssert(!isValid)
    }

If you change the priv key hex to 0xfffffffffffffffffffffffffffffffebaaedce6af48a03bbfd25e8cd0364140 the test will fail.

-- Forgot to add the extension used to transform the hex string to Data

extension Data {
    init?(hexadecimalString: String) {
        let cleanString: String
        if hexadecimalString.hasPrefix("0x") {
            cleanString = String(hexadecimalString.dropFirst(2))
        } else {
            cleanString = hexadecimalString
        }

        guard cleanString.count % 2 == 0 else { return nil }

        var bytes = [UInt8]()
        var startIndex = cleanString.startIndex
        while startIndex < cleanString.endIndex {
            let endIndex = cleanString.index(startIndex, offsetBy: 2)
            let hexSubstring = cleanString[startIndex..<endIndex]

            if let byte = UInt8(hexSubstring, radix: 16) {
                bytes.append(byte)
            } else {
                return nil
            }

            startIndex = endIndex
        }

        self.init(bytes: bytes)
    }
}

[charlie632]

So, maybe in the initialization following the SecureBytes call, there could be a small check with a loop until it gets a valid sec_key

[csjones]

It might be simpler if we tried testing like this:

let privateBytes = try! "fffffffffffffffffffffffffffffffebaaedce6af48a03bbfd25e8cd0364142".bytes
XCTAssertThrowsError(try secp256k1.Signing.PrivateKey(dataRepresentation: privateBytes))

We can use XCTAssertThrowsError to make sure an error is thrown which indicates a failure. Throwing an error should prevent the initialization of secp256k1.Signing.PrivateKey with invalid keys. This also uses the existing String extension bytes to convert a hex String to Data.

[charlie632]

yup, that is way simpler!.

However, I think the issue still remains where if you don't pass a dataRepresentation to the initializer (meaning, you want to generate a random private key), you might get an error.

In my code, I did this just to be safe:

            while true {
                guard let privateKey = try? secp256k1.Signing.PrivateKey() else {
                    continue
                }
                return privateKey
            }

Not sure if something like this can be implemented directly in the library

[csjones]

I understand what you're saying now 😅 It makes sense to have the ability to constrain the entropy when using secp256k1.Signing.PrivateKey() and this change would bring the API closer to what Apple provides with P256.Signing.PrivateKey()

I'm not sure an infinite loop is the right solution for this package (but don't let me prevent you from using it in your package 😉 ) because it should be optimal and always provide a valid key in a single pass. Looking at SecureBytes and libsecp256k1, I don't have any ideas off the top of my head how we could achieve this without looping.

I'll need to investigate this more but in the meantime share any ideas you might have because I would love to make the API closer to what Apple provides.

[csjones]

After a little investigation, I learned that Apple implements looping logic on initialization similar to what you described. I take it back and now think we can explore this change further for the package 😁

[charlie632]

Ohh, limiting the number of times it tries sounds like a good idea. Let me know if you need any help, I'm happy to help!

[csjones]

Your help would be greatly appreciated! The first of the two functions could be changed to:

@usableFromInline init(format: secp256k1.Format = .compressed) {
    for _ in 0 ..< 10 {
        let randomBytes = SecureBytes(count: secp256k1.ByteDetails.count)
        if let privateKey = try? Self.PrivateKey(dataRepresentation: Data(randomBytes), format: format) {
            self = privateKey
            return
        }
    }

    fatalError("Looped more than 10 times trying to generate a key")
}