BlockchainCommons / GordianSeedTool-iOS

Cryptographic Seed Manager for iOS
Other
36 stars 8 forks source link

Public HD key export sets "master" flag for public key #182

Open craigraw opened 1 year ago

craigraw commented 1 year ago

As the title suggests, the ur:crypto-hdkey shared from Derive Key > Other Key Derivations > Public HD Key actually shares the private key information.

ChristopherA commented 1 year ago

Assigned to @wolfmcnally to evaluate and @ChristopherA to prioritize.

wolfmcnally commented 1 year ago

@craigraw I followed the Derive Key > Other Key Derivations > Public HD Key instructions you gave. Below is my test case and analysis:

Seed: Oxford Blue Free Fair

ur:crypto-seed/otadgdzofndnptamfmditpgssnuoladkglsbvaaosecyiaytoentaxkpgwksiyjljpiecxfwjzkpihcxfgjpihihcxfghsinjpcprnvdtk

Public key from Oxford Blue Free Fair

ur:crypto-hdkey/onadykaxhdclaozsnnfhfxadgsotjnahtlfxveonnlrerflubkrsmouybbgrsadputtdsphdlecfjlaahdcxrnstsbsosgvlampmgauedknlwdbdzoimayenmhahgrdpprqzbsnefsrfrntnbdssahtaadehoyaoadaskscxfdfygrihkkcxiyjpjljncxgwksiyjljpiecxfwjzkpihcxfgjpihihcxfghsinjpprpemdhs

a501f503582102fa9e3f43014ca36d05d543e4a599b5bc8b0abf92db144bc22dddd2c8588a196f045820bec7cbc9cae306ad49de2499ea0bfb6a083690054b2db2b40f9f3dbcbeda0bc405d90131a1020109782048444b65792066726f6d204f78666f726420426c756520467265652046616972

{
   1:
   true,
   3:
   h'02fa9e3f43014ca36d05d543e4a599b5bc8b0abf92db144bc22dddd2c8588a196f',
   4:
   h'bec7cbc9cae306ad49de2499ea0bfb6a083690054b2db2b40f9f3dbcbeda0bc4',
   5:
   305(
      {2: 1}
   ),
   9:
   "HDKey from Oxford Blue Free Fair"
}

Private key from Oxford Blue Free Fair

ur:crypto-hdkey/oladykaoykaxhdclaelrkppmhhndlksfpmnylyenfhbwghtbsphecftydtrkyndahecmiabgceoscasegdaahdcxrnstsbsosgvlampmgauedknlwdbdzoimayenmhahgrdpprqzbsnefsrfrntnbdssahtaadehoyaoadaskscxfdfygrihkkcxiyjpjljncxgwksiyjljpiecxfwjzkpihcxfgjpihihcxfghsinjpjpgojtrh

a601f502f5035821008475ad5c9b8cccad9a81363f1354d6c85f19d429bbf6255f1663121ca71dc150045820bec7cbc9cae306ad49de2499ea0bfb6a083690054b2db2b40f9f3dbcbeda0bc405d90131a1020109782048444b65792066726f6d204f78666f726420426c756520467265652046616972

{
   1:
   true,
   2:
   true,
   3:
   h'008475ad5c9b8cccad9a81363f1354d6c85f19d429bbf6255f1663121ca71dc150',
   4:
   h'bec7cbc9cae306ad49de2499ea0bfb6a083690054b2db2b40f9f3dbcbeda0bc4',
   5:
   305(
      {2: 1}
   ),
   9:
   "HDKey from Oxford Blue Free Fair"
}

Analysis

Field 1 is true for both of them, which marks them as "master", as expected. Field 2 is true for the private key, which marks it as private as expected, and omitted from the public key, which defaults to false hence marking it as public as expected. Field 3, the key data, is different for each of them, as expected. Field 4, the chain code, is the same for both of them, as expected.

Reference: HDKey CBOR Spec

Conclusion

I don't seem to be able to replicate the problem. Please let me know if there's something I'm missing here.

shannona commented 1 year ago

Testing out our on our default Yinmn Blue, I also couldn't replicate. Just like Wolf, I had different public & private keys, with only the private key marked appropriately in field 2.

If you can offer any guidance to replicate, @craigraw that'd be great!

craigraw commented 1 year ago

Apologies, I was incorrect - ur:crypto-hdkey does not share the private key.

However, I think there is still a problem. The UR is encoded with the master field as true, however the specification states that:

; A master key is always private, has no use or derivation information,
; and always includes a chain code.

Therefore hummingbird was creating an object that represented a private key - I believe correctly?

wolfmcnally commented 1 year ago

I'm a bit confused: are you saying there is a problem with how SeedTool is encoding non-master URs?

craigraw commented 1 year ago

Yes - specifically that a public key is encoding with the master field set to true, in contravention of the spec.

wolfmcnally commented 1 year ago

Thanks for calling this to my attention. I'm flagging this issue as a bug.

craigraw commented 1 year ago

Should this be fixed? I tested using GST 1.6 (66) and found the public key is still being shared with the master field set to true.

shannona commented 5 months ago

This seems irrelevent as this is now shared as a ur:envelope rather than a ur:crypto-hdkey.

This is the Public HD Key from Yinmn Blue, derived from a Master Key:

ur:envelope/lntpcshdclaomucnprrerhldtidtkokkoszogolatlpkdintpfdkgygtbwzckigtmerpcpatstgsoycfaddwlfcfaddpoycfadmhcfadmeoycfadyktpcshdcxvdbggasekkuytykkfefdlahtflkecpbzyackrpfmghvyqdssfsfhmtzcttsehdcsoyadcssgoyadcfadwkoybdtpcskscxfdfygrihkkcxiyjpjljncxfyhsjpjecxgdkpjpjojzihcxgdihiajecxhfinhsjzlrolwsts

Envelope describes it as such:

Bytes(33) [
    isA: BIP32Key
    isA: PublicKey
    asset: BTC [
        network: MainNet
    ]
    chainCode: Bytes(32)
    hasName: "HDKey from Dark Purple Peck Vial"
]

Transforming the Envelope's bytewords to hex results in this:

86d8185821029323b2b5b989d0297679a7fb5580d5aa279db024514d13fd7d4d91b62207c74ca119012c8219012da1190190190191a11901f5d8185820e71249c179dbd4794548805a477c2215f81eb63e54e1b3c43d3f96fdd1c15818a10118caa1011901f4a10bd818782048444b65792066726f6d204461726b20507572706c65205065636b205669616c

Which cbor2diag reads as follows:

[24(h'029323b2b5b989d0297679a7fb5580d5aa279db024514d13fd7d4d91b62207c74c'), {300: [301, {400: 401}]}, {501: 24(h'e71249c179dbd4794548805a477c2215f81eb63e54e1b3c43d3f96fdd1c15818')}, {1: 202}, {1: 500}, {11: 24("HDKey from Dark Purple Peck Vial")}]

Any other thoughts, @craigraw ? (We're now at 74.)

craigraw commented 5 months ago

I guess if not relevant anymore, it can be closed. Fwiw I have missed the ability to test certain URs in Seed Tool.

ChristopherA commented 5 months ago

I have missed the ability to test certain URs in Seed Tool.

Which ones? We can always enable some when "Developer Mode" is turned on in settings. Let us know.