search

binance-exchange / binance-java-api

binance-java-api is a lightweight Java library for the Binance API, supporting synchronous and asynchronous requests, as well as event streaming using WebSockets.
MIT License
830 stars 622 forks source link

#402 - Secret key should be stored as a byte[] #403

Open antlen opened 3 years ago

antlen commented 3 years ago

https://docs.oracle.com/javase/8/docs/technotes/guides/security/crypto/CryptoSpec.html#PBEEx

However, here's the caveat: Objects of type String are immutable, i.e., there are no methods defined that allow you to change (overwrite) or zero out the contents of a String after usage. This feature makes String objects unsuitable for storing security sensitive information such as user passwords.

DeveloperKurt commented 3 years ago

But you will never need to override this credential, it is meant to be used consistently so the system this application is going to be used has to be secure. Also, this change will break the existing code.

antlen commented 3 years ago

Even though it is not being overwritten just keeping it in memory is a security risk, the JVM can be inspected for the string value.

There is no real need to store a secret key as a String as Binance just needs the byte[] to create the SecretKeySpec. Added to this, a lot of developers will store their private key in a java keystore and when the key is loaded from the keystore it will be in a byte[]. So the the ideal scenario is to load the key as byte[] from the keystore and pass to binance to create the SecretKeySpec from the byte[]. In this flow the secret key never needs to be stored as a String for the lifetime of the application.

I dont think this change will break existing code, I have kept the old String methods for backward compatibility.

Although for this change it might be better to allow a breaking change so developers have the opportunity to fix their code so that they never need to load the secret key as a String.

antlen commented 2 years ago

For example, this is how I instantiate Binance in my app using my binance fork

char[] keystorePassword; // ="xxxxx"; KeyStore keystore = KeyStore.getInstance("JCEKS"); keystore.load(new FileInputStream(ATH), password); Key k = keystore.getKey("BinanceSecretKey", password); SecretKeySpec secret = new SecretKeySpec(k.getEncoded(),"AES"); BinanceApiClientFactory factory = BinanceApiClientFactory.newInstance(apiKey, secret.getEncoded());

This means I never need to store the secret key as a string in memory