yenom / BitcoinKit

Bitcoin protocol toolkit for Swift
MIT License
841 stars 261 forks source link

`sign` functions does not enforce hashing #194

Open Sajjon opened 5 years ago

Sajjon commented 5 years ago

Current behavior

BitcoinKit built using Carthage:

verifySignature fails ~ 10% of the times. This seems troublesome.

Expected behavior

I expect a signature to always validate.

Steps to reproduce

Just create a Xcode project, and create a Unit Test file and paste this code. I'm using this in a Cocoa Touch Framework.

import Foundation
import BitcoinKit
import XCTest

class PrivateKeyTests: XCTestCase {

    func testPerformanceOfBitcoinKitSignAndVerify() {
        let seed = BitcoinKit.Mnemonic.seed(mnemonic: expected.seedWords)
        let wallet = BitcoinKit.HDWallet(seed: seed, network: expected.network)
        let privateKey: BitcoinKit.PrivateKey
        let publicKey: BitcoinKit.PublicKey
        do {
            privateKey = try wallet.privateKey(index: expected.hdWalletIndex)
            publicKey = try wallet.publicKey(index: expected.hdWalletIndex)
            let publicKeyString = publicKey.description
            XCTAssertEqual(publicKeyString, expected.publicKey)
        } catch {
            return XCTFail("Key generation should not have throwed error: \(error)")
        }
        measure {
            do {
                let message = "Hello BitcoinKit".data(using: .utf8)!
                let signatureData = try BitcoinKit.Crypto.sign(message, privateKey: privateKey)
                // 10 % of the times the verification fails. This is not good.
                XCTAssertTrue(try BitcoinKit.Crypto.verifySignature(signatureData, message: message, publicKey: publicKey.raw))
            } catch {
                return XCTFail("Key generation should not have throwed error: \(error)")
            }
        }

    }
}

private let expected = (
    seedWords: ["economy", "clinic", "damage", "energy", "settle", "lady", "crumble", "crack", "valley", "blast", "hair", "double", "cute", "gather", "deer", "smooth", "finish", "ethics", "sauce", "raw", "novel", "hospital", "twice", "actual"],
    network: BitcoinKit.Network.mainnet,
    hdWalletIndex: UInt32(0),
    wif: "L5TEfWGHHUVyt16MbiZiQmANrz8zEaKW2EBKiZRfZ8hbEcy2ymGd",
    publicKey: "030cd0e6d332de86d3c964cac7e98c6b1330154433a8f8aa82a93cbc7a1bfdd45b"
)

Environment

Sajjon commented 5 years ago

I think the signature and verify works if I take the sha256 hash of the data of the message first.

So changing: let message = "Hello BitcoinKit".data(using: .utf8)!

to:

Crypto.sha256("Hello BitcoinKit".data(using: .utf8)!)

But this was not documented. Also it might be a good idea to change: public static func sign(_ data: Data, privateKey: BitcoinKit.PrivateKey) throws -> Data to

public static func sign(hashedData: Data, privateKey: PrivateKey) throws -> Data {
    guard hashedData.count == 32 else {
        throw Error.dataShouldHaveLengthOf32
    }
....
}

and add a method:

public static func sign(message: String, encoding: String.Encoding = .utf8, privateKey: PrivateKey) throws -> Data {
    guard let encoded = message.data(using: encoding) else {
         throw Error.failedToEncodeMessage
    }
    return sign(hashedData: Crypto.sha256(encoded), privateKey: privateKey)
}