Change for issue #3 - Added Bip38 encryption #6
Conversation
(code taken directly from the live bitaddress.org version)
|
I've just updated that with another commit to allow encrypting custom/vanity addresses as well. |
|
Guys. It's Christmas...
|
|
I'll take that as a compliment? ;) |
|
Hi artiomchi, This is a great start! Thank for being proactive. Every day or so I get a request for BIP38, so I really appreciate your efforts. Your implementation unfortunately faithfully reproduces some usability problems I have with how bitaddress.org implements BIP38. For example:
Here's how I think it should work. The reason I haven't implemented BIP38 yet is because I want it to work much better than it does with bitaddress.org. See this flow mockup: http://cl.ly/image/0e3v1M2B081X Description:
What do you think? |
|
Hmm.. I think that's a pretty good idea in terms of usability.. And it should be relatively simple to implement as well.. Do you have the mockups working in html, or were these just photoshopped? |
|
That was just Photoshop. If someone (you?) were to implement the very basic The other idea I had last night is that the introduction/instructions tab
|
|
Hmm.. I looked around the code again, and in my opinion a good way around it would be at least a partial rewrite of the Nonetheless, I've somewhat adapted it for the commits above, so I changed it a bit more :) Only thing to note is - generating a new BIP0038 address uses the password as entropy, so at no point during the generation can I see a decoded private key.. If the address displayed on screen in encoded and was randomly generated, disabling BIP0038 encoding will have to either run through a decoding procedure, or generate a new address.. Other than that - the rest is simple. I should have a commit with it pretty soon :) |
|
I've comitted the new changes.. They have the new design as you showed.. I've hosted it here, so you should be able to see it immediately: https://bitcoinpaperwallet.azurewebsites.net/ |
|
Artiom, This is exceptional! I could hug you. It's really stellar work, absolutely true to the mockups. Seeing it in action, does the workflow feel right to you? As for me, I need to play around with it some, do a fair amount of round-trip testing, etc. Initial impressions (aside from it being altogether excellent and stable): *** Number One *** I do hope there's a solution or work-around for this scenario:
Result: "No private key available. Do you want to generate a new one." I really do appreciate that you thought to pop up an alert to handle this scenario, but is there really no way we have access to the private key? What makes me curious is that of course if I take that same "no private key available" BIP38 private key and paste it into bitaddress.org along with my passphrase, it can reverse-engineer the WIF-format unencrypted key. Our wallet knows the passphrase, it knows the BIP38 key, so can we make it so that you can toggle BIP38 on and off for any given wallet, whether it was generated with the checkbox turned on or not? *** Number Two *** There are a few situations where should have some "please wait, doing BIP38 encryption/decryption" feedback. (I would always prefer a message to a spinning graphic as well.) For example:
Result: browser appears to hang with no message until key is generated. I'd like it if anytime BIP38 work is being done, a clear message is displayed especially for the benefit of people using slow laptops they have dedicated for printing wallets. It can take minutes! * Number Three * Should we make it so that the "supply your own key" feature allows you to supply either a WIF key (5_) or a BIP38 key (6_)? I guess we'd have to also let them type in the passphrase at that point... Kind of a hairy mess. It would definitely be appreciated by people who intend to sell or give away blank paper wallets to people who provide their own BIP38 keys. It could possibly also give us at least one way to let people decrypt their own BIP38 encrypted wallets, which would be a welcome feature. Anyway that's my feedback so far. Again, excellent work. With your permission what I'd like to do by way of compensation is start sharing donations that bitcoinpaperwallet receives. Please email me privately at canton@gmail.com and we'll work out a fair arrangement to compensate you for your excellent work. Thanks again!
canton@gmail.com • (505) 570-0635 • http://cantonbecker.com |
|
Nice work! Share the love Canton. Cheers from Hamburg@30C3, Levin 2013/12/27 cantonbecker notifications@github.com
|
|
Hi Canton, I'm really glad you liked what I did.. So I'll continue on it, if you don't mind :) And that's some great feedback! Really!
Finally, I know this code is based on bitaddress.org, but how close to the original do you want to keep it? I mean, bitaddress.org has a lot more options than this site uses, the wallet generation has multiple checks and switches to support generating wallets without design, to generate multiple wallets at the same time (controls that are hidden on the page) and so on.. Are you interested in keeping them, or are they left in as legacy code at the moment, and you're not interested in keeping them? If it's the latter, I could make some changes to the ninja class, that would make it more streamlined and manageable, since I've had to leverage the existing classes to do my bidding, but I don't really find the current solution elegant and to my liking, if you know what I mean :) But it's your project, and you're the boss, so I'll follow whatever path you choose Cheers, |
|
Regarding point 1. above - I've just read parts of the BIP0038 spec - there are two ways of generating an encoded address -
The second approach is used when you want a third party to print your paper wallets, or hard coins, but don't want to compromise these wallets. It's a good feature, but considering that we generate the addresses in the same context where we have the passcode - completely unnecessary, since both methods are just as secure, as long as your passkey is secure. As such, I propose to change the BIP0038 encoded wallet generation to generate a random address, and encode the known private key. This will get rid of that issue whatsoever. Cheers, |
Something I try to do is make sure that someone who wants to verify the integrity of the RNG and such can easily "diff" the code and see that the changes are all interface, not crypto-related.
Canton Becker |
|
If necessary, we can even do better, and allow for both implementations (e.g. to allow bitcoinpaperwallet.com to be used by third parties printing wallets for someone). This could be implemented using a dropdown of supported wallet encryption methods, like you suggested here: http://www.reddit.com/r/Bitcoin/comments/1topvo/bitcoin_paper_wallet_with_bip0038_support_please/ceaggq4 |
All right, that's what I wanted to know.. Basically, the page is made up of open-source scripts (Crypto, EC, Bitcoin, Crypto_Scrypt and so on, that I'd rather leave completely untouched), the ninja.privateKey, ninja.publicKey, ninja.qrcode helper classes that don't really need to be changed either, and ninja.wallets.* that contain the link between the page layout and the rest of the objects.. Only the latter need changing, since your interface is different, some parts are not used, and others might work differently. This way it can be more manageable, while keeping it close to the source, so that it can be diffed or updated |
|
One additional feature as part of the BIP38 capability is to start from a brainwallet passphrase to create the first WIF private key. I would then use that private key and BIP38 (with a new passphrase) to create the encrypted private key. |
|
Regarding what level of BIP38 support to offer, my inclination is to only handle the most common scenario -- someone password-protecting a wallet they are generating for themselves -- and not bother dealing with the "EC multiply" cases. As I read it, EC multiply is fairly specific to selling loaded paper wallets. While this is a practice I won't actively discourage, I'm not at all inspired to put time or money into developing it. The fundamental idea behind bitcoinpaperwallet.com is to empower individuals to print and load their own wallets without any 3rd parties whatsoever. So:
A sidebar to #2: If we WERE going to enhance the "supply my own key" function at all, the format I really want to support is being able to type in any significantly long string (say minimum of 100 characters) and have its SHA256 hash used in generating the keypair. I suppose this could be used for brain wallets, but more importantly, we could guide users to type in the results from throwing a 6-sided die 100 times. I'd like to give users at least one option that doesn't involve trusting the RNG. Question of the moment: What do you think? Instead of rolling a single 6-sided die 100 times, is it just as random to throw ten 6-sided dice 10 times, provided you accurately reflect the actual left-to-right physical position in which the multiple dice fall?
|
…ng to rended an unencoded version as well * streamlined the code to improve manageability
|
Hi Canton, After reviewing the BIP0038 spec yesterday, I believe my initial assumption that the passcode was used as a random seed was inaccurate.. The code was using the EC multiply version of the encoder, hence the two-call approach in code. I'm really not entirely sure why they did that, instead of encoding a randomly generated address in the first place, since both are the same in terms of security, and both use the secure random generator get a random seed. I've made some changes to the ninja class as discussed, to make it a bit more manageable, while keeping it close enough to the original source. None of the open source library code was changed at all. I'm generating a new address, and if encryption was enabled- encrypting it with BIP0038. This way we always have a private key as well for the generated address. The loading progress seems to be working fine, I may know what was happening (browser wars, they will never cease!), so hopefully it should work in any browser now. I haven't touched the "Supply your own key" for now, but it would be a good idea to have some option to decode a BIF0038 encoded address. I'm sure people would appreciate if we not only allow them to encode their addresses, but also test and decode them back if needed. Could you come up with an interface piece for that? Regarding the last point - generating a new address is done by the ECKey class from bitcoin-js, and it doesn't allow for a custom seed, it seems. Maybe we could extend the code to do that, but I'm not sure how good of an idea is it to play with the bitcoin-js code... P.S. Now that's a good question. Without thinking too much into it - I'd say it's completely the same.. But I know statistics are never that easy, so I'm sure there is some difference. I wouldn't be able to say exactly how different it would be, but I think it would still be random enough :) |
|
UPD: Nevermind.. I can just run any random text through SHA256, and feed the result to ECKey, which would generate a address using this feed! So if you're interested in adding this functionality - it should be quite simple :) In fact, if you enter any non valid private key to the page, it will suggest if you want to use the text to generate a new address :) Technically, this is basically the same code that generates brain wallets, by the way... |
…lowing to generate a wallet with very short entropy
|
This is some great work! I really like the new implementation of the brainwallet capability. |
|
I deleted the previous message since it was just wrong. Looking at the source of Crypto_Scrypt, I noticed that it's using web workers to run the processing. It's an HTML 5 feature to run some processing in a background thread without blocking the UI. That's why when you run the page in Chrome and Firefox (which support this new feature) the loading animation works fine, while when you run this in other browsers (IE, Safari) the animation is not displayed/frozen. The best I can do at the moment is delay the processing 10ms after I show the loading bar.. This way the loading animation will be shown anyway, even if the browser doesn't support web workers. Please tell me if this works on your side |
|
Hi Canton, So there are two issues here, really.. I haven't used innerHTML, since I want to make sure the passphrase will appear exactly as the user has entered it, even if it has angle brackets, etc. The second one is that the passkey is "converted" somehow in safari. I've tried in a bunch of Windows browsers (Chrome, Firefox, IE 10 and Safari) and in every case I got the same encrypted passkey. That doesn't say there isn't an issue though. In the very beginning, I noticed that you changed the character charset on your page from utf-8 (as it is on bitaddress.org) to iso-8859-1. I didn't question it, and hoped we won't run into issues. I suspect this may be the issue in this case. I've changed the charset in HTML back to utf-8, re-saved the file under UTF-8 with BOM encoding, and pushed the changes to git. Please let me know if it gets it working in Safari on Mac OS X. I hope it does, since while limiting the characters you can enter as a passkey will technically work, I think it's a very bad solution, that will turn people away from your site. After all, the beauty of BIP0038 is that it uses unicode characters, which massively increases entropy with the same length password - no wonder someone put up their paper wallet with a 3 char password on Reddit, and it hasn't been cracked for a long time! Brute-forcing an ascii (or even a subset of that) password is massively faster than a full-on unicode one. Also, if you implement that filter, then if an attacker will see an encrypted paper wallet with your design - he will immediately know that this wallet is immediately hundreds of times less secure then the same one next to it printed from bitaddress.org. Not something you want your site to be known for :( |
|
Interesting point regarding whitespace in the passkey field.. I could trim it (and that's what bitaddress.org does, actually), but what if we checked for whitespace, and prompted the user with something like "We detected you have some whitespace characters around your passkey. This usually happens when you accidentally copy the passkey from someone else and accidentally include them. Would you like us to remove them?" This way if the end user wants his passkey to end in a space (for whatever reason) they can do that, and we don't force-trim the field. Or should I just trim the passkey field by default? What do you think? |
|
Hi Artiomchi, My Safari still encodes 5J4pcwBDPwPY1cdNqxTdmZWr7yCK8rXi9avFvezgYbmoatJpKGn with घोडा स्टेपल as whereas other browsers encode it as Notice the introduction of the possessive "My" Safari. After further testing I think this may get even worse: it may not just be a matter of needing to use Safari, but the same version (or OS?) of Safari to successfully decode. Argh! Also I'm pretty sure this isn't just limited to our BIP38 encryption. I was able to produce a Safari-only decodable BIP38 wallet using bitaddress.org as well. See: Crossquote: "At the moment I'm thinking the problem here is something to do with the BIP38 code where depending on the browser you're using (and with Safari in particular) some extended charsets are interpreted differently -- whether encrypting or decrypting -- which is why you are obligated to use the same browser to decrypt as you used to encrypt." |
|
Regarding firefox .innerText vs. IE . textContent, all I can offer is my sincere condolences backed by well over 10 years of having dealt with similar browser-exclusive quirks. It's like when you're smoothing out the bubbles beneath a screen protector and when you smooth out one air bubble, an opposite air bubble pops up... Anyway, I assume this can be addressed with a wrapper function that tests the property availability or something. Let me know if it's a more serious problem. As for whitespace, the only whitespace I want to allow is spaces. The problem of people copying/pasting tabs or worse yet carriage returns without realizing it could be very serious, since they might test their ability to decrypt the wallet also by copying/pasting and wouldn't realize the issue until years later when they fail to decrypt their wallet typing in the passphrase by hand for the first time. As for extended charsets, I'm not yet ready to throw out the baby with the bathwater and disallow anything besides plain-jane aA-zZ0-9. I agree it limits the brute force space too much. Besides which, it's not only our problem -- I believe this is endemic to all services doing javascript BIP38. I'm willing to wait and see if those communities have any thoughts on this issue before we do any limitation. That said, I've definitely got my "the sky is falling" hat on. Unless someone points out that I've got something seriously wrong with my testing methodology, I'll put up a PSA on reddit on thursday saying, "Hey if you use BIP38, you might not want to use extended charsets for the time being." |
|
Don't worry about the innerText vs textContent property field.. I've already fixed it in 22f3817 :) I'm doing a bit more testing for the other issue.. I've confirmed that it doesn't seem to have anything to do with unicode, and might be an issue with Safari, actualy.. Specifically, Safari 6! I don't have a Mac to test it with, so I got a trial on www.browserstack.com, and tried it there. As previously discussed, I tested it on Windows 8, latest Chrome + Firefox + IE 11 + Safari, and it works exactly as intended. I then went to browserstack, and tried it on different combinations.. (private key used: 5J4pcwBDPwPY1cdNqxTdmZWr7yCK8rXi9avFvezgYbmoatJpKGn, passphrase: घोडा स्टेपल )
I've then tried to do something else.. Use a NON unicode passkey:
Just to prove that, I went to bitaddress.org, and tried to test the private keys from the previous test, with the simple passkey:
UPD: Same result on bitaddress.org
What this basically is proving is that the system works fine on Safari 5 and 7, but on Safari 6 the actual encryption/decryption code provides different results, compared to all other browsers. It's not related to UTF-8 as I originally expected, I think it might be something in the actual encryption code working differently on Safari 6, which is actually quite worrying! |
Holy crapballs. That is all. |
|
Updated post above with confirmation that the issue also exists in bitaddress.org and bit2factor.org =/ Holy crapballs indeed |
|
Hi Everyone, I've spent the last 4-5 hours on this, and it feels like I'm going insane.. I don't have a mac as well, which means I have to do it over a Flash-VM interface, which slows me down a lot! Anyway, after adding multiple traces to the code, I've eventually reached a conclusion - Safari is terrible.. Among other things, it renders console.log output LONG AFTER you call them, sometimes with completely different results, and after the variables you log have been changed =/ After wasting 4 hours on fake readings, I got some results... It needs more logging, but it's almost 3 AM, and I REALLY need to sleep. At this time, I can see that the call to Crypto_Scrypt is returning different results when started on Safari 6, than in other browsers. I may be wrong, since I'm pretty silly sired at this point, but that's what seems to be causing the issue here. I'll try dig deeper tomorrow evening |
|
Here's a small self-contained page that demonstrates the issue: http://jsutftest.azurewebsites.net/testCrypto.html |
|
Ok, got some further progress on it. I've narrowed it down to the scrypt library, and managed to replicate it with minimal code. Since no other library is required to replicate the test, I've opened an issue in the scrypt-js project, where we can follow-up, and hopefully find a fix. cheongwy/node-scrypt-js#2 |
|
I can't be 100% certain, so this needs testing, but this may be the fix to the crypto_scrypt issue for Safari 6, without compromising performance or stability on other browsers. Please test it out, and reply here with whether it works or now. It would be also good to see if all the async tests start passing, so test this url as well: http://jsutftest.azurewebsites.net/generate-wallet.html?asyncunittests=1 I've written a bit more about it in the node_scrypt issue tracker above... Fingers crossed! |
|
I loaded up pull revision 53aa090 on my local safari 6.05 and confirmed:
running 8 tests named testDecryptBip38 |
|
I've sent you a private email artiomchi, but I think with this safari fix (!! HUZZAH !!) all that remains to be done before I do my final rounds of tests and minor CSS tweaks are:
|
pointbiz/bitaddress.org@897ad54f5e - "Initialize pptr to pseudo-random place before seeding." pointbiz/bitaddress.org@dc52dfe703 - "More entropy." pointbiz/bitaddress.org@19627f73cf - "Seed mouse movements as 16-bit numbers."
pointbiz/bitaddress.org@76704bfce2 - "v2.7.6 increase seed pool size. show seed pool while moving mouse. add textbox for keyboard entropy. do not generate address without human added entropy. browser fingerprint entropy added to seed pool starting in a random position. whole seed pool initialized with getRandomValues." pointbiz/bitaddress.org@d389e975fe - "Change pool size back to 256 bytes." pointbiz/bitaddress.org@3d135f7ea6 - "v2.8.0 visualize entropy collection" pointbiz/bitaddress.org@b6fb655b3a - "v2.8.1 […] Fix mouse movement visualization for Firefox and IE."
… easily brought back)
|
Hey Canton! Here's a rather bulky update! Merged the changes from bitaddress again (got some visual improvements, and better entropy) The upstream merge commits have also brought in the green seedpoint dots (on purpose, check out d5b2012 to see them) and I've removed them in 07ea52c (which makes it easier to bring them back if you want to) The last commit adds the visual mockup for the entropy generator.. You would have to improve the visuals there though, I'm not that good with css :) How does that look? |
|
Superb! Thanks Artiom. So far, the upstream entropy migrations seem to work great. Right now the hexadecimal display overprints the rest of the page and I assume that's what you'd like me to tackle. I think I'd like to use the "green dots" version (d5b2012) for the final candidate. I might style them a little differently, but let's leave them in for now. Possible bug: I just noticed something now, sorry I didn't catch this earlier: Typing an invalid key into the validate field and pressing "View Details" does nothing, and throws "11831: TypeError: 'undefined' is not an object (evaluating 'ninja.wallets.brainwallet.minPassphraseLength')" I figure it should alert something like, "Sorry but that is not a valid private key. Supported formats include WIF and BIP38." |
…he seedpoints! ! Fixed a broken reference to brain wallet passkey length!
|
Hi Canton, You're absolutely right.. I've gone with the same code/behaviour that is on bitaddress.org - if the key is not valid it will either show an error message, or if the message is long enough - show that message, suggesting to create a brain wallet out of it. I've also reverted the previous commit, restoring the green dots. By the way, e9afe25 should have taken care of the seed display overlaying the page.. It looks rather rough, and not quite as in the template (needs some positioning and font size playing around with) so you can adjust it to make it look as you see fit |
|
For anyone interested, Artiomchi's work is going live in the next few days unless any serious bugs are reported. Artiomchi, GREAT WORK, thanks. Here's a preview URL of artiomchi's pull request merged with a bunch of interface changes/enhancements: https://bitcoinpaperwallet.com/bitcoinpaperwallet/generate-wallet-bip38.html |
|
Closed with release of Feb. 10, 2014. |
As discussed in issue #3 - we love this wallet generator, but it lacks BIP0038 support.. I'd love to create a paper wallet, and then print a copy of it and give it to my friend/brother for safe storage, while not worrying that someone might take the money off the wallet.
I've taken the encryption code taken directly from the live bitaddress.org version