Add type for passwords. by twittner · Pull Request #8920 · openethereum/parity-ethereum

twittner · 2018-06-19T08:40:23Z

No description provided.

parity-cla-bot · 2018-06-19T08:40:25Z

It looks like @twittner signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

niklasad1 · 2018-06-19T09:20:46Z


 	/// Changes the password of `account` from `password` to `new_password`. Fails if incorrect `password` given.
-	pub fn change_password(&self, address: &Address, password: String, new_password: String) -> Result<(), Error> {
+	pub fn change_password(&self, address: &Address, password: Password, new_password: Password) -> Result<(), Error> {


Out-of-scope for this PR:
Why not pass-by_ref here?

niklasad1 · 2018-06-19T09:21:04Z


 	/// Exports an account for given address.
-	pub fn export_account(&self, address: &Address, password: String) -> Result<KeyFile, Error> {
+	pub fn export_account(&self, address: &Address, password: Password) -> Result<KeyFile, Error> {


Out-of-scope for this PR:
Why not pass-by_ref here?

niklasad1 · 2018-06-19T09:21:31Z


 	/// Helper method used for unlocking accounts.
-	fn unlock_account(&self, address: Address, password: String, unlock: Unlock) -> Result<(), Error> {
+	fn unlock_account(&self, address: Address, password: Password, unlock: Unlock) -> Result<(), Error> {


Out-of-scope for this PR:
Why not pass-by_ref here?

niklasad1 · 2018-06-19T09:53:15Z

+		let p = vec.as_mut_ptr();
+		for i in 0..n {
+			unsafe {
+				ptr::write_volatile(p.offset(i as isize), 0)


I think it could be worth to add a comment and explaining why write_volatile is used here. i.e., to actually make sure that passwords are erased and not LLVM performs some magic optimization!

Also consider to simply the code to:

let ptr = unsafe { s.as_mut_vec() }; for byte in ptr { unsafe { ptr::write_volatile(byte, 0) } }

niklasad1

Looks good but document why volatile writes are used to erase the passwords in the memory!

5chdn · 2018-06-22T09:17:34Z

Needs 2nd review.

andresilva

LGTM

andresilva · 2018-06-22T10:57:45Z

+
+use std::ptr;
+
+#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]


Should we provide a custom Debug that prints a static Password(******)? 😄

dvdplm

Seems like a good improvement. I wonder if we should accompany this with std::mem::drop(password) calls in the appropriate places as well?
Minor grumble: indentation in a few places look off: https://github.com/paritytech/parity/pull/8920/files#diff-d3979a57cbb0a0d1504cc1f5bab2d007R36 and https://github.com/paritytech/parity/pull/8920/files#diff-4d8151f665cbc6bf2a9fe83d64a61b64R35

andresilva · 2018-06-22T11:11:52Z

+impl Drop for Password {
+	fn drop(&mut self) {
+		unsafe {
+            for byte_ref in self.0.as_mut_vec() {


Replace indentation spaces with tabs.

andresilva · 2018-06-22T11:12:17Z

-				ptr::write_volatile(p.offset(i as isize), 0)
+		unsafe {
+			for byte_ref in self.mem.as_mut() {
+                ptr::write_volatile(byte_ref, 0)


niklasad1 · 2018-06-22T14:58:32Z

@dvdplm

Explicit mem::drop is just to drop a value earlier than when it goes out-of-scope and in this case it doesn´t make sense to me! In this case, drop will called on the String after the drop on Password has been executed!

Also in context of this issue the essential part is the zero out the memory in RAM holding the password because after Rust is dropping the value (calling free() I guess), it's no guarantee that it actually gets destroyed it´s up the each Operating System's free() implementation to decide what to do with the memory. It might just be in memory until a malloc() call with the specific chunk size arrives for efficiency reasons!

dvdplm · 2018-06-22T17:59:07Z

@niklasad1 makes sense, especially considering that String is not Copy. My (wrong) thinking was this: perhaps there are places in the code where the Password is copied as part of a method call and so if we are worried about the data lingering in memory we might want to zero out the mem of the copy asap instead of waiting for it to go out of scope. Brainfart. :)

andresilva · 2018-06-22T18:00:49Z

@niklasad1 I think what @dvdplm meant was to use drop to make sure the value is dropped as soon as possible (before end of scope), the idea would be to minimize the amount of time sensitive information remains in memory. I didn't check the "owning" sites to see if there was any place where it made sense to drop eagerly.

* master: Add type for passwords. (#8920) deps: bump fs-swap (#8953) Eliminate some more `transmute()` (#8879) Restrict vault.json permssion to owner and using random suffix for temp vault.json file (#8932) print SS.self_public when starting SS node (#8949) scripts: minor improvements (#8930)

…rp_sync_on_light_client * 'master' of https://github.com/paritytech/parity: parity: omit redundant last imported block number in light sync informant (openethereum#8962) Disable hardware-wallets on platforms that don't support `libusb` (openethereum#8464) Bump error-chain and quick_error versions (openethereum#8972) EVM benchmark utilities (openethereum#8944) parity: hide legacy options from cli --help (openethereum#8967) scripts: fix docker build tag on latest using master (openethereum#8952) Add type for passwords. (openethereum#8920)

Add type for passwords.

ba2ac35

5chdn added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Jun 19, 2018

5chdn added this to the 1.12 milestone Jun 19, 2018

niklasad1 reviewed Jun 19, 2018

View reviewed changes

Fix test.

e2bcac3

niklasad1 reviewed Jun 19, 2018

View reviewed changes

niklasad1 suggested changes Jun 19, 2018

View reviewed changes

Simplify Drop impls of Password and Memzero.

2090738

niklasad1 approved these changes Jun 19, 2018

View reviewed changes

andresilva self-requested a review June 22, 2018 00:32

andresilva approved these changes Jun 22, 2018

View reviewed changes

andresilva added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jun 22, 2018

dvdplm approved these changes Jun 22, 2018

View reviewed changes

andresilva reviewed Jun 22, 2018

View reviewed changes

andresilva added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Jun 22, 2018

twittner added 3 commits June 22, 2018 13:36

Merge branch 'master' into tw/password-type

b1dbfee

Spaces to tabs.

ae88e1c

Custom Drop impl for Password.

b58e87a

dvdplm added A8-looksgood 🦄 Pull request is reviewed well. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Jun 22, 2018

dvdplm merged commit 41348de into master Jun 22, 2018

twittner deleted the tw/password-type branch June 22, 2018 13:13


		use std::ptr;

		#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]

Conversation

twittner commented Jun 19, 2018

Uh oh!

parity-cla-bot commented Jun 19, 2018

Uh oh!

niklasad1 Jun 19, 2018

Choose a reason for hiding this comment

Uh oh!

niklasad1 Jun 19, 2018

Choose a reason for hiding this comment

Uh oh!

niklasad1 Jun 19, 2018

Choose a reason for hiding this comment

Uh oh!

niklasad1 Jun 19, 2018

Choose a reason for hiding this comment

Uh oh!

niklasad1 left a comment

Choose a reason for hiding this comment

Uh oh!

5chdn commented Jun 22, 2018

Uh oh!

andresilva left a comment

Choose a reason for hiding this comment

Uh oh!

andresilva Jun 22, 2018

Choose a reason for hiding this comment

Uh oh!

dvdplm left a comment

Choose a reason for hiding this comment

Uh oh!

andresilva Jun 22, 2018

Choose a reason for hiding this comment

Uh oh!

andresilva Jun 22, 2018

Choose a reason for hiding this comment

Uh oh!

niklasad1 commented Jun 22, 2018

Uh oh!

dvdplm commented Jun 22, 2018

Uh oh!

andresilva commented Jun 22, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants