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

SigningSecrets serialization: Change to not use Gson and work with byte array #110

Closed
MrStahlfelge opened this issue Apr 14, 2022 · 2 comments · Fixed by #111
Closed

SigningSecrets serialization: Change to not use Gson and work with byte array #110

MrStahlfelge opened this issue Apr 14, 2022 · 2 comments · Fixed by #111
Labels
enhancement New feature or request
Milestone

Comments

@MrStahlfelge
Copy link
Member

To not hold Strings containing the mnemonic in memory longer than needed, saving SigningSecrets into db should be changed.
At the moment, SigningSecrets are:

  • serialized to json with Gson - this was done to have more fields than mnemonic phrase saved in the future, but is not used at the moment
  • returned as a String
  • this String is converted into a Bytearray, encrypted and stored

The internal processing of Gson holds the mnemonic as a String as well as the json which means it can't be blanked immediately when not needed any more.

Processing should be changed as following:

  • mnemonic is not serialized any more, but converted into a byte array by Signing Secrets. Warning: ByteArrayOutputStream cannot be used, use this method
  • the byte array is blanked by AesEncryptionManager after encryption
  • for reading the mnemonic, SigningSecrets should get a Bytearray as well and convert the mnemonic right into SecretString
  • backwards compatibility for already stored secrets: if the Bytearray starts with {, the old deserialization method (Gson) should be applied

The backwards compatibility is important so that current users of the app do not need to restore their wallets but can continue using them.

@MrStahlfelge MrStahlfelge added the enhancement New feature or request label Apr 14, 2022
@aslesarenko
Copy link
Member

Sound good, in addition the old secret storage can be auto-converted to the new format.

@MrStahlfelge
Copy link
Member Author

Implementation is added, please check. As said I would prefer to have this merged for 1.9 to have more time testing it.

Regarding auto-conversion: I am always hesitant in changing user data under the hood so I would rather not do it. If we really want to do it, it can only be done on certain usages because the layer actually using the mnemonic has no access to the database objects any more. Since sending funds is most probably the most frequently used code accessing the mnemonic, it would probably be enough to add it there if we really want to do it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
2 participants