Improve ECDSA Signing on iOS #22

pelle · 2018-11-17T23:48:03Z

I've rewritten and refactored the signing code.

There is a testing function in hubSaga.js that can be enabled by uncommenting line 300

This now works 100% of the time. At least I've tested it up to 10,000 signatures.

pelle · 2018-11-17T23:53:21Z

ios/uPortMobile/Classes/EthereumSigner.m

+}
+
+
+NSDictionary *genericSignature(BTCKey *keypair, NSData *hash, BOOL lowS) {


@christianlundkvist @mirceanis This code occasionally creates an invalid signature. It's something like 1 in a 1000 (unscientifically).

Can you guys please look at the maths if there is something wrong I'm doing.

Should just comment that the previous code was seemingly worse than this.

@pelle do you have an example of invalid signature?

meaning-matters · 2018-11-19T09:01:35Z

ios/uPortMobile/Classes/EthereumSigner.m

+#include <openssl/obj_mac.h>
+
+static int BTCRegenerateKey(EC_KEY *eckey, BIGNUM *priv_key) {
+  BN_CTX *ctx = NULL;


I suggest using 4-space indentation (like all other iOS code), not 2.

meaning-matters · 2018-11-19T09:01:49Z

ios/uPortMobile/Classes/EthereumSigner.m

+             @"recoveryParam": @(recoveryParam)
+             };
+  }
+  return @{};


Just saving work. This still doesn't fix it

I've gone through and compared it with other implementations. There still seems to be a few very occasional cases where it breaks.

Signatures seem to be valid now

meaning-matters · 2018-11-21T10:19:53Z

ios/uPortMobile/Classes/EthereumSigner.m

+#include <openssl/obj_mac.h>
+
+NSMutableData *compressedPublicKey(EC_KEY *key) {
+  if (!key) return nil;


Please indent all changed/added code by 4 spaces.

meaning-matters · 2018-11-21T10:20:17Z

ios/uPortMobile/Classes/EthereumSigner.m

+
+  NSDictionary *signatureDictionary = @{ @"v": @(base + [sig[@"recoveryParam"] intValue]),
+                                         @"r": [rData base64EncodedStringWithOptions:0],
+                                         @"s":[sData base64EncodedStringWithOptions:0]};


Add spaces before & after the : and before the closing } =>

NSDictionary *signatureDictionary = @{ @"v" : @(base + [sig[@"recoveryParam"] intValue]), @"r" : [rData base64EncodedStringWithOptions:0], @"s" : [sData base64EncodedStringWithOptions:0] };

meaning-matters · 2018-11-21T10:27:37Z

ios/uPortMobile/Classes/EthereumSigner.m

+    // K was infinity and does not work, redo it
+    if ([K isInfinity])  return NULL;
+    BTCMutableBigNumber* Kx = [K.x mutableCopy];
+    [Kx mod: n];


Remove space before n => [Kx mod:n];

meaning-matters · 2018-11-21T10:28:54Z

ios/uPortMobile/Classes/EthereumSigner.m

+    BTCBigNumber* hashBN = [[BTCBigNumber alloc] initWithUnsignedBigEndian:hash];
+
+    // Compute s = (k^-1)*(h + Kx*privkey)
+


Assuming above comment belongs to line below: Remove empty line.

meaning-matters · 2018-11-21T10:30:00Z

ios/uPortMobile/Classes/EthereumSigner.m

+             @"r": rData,
+             @"s": sData,
+             @"recoveryParam": @(recoveryParam)
+             };


Format as other literal =>

return @{ @"r" : rData, @"s" : sData, @"recoveryParam" : @(recoveryParam) };

Would also fit on one line, but it's nice/more-clear like this.

meaning-matters · 2018-11-21T10:33:06Z

ios/uPortMobile/Classes/EthereumSigner.m

+
+    // Compute s = (k^-1)*(h + Kx*privkey)
+
+    BTCMutableBigNumber* signatureBN = [[[[privkeyBN mutableCopy] multiply:Kx mod:n] add:hashBN mod: n] multiply:[k inverseMod:n] mod:n];


Remove space between : and n => [[[[privkeyBN mutableCopy] multiply:Kx mod:n] add:hashBN mod:n] multiply:[k inverseMod:n] mod:n];

meaning-matters · 2018-11-21T10:34:28Z

ios/uPortMobile/Classes/EthereumSigner.m

+    [K clear];
+    [Kx clear];
+
+    BTCBigNumber *twiceS = [[signatureBN mutableCopy] add: signatureBN];


Remove space after : => [[signatureBN mutableCopy] add:signatureBN];

meaning-matters · 2018-11-21T10:34:45Z

ios/uPortMobile/Classes/EthereumSigner.m

+    BTCBigNumber *s;
+    if (lowS && BN_cmp(twiceS.BIGNUM, n.BIGNUM) > 0) {
+      // enforce low S values, by negating the value (modulo the order) if above order/2.
+      s = [[n mutableCopy] subtract: signatureBN]; // Maybe this should be swapped


Remove space after :

meaning-matters · 2018-11-21T10:37:56Z

ios/uPortMobile/Classes/EthereumSigner.m

+  memset(sigBytes, 0, 64);
+
+  memcpy(sigBytes, rData.bytes, 32);
+  memcpy(sigBytes+32, sData.bytes, 32);


Add spaces around +

meaning-matters · 2018-11-21T10:39:40Z

ios/uPortMobile/Classes/EthereumSigner.m

+  int n = 0;
+  int i = recid / 2;
+
+  const EC_GROUP *group = EC_KEY_get0_group(eckey);


In lines below, add spaces around ='s

I'll leave this bit as is, because it's copied verbatim from openssl source. So it's easier to update. Actually, while it's ugly I'll comment it out. It's not needed here, but may be needed by the sdk.

meaning-matters · 2018-11-21T10:44:11Z

ios/uPortMobile/Classes/UPTEthSigner.m

+        if (signature) {
+            result(signature, nil);
+        } else {
+            NSError *signingError = [[NSError alloc] initWithDomain:@"UPTError" code:UPTSignerErrorCodeLevelSigningFailed.integerValue userInfo:@{@"message": @"signing failed due to invalid values"}];


I think it's good to keep lines shorter than 120 characters. Please break up this line, and use same dictionary literal format as commented on above =>

NSError *signingError; signingError = [[NSError alloc] initWithDomain:@"UPTError" code:UPTSignerErrorCodeLevelSigningFailed.integerValue userInfo:@{ @"message" : @"signing failed due to invalid values" }];

meaning-matters · 2018-11-21T10:45:49Z

ios/uPortMobile/Classes/UPTEthSigner.m

+        if (signature) {
+            result(signature, nil);
+        } else {
+            NSError *signingError = [[NSError alloc] initWithDomain:@"UPTError" code:UPTSignerErrorCodeLevelSigningFailed.integerValue userInfo:@{@"message": @"signing failed due to invalid values"}];


Break up in 4 lines, as above, to keep lines < 120 chars.

And again use same dictionary literal format => @{ @"message" : @"signing failed due to invalid values" }

meaning-matters · 2018-11-21T10:47:28Z

ios/uPortMobile/Classes/UPTHDSigner.m

+      callback(signature, nil);
+    } else {
+      NSError *signingError = [[NSError alloc] initWithDomain:@"UPTHDSignerError" code:UPTHDSignerErrorCodeLevelSigningFailed.integerValue userInfo:@{@"message": [NSString stringWithFormat:@"signing failed due to invalid values for eth address: signJWT %@", rootAddress]}];
+      callback( nil, signingError);


Remove space after (

meaning-matters · 2018-11-21T10:51:40Z

ios/uPortMobile/Classes/EthereumSigner.h

@@ -0,0 +1,25 @@
+// Copyright (C) 2018 ConsenSys AG


Add file name to header, to stick with iOS/Xcode standard =>

// // EthereumSigner.h // uPort Mobile App // // Copyright (C) 2018 ConsenSys AG

meaning-matters · 2018-11-21T10:53:04Z

ios/uPortMobile/Classes/EthereumSigner.h

@@ -0,0 +1,25 @@
+// Copyright (C) 2018 ConsenSys AG
+//
+// This file is part of uPort Mobile App.


Could be deleted because it's now mentioned above?

meaning-matters · 2018-11-21T10:54:19Z

ios/uPortMobile/Classes/EthereumSigner.h

+//
+// This file is part of uPort Mobile App.
+//
+// uPort Mobile App is free software: you can redistribute it and/or modify


Is it correct to call this uPort Mobile App, since it's not an app but SDK instead, and I'd expect iOS in the project/module name.

meaning-matters · 2018-11-21T10:55:07Z

ios/uPortMobile/Classes/EthereumSigner.h

+//
+// This file is part of uPort Mobile App.
+//
+// uPort Mobile App is free software: you can redistribute it and/or modify


Would be nice to indent all lines below as the standard Xcode header, so adding one space before each line.

meaning-matters · 2018-11-21T10:55:51Z

ios/uPortMobile/Classes/EthereumSigner.m

@@ -0,0 +1,203 @@
+// Copyright (C) 2018 ConsenSys AG


Update file header; see .h comment above.

meaning-matters · 2018-11-21T10:59:48Z

ios/uPortMobile/Classes/EthereumSigner.m

+#include <openssl/obj_mac.h>
+
+NSMutableData *compressedPublicKey(EC_KEY *key) {
+  if (!key) return nil;


I believe it's good practice to always have the if/else/while/... code blocks as a real block between { }. Placing code on the same line makes that they can be missed easily (because it's a second way to format these statements; best is to have as little ways to format code as possible making it easier to scan by a human).

meaning-matters · 2018-11-21T11:00:09Z

ios/uPortMobile/Classes/EthereumSigner.m

+  return data;
+}
+
+


Remove line?

meaning-matters · 2018-11-21T11:00:53Z

ios/uPortMobile/Classes/EthereumSigner.m

+  return signatureDictionary;
+}
+
+


Remove line?

meaning-matters · 2018-11-21T11:02:35Z

ios/uPortMobile/Classes/EthereumSigner.m

+  if (!sig) return NULL;
+  NSData *rData = (NSData *)sig[@"r"];
+  NSData *sData = (NSData *)sig[@"s"];
+  ///////


meaning-matters · 2018-11-21T11:04:04Z

ios/uPortMobile/Classes/EthereumSigner.m

+
+NSData *simpleSignature(BTCKey *keypair, NSData *hash) {
+  NSDictionary *sig = genericSignature(keypair, hash, NO);
+  if (!sig) return NULL;


Although it's correct in C, what you're actually testing if sig is nil. So to keep close to that meaning, I think it's 'better' to write if (sig == nil) Meaning matters ;-)

meaning-matters · 2018-11-21T11:16:39Z

ios/uPortMobile/Classes/EthereumSigner.m

+  int i = recid / 2;
+
+  const EC_GROUP *group = EC_KEY_get0_group(eckey);
+  if ((ctx = BN_CTX_new()) == NULL) { ret = -1; goto err; }


It may help remove some visual clutter here to use a good old CPP macro:

#define CHECK(condition, error) { if (!(condition)) { ret = (error); goto err; } CHECK(EC_GROUP_get_order(group, order, ctx)), -1) x = BN_CTX_get(ctx); CHECK(BN_copy(x, order)), -1) CHECK(BN_mul_word(x, i)), -1)

Just an idea eh; probably best to leave code as it is.

meaning-matters · 2018-11-21T11:22:20Z

ios/uPortMobile/Classes/UPTEthSigner.m

+            result(signature, nil);
+        } else {
+            NSError *signingError = [[NSError alloc] initWithDomain:@"UPTError" code:UPTSignerErrorCodeLevelSigningFailed.integerValue userInfo:@{@"message": @"signing failed due to invalid values"}];
+            result( nil, signingError);


Remove space before nil.

meaning-matters · 2018-11-21T11:22:40Z

ios/uPortMobile/Classes/UPTEthSigner.m

+            result(signature, nil);
+        } else {
+            NSError *signingError = [[NSError alloc] initWithDomain:@"UPTError" code:UPTSignerErrorCodeLevelSigningFailed.integerValue userInfo:@{@"message": @"signing failed due to invalid values"}];
+            result( nil, signingError);


Remove space before nil.

meaning-matters · 2018-11-21T11:24:10Z

ios/uPortMobile/Classes/UPTHDSigner.m

+    if (signature) {
+        callback(signature, nil);
+    } else {
+        NSError *signingError = [[NSError alloc] initWithDomain:@"UPTHDSignerError" code:UPTHDSignerErrorCodeLevelSigningFailed.integerValue userInfo:@{@"message": [NSString stringWithFormat:@"signing failed due to invalid values for eth address: signJWT %@", rootAddress]}];


Again, please break up for line < 120

meaning-matters · 2018-11-21T11:25:24Z

ios/uPortMobile/Classes/UPTHDSigner.m

+    if (signature) {
+      callback(signature, nil);
+    } else {
+      NSError *signingError = [[NSError alloc] initWithDomain:@"UPTHDSignerError" code:UPTHDSignerErrorCodeLevelSigningFailed.integerValue userInfo:@{@"message": [NSString stringWithFormat:@"signing failed due to invalid values for eth address: signJWT %@", rootAddress]}];


Break up in multiple lines to keep line length below 120; possibly by introducing local variable(s).

meaning-matters

All I could do (at the moment, without knowing yet how to run this) is comments about formatting. I'm glad you managed to get it working. I'm curious what changes fixed things.

meaning-matters · 2018-11-21T14:09:18Z

ios/uPortMobile/Classes/EthereumSigner.m

+
+    NSDictionary *signatureDictionary = @{ @"v" : @(base + [sig[@"recoveryParam"] intValue]),
+                                           @"r" : [rData base64EncodedStringWithOptions:0],
+                                           @"s" :[sData base64EncodedStringWithOptions:0] };


Space missing after : => @"s" : [sData base64EncodedStringWithOptions:0] };

meaning-matters

Great!

(One space missing somewhere.)

pelle requested review from mirceanis and meaning-matters November 17, 2018 23:48

pelle commented Nov 17, 2018

View reviewed changes

meaning-matters reviewed Nov 19, 2018

View reviewed changes

ios/uPortMobile/Classes/EthereumSigner.m Outdated

@"recoveryParam": @(recoveryParam)

};

}

return @{};

Copy link

Contributor

meaning-matters Nov 19, 2018

Choose a reason for hiding this comment

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

Redundant.

pelle added 4 commits November 20, 2018 08:09

Restart if things are invalid

9259e63

Just saving work. This still doesn't fix it

Better generation of recoveryParam

be094e0

Improve signing code

bba9b38

I've gone through and compared it with other implementations. There still seems to be a few very occasional cases where it breaks.

remove unnecessary return

f1c240d

pelle force-pushed the feature/#162028218/force-recovery-ios branch from b8389c0 to f1c240d Compare November 20, 2018 14:20

pelle added 4 commits November 20, 2018 10:08

Testing code (to be removed after fix)

b52f46f

Rewrote several things using BTCBigNumber

71e505e

Signatures seem to be valid now

Snip/Snip/SNip

2b7f4fb

More cleanups

a13305a

meaning-matters reviewed Nov 21, 2018

View reviewed changes

ios/uPortMobile/Classes/EthereumSigner.m Outdated

return data;

}

Copy link

Contributor

meaning-matters Nov 21, 2018

Choose a reason for hiding this comment

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

Remove line?

meaning-matters reviewed Nov 21, 2018

View reviewed changes

ios/uPortMobile/Classes/EthereumSigner.m Outdated

return signatureDictionary;

}

Copy link

Contributor

meaning-matters Nov 21, 2018

Choose a reason for hiding this comment

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

Remove line?

meaning-matters reviewed Nov 21, 2018

View reviewed changes

meaning-matters suggested changes Nov 21, 2018

View reviewed changes

pelle added 2 commits November 21, 2018 07:31

Formatting fixes

87dff07

Missed a couple

64f19ac

meaning-matters reviewed Nov 21, 2018

View reviewed changes

meaning-matters approved these changes Nov 21, 2018

View reviewed changes

pelle merged commit 08f4b1a into develop Nov 21, 2018

mirceanis deleted the feature/#162028218/force-recovery-ios branch January 22, 2020 11:10

		}


		NSDictionary genericSignature(BTCKey keypair, NSData *hash, BOOL lowS) {

		BTCBigNumber* hashBN = [[BTCBigNumber alloc] initWithUnsignedBigEndian:hash];

		// Compute s = (k^-1)(h + Kxprivkey)


		// Compute s = (k^-1)(h + Kxprivkey)

		BTCMutableBigNumber* signatureBN = [[[[privkeyBN mutableCopy] multiply:Kx mod:n] add:hashBN mod: n] multiply:[k inverseMod:n] mod:n];

Improve ECDSA Signing on iOS #22

Improve ECDSA Signing on iOS #22

Conversation

pelle commented Nov 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meaning-matters Nov 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meaning-matters Nov 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meaning-matters Nov 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meaning-matters Nov 21, 2018 • edited Loading

Choose a reason for hiding this comment

meaning-matters Nov 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meaning-matters Nov 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meaning-matters Nov 21, 2018 • edited Loading

Choose a reason for hiding this comment

meaning-matters Nov 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meaning-matters left a comment • edited Loading

Choose a reason for hiding this comment

meaning-matters Nov 21, 2018 • edited Loading

Choose a reason for hiding this comment

meaning-matters left a comment

Choose a reason for hiding this comment

pelle commented Nov 17, 2018 •

edited

Loading

meaning-matters Nov 21, 2018 •

edited

Loading

meaning-matters Nov 21, 2018 •

edited

Loading

meaning-matters Nov 21, 2018 •

edited

Loading

meaning-matters Nov 21, 2018 •

edited

Loading

meaning-matters Nov 21, 2018 •

edited

Loading

meaning-matters Nov 21, 2018 •

edited

Loading

meaning-matters Nov 21, 2018 •

edited

Loading

meaning-matters Nov 21, 2018 •

edited

Loading

meaning-matters left a comment •

edited

Loading

meaning-matters Nov 21, 2018 •

edited

Loading