Skip to content
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.

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

NFC backup. can store wallet address, spend key ,view key, seed, block height in NFC(Authenticated by the NFC default key now) #282

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

jianjunchu
Copy link

TODO:

  1. read keys from NFC and restore the wallet
  2. change the NFC default key to a user defined key.

@jenniferberger
Copy link
Contributor

http://www.docjar.com/jar_detail/sunjce_provider.jar.html ? has a different sha256sum than the commited file
https://github.com/m2049r/xmrwallet/pull/282/files?short_path=9356291#diff-b871a8f845c6b38c020e11f57ee0fc9bR39
what is it used for? can something in the android api already do that?

http://jacksum.net/en/download.html ? has the same sha256sum as the commited file

but when you read the legals it is imo blocking the addition:

Can I incorporate Jacksum into my project?

Yes, but Jacksum is not "public domain", and Jacksum is not "freeware". Jacksum is free software which is compliant with the Free Software Foundation idea. Jacksum is distributed under the terms of the GNU General Public License (GPL), as published by the Free Software Foundation (FSF) - GPLv2 or later. If you develop a program that is based on Jacksum, the program must be compatible with the GPL. It is against all policies, if you use Jacksum's source code, and put it in a closed source project, and/or claim it as yours. Jacksum's sources are free and must be free for ever, and you should respect this freedom. Once you have understand the advantages of openess, you have done a step to a greater world.

not sure if this is Apache compatible

From a first code skim it is visible that this library is only used to create a crc:
String checksumArg="crc:16,1021,c6c6,true,true,0"; checksum = JacksumAPI.getChecksumInstance(checksumArg,false);
https://github.com/m2049r/xmrwallet/pull/282/files#diff-29669d3fa2daf6a054e456160ebb0d81R987
so it should be simple to implement it again or find a more liberal implementation of that checksum method.
In fact, this getCheckSum method gets the view&spend key of the wallet, the handling of that data should not be externalized if possible

suggestions:

  • lint the code
var= something +else + on [very,long, lines];
if (expr ) {
}else
for (..)
{
}catch () {

is messing with the reviewer.

  • review comments - remove commented out blocks, rewrite Chinese to English
  • add description to help what kind of devices one should use
  • store a height (of the first transaction) in the backup to speed up restores
  • add a hint to the user that the secrets are transfered in plain-text over the air and that a secure location should be used to prevent eavesdropping.

I like the idea to move the wallet off the phone to a different device. Can you point out compatible vendors? How does this differ from #133? and how can the restore code be shared?

@jianjunchu
Copy link
Author

ok, I will replace Jacksum.jar, and lint the code , etc..
Thanks for the review.

@jianjunchu jianjunchu changed the title Add NFC backup. now can save wallet spend/view key and seed in NFC(authenticated by the default key) NFC backup. can store wallet address, spend key ,view key, seed, block height in NFC(authenticated by the default key now) May 17, 2018
@jianjunchu jianjunchu changed the title NFC backup. can store wallet address, spend key ,view key, seed, block height in NFC(authenticated by the default key now) NFC backup. can store wallet address, spend key ,view key, seed, block height in NFC(Authenticated by the default key now) May 17, 2018
@jianjunchu jianjunchu changed the title NFC backup. can store wallet address, spend key ,view key, seed, block height in NFC(Authenticated by the default key now) NFC backup. can store wallet address, spend key ,view key, seed, block height in NFC(Authenticated by the NFC default key now) May 17, 2018
@jianjunchu
Copy link
Author

@jenniferberger Yes, the NFC Backup is similar to #133. but it is more convenient and security I think. they can use the same restore code.
I am designing the NFC device(the first generation is only a set of two NFC cards. the next generation is a small PCB with two NFC chips and two button. not sure by now)

@m2049r
Copy link
Owner

m2049r commented Jun 10, 2018

     Caused by: java.lang.NoClassDefFoundError: Failed resolution of: Lcom/sun/crypto/provider/SunJCE;
        at com.m2049r.xmrwallet.nfc.ThreeDES.<clinit>(ThreeDES.java:26)
        at com.m2049r.xmrwallet.LoginActivity.getKey(LoginActivity.java:483)
        at com.m2049r.xmrwallet.LoginActivity.backupWalletToNFC(LoginActivity.java:454)

@m2049r
Copy link
Owner

m2049r commented Jun 10, 2018

also, maybe NFC functions should only be shown if NFC is enabled?

@jianjunchu
Copy link
Author

@m2049r com.sun.crypto.provider.SunJCE is in lib/sunjce_provider.jar
and add "compile files('lib/sunjce_provider.jar')" in the build.gradle file.
if NFC is not found or not enabled, will show a message "Please Enable NFC fIrst"?

@jenniferberger
Copy link
Contributor

When reading it again (thanks for the lint) with a bit distance it feels like a patch that enables leaking the most sensitive information over an unsecured wireless interface. There should be no code that does that in the app. Its hard to audit and physically verify.
In addition the remote transfer would make more sense if the complete wallet (with synced blocks) is exported so it saves me a 30++min wait after i open it. NFC has not enough memory/bandwidth for that imo (1k in smartcard, 10kbs over wireless?). Doing this with a wifi-ap in the remote device may be acceptable.

@jianjunchu
Copy link
Author

@jenniferberger Thanks for reply. The NFC reading/writing distance is very short (less than 5 cm),so it 's hard to leak any information. Acturelly there are some similar NFC key storage devices now. see https://www.youtube.com/watch?v=5nOzobRr4kc. And we also have most secure NFC chips (the space wave is encrypted) for the most secure levels. We can use these kind of chip if necessary.

@m2049r
Copy link
Owner

m2049r commented Aug 5, 2018

@jianjunchu after spending over 12 hours with this code and rewriting most of it I still can't figure out what the memory layout of the NFC tags is. On the one hand you have AUTH0/AUTH1 at pages 241/242 and on the other you have the key stored in pages 0x2C-0x2F which is in the middle of where you are storing the viewkey! Also the tag authenticates any(!) key. I could not find any code you have which actually sets the tag's keys.

Please post a spec of the tags used.

Also, I am not really sure what the purpose of the authentication is in this scenario. Unless we create a key from user input it seems pointless, as all devices would have the same code.

@jianjunchu
Copy link
Author

@m2049r

  1. Sorry , I fixed the writeNewKey method (the original 0x2C-0x2F is for another 144 bytes chip not for this app)
  2. Storage area of the NFC chips is show as pic
    image
    the more detailed spec will be upload later.
  3. I don't write "Set NFC password" code in the UI. Because I don't know where to set the NFC password in the UI. in the right corner menu(above language item) or in wallet popup menu? how do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants