New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Look for cyptocurrency (Bitcoin) private keys #104

Merged

s-rah merged 1 commit into s-rah:onionscan-0.3 from gchan:onionscan-0.2

Nov 1, 2016

gchan commented Oct 30, 2016 •

edited

I have started my attempt at addressing #76 (looking for cryptocurrency private keys). This is my first GoLang PR for an OS project and I welcome any feedback and assistance.

There is a few things I'm unsure of in my approach:

Should I be creating a new type and functions for handling private keys (37 bytes vs 25 bytes for addresses)
My implementation is based on the format described here but in bitcoin.html on line 91 the private key begins with an 'L'. When using this private key validator, it it unable to validate this private key. Even when I remove the 'L' character, it does not pass the checksum tests.

s-rah reviewed

View reviewed changes

Owner

s-rah left a comment •

edited

Thanks, this is looking good - a few comments attached.

Should I be creating a new type and functions for handling private keys (37 bytes vs 25 bytes for addresses)

I think so. These are different enough to warrant new code. Though at some point we should pribably abstract our the base58 function.

My implementation is based on the format described here but in bitcoin.html on line 91 the private key begins with an 'L'. When using this private key validator, it it unable to validate this private key. Even when I remove the 'L' character, it does not pass the checksum tests.

I believe this is just an artifact - pinging @laanwj to confirm :)

deanonymization/check_bitcoin_addresses.go

+              // Errors are returned if the argument is not valid base58 or if the decoded
+              // value does not fit in the 37 byte private key.  The private key is not otherwise
+              // checked for validity.
+              func (a *P37) Set58(s []byte) error {

Owner

s-rah Oct 30, 2016

it's good practice to call the label the variable after the type in this case a *P37 should be p * P37

deanonymization/check_bitcoin_addresses.go

+              	return nil
+              }
+              // ValidA58 validates a base58 encoded private key.  An private key is valid

Owner

s-rah Oct 30, 2016

ValidateP58

deanonymization/check_bitcoin_addresses.go

    
              // ExtractBitcoinAddress extracts any information related to bitcoin addresses from the current crawl.

              func ExtractBitcoinAddress(osreport *report.OnionScanReport, anonreport *report.AnonymityReport, osc *config.OnionScanConfig) {

              	bcaregex := regexp.MustCompile(`[13][a-km-zA-HJ-NP-Z1-9]{25,34}`)

              	bcaregex := regexp.MustCompile(`[135][a-km-zA-HJ-NP-Z1-9]{25,51}`)

Owner

s-rah Oct 30, 2016

I think we should do this as two separate regex's.

deanonymization/check_bitcoin_addresses.go

               				if ValidA58([]byte(result)) {
               					anonreport.BitcoinAddresses = append(anonreport.BitcoinAddresses, result)
               					osc.Database.InsertRelationship(osreport.HiddenService, "snapshot", "bitcoin-address", result)
+              				} else if ValidP58([]byte(result)) {
+              					anonreport.BitcoinAddresses = append(anonreport.BitcoinAddresses, result)

Owner

s-rah Oct 30, 2016

We should define a new field in AnonymityReport for BitcoinPrivateKeys, these don't belong in BitcoinAddresses

deanonymization/check_bitcoin_addresses.go

               				if ValidA58([]byte(result)) {
               					anonreport.BitcoinAddresses = append(anonreport.BitcoinAddresses, result)
               					osc.Database.InsertRelationship(osreport.HiddenService, "snapshot", "bitcoin-address", result)
+              				} else if ValidP58([]byte(result)) {
+              					anonreport.BitcoinAddresses = append(anonreport.BitcoinAddresses, result)
+              					osc.Database.InsertRelationship(osreport.HiddenService, "snapshot", "bitcoin-private-key", result)

Owner

s-rah Oct 30, 2016

Can you also add a pretty print option for bitcoin-private-key in webgui?

Contributor

laanwj commented Oct 30, 2016 •

edited

I believe this is just an artifact - pinging @laanwj to confirm :)

I may have screwed up there. When in doubt I generally use python-bitcoinlib to check:

>>> from bitcoin import wallet
>>> key=wallet.CBitcoinSecret('L52r4HGHVKCUAtVxEhwvtSRakbLyDLRzCnWxm41mF2ceoPcd4zdY')
>>> key
CBitcoinSecret('L52r4HGHVKCUAtVxEhwvtSRakbLyDLRzCnWxm41mF2ceoPcd4zdY')
>>> key.nVersion
128
>>> binascii.b2a_hex(key)
b'e91c3515a558a7d11210534886ac73d2e007b628ef31e8dd5013e86be2e7d5af01'
>>> wallet.P2PKHBitcoinAddress.from_pubkey(key.pub)
P2PKHBitcoinAddress('1E2yYxpLeoauVFppDj4R5QTsx4NApbnSxs')

Seems like a typical mainnet private key (prefix 128), and it works out to the address mentioned in bitcoin.html. I think I used stock bitcoind to generate it. Also that wiki page confirms they can start with L or K:

Private keys associated with compressed public keys are 52 characters and start with a capital L or K on mainnet (c on testnet).

I suspect that private key validator doesn't handle compressed keys, which are the most common type of key these days (their private data is 33 bytes - 32 bytes key data | 0x01).

gchan force-pushed the onionscan-0.2 branch 2 times, most recently from 623fb4a to aec295b Compare

October 31, 2016 10:50

Author

gchan commented Oct 31, 2016

Thanks for the information @laanwj . It appears the mainnet keys do begin with a prefix. I have adjusted my implementation and completed the checksum validation.

@s-rah, I believe I have addressed all of your feedback in my latest changed. I also decided to move away from the using the structs to abstracted functions. WDYT?

Owner

s-rah commented Nov 1, 2016

Thanks, This looks good to me. @gchan - can you rebase onto the onionscan-0.3 branch please - that has some extra testing/coverage checks and will be where this commit ends up until we finalize the 0.3 release.

s-rah added the enhancement label

s-rah added this to the OnionScan 0.3 milestone

gchan force-pushed the onionscan-0.2 branch from 31c317b to e11b776 Compare

November 1, 2016 05:49

gchan changed the base branch from onionscan-0.2 to onionscan-0.3

November 1, 2016 05:50

gchan force-pushed the onionscan-0.2 branch from e11b776 to cc9f078 Compare

November 1, 2016 05:51

gchan changed the title ~~Look for cyptocurrency private keys WIP #76~~ Look for cyptocurrency (Bitcoin) private keys

coveralls commented Nov 1, 2016 •

edited

Coverage increased (+0.9%) to 26.288% when pulling cc9f078 on gchan:onionscan-0.2 into d48ca1b on s-rah:onionscan-0.3.

gchan force-pushed the onionscan-0.2 branch from cc9f078 to edad49f Compare

November 1, 2016 05:55

coveralls commented Nov 1, 2016 •

edited

Coverage increased (+0.9%) to 26.288% when pulling cc9f078 on gchan:onionscan-0.2 into d48ca1b on s-rah:onionscan-0.3.

coveralls commented Nov 1, 2016 •

edited

Coverage increased (+0.9%) to 26.288% when pulling edad49f on gchan:onionscan-0.2 into d48ca1b on s-rah:onionscan-0.3.


          Look for cyptocurrency (Bitcoin) private keys s-rah#76

f5f8226

gchan force-pushed the onionscan-0.2 branch from edad49f to f5f8226 Compare

November 1, 2016 06:01

Author

gchan commented Nov 1, 2016

I've rebased the branch on onionscan-0.3. I also changed the base branch to the same.

Oops, I didn't create branch a when working on this and I can't change the branch on GitHub without creating a new PR. I don't think it should matter for our purposes (it will just say I want to merge from gchan:onionscan-0.2).

coveralls commented Nov 1, 2016 •

edited

Coverage increased (+0.9%) to 26.307% when pulling f5f8226 on gchan:onionscan-0.2 into d48ca1b on s-rah:onionscan-0.3.

s-rah merged commit 751cc8a into s-rah:onionscan-0.3

Owner

s-rah commented Nov 1, 2016

Woohoo! & Thanks! I'll take the +0.9% code coverage increase any day.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment