BitcoinKit icon indicating copy to clipboard operation
BitcoinKit copied to clipboard

`sign` functions does not enforce hashing

Open Sajjon opened this issue 6 years ago • 1 comments

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

  • macOS Mojave 10.14.2 (18C54)
  • BitcoinKit: 1.0.2
  • Xcode Version: 10.1 (10B61)
  • Swift Version: 4.2

Sajjon avatar Jan 14 '19 21:01 Sajjon

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)
}

Sajjon avatar Jan 15 '19 11:01 Sajjon