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

Safer way to handle secret information of cli_wallet #1171 #97

Merged
merged 3 commits into from
Jun 5, 2019

Conversation

cogutvalera
Copy link
Member

@cogutvalera cogutvalera commented Nov 21, 2018

This PR is for fixing bitshares/bitshares-core#1171 issue

Safer way to handle secret information of cli_wallet

Depends on already merged PRs:

  1. Safer way to handle unlock command of cli_wallet #1171 #86
  2. Safer way to handle unlock command of cli_wallet #1171 #82

Copy link

@pmconrad pmconrad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this approach much better.

src/rpc/cli.cpp Outdated
@@ -250,6 +249,12 @@ void cli::getline( const fc::string& prompt, fc::string& line)
FC_THROW_EXCEPTION( fc::eof_exception, "" );
line = line_read;
// we don't need here to add line in editline's history, cause it will be doubled
if (cli_check_secret(line_read)) {
el_no_echo = 1;
line_read = readline("Enter password: ");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Must free(line_read) before re-using it

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

src/rpc/cli.cpp Outdated
el_no_echo = 1;
line_read = readline("Enter password: ");
el_no_echo = 0;
line = line + ' ' + line_read;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Must add check for null like above

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@abitmore
Copy link
Member

abitmore commented May 5, 2019

@cogutvalera do you have time to continue the work on this?

@abitmore abitmore mentioned this pull request May 5, 2019
@cogutvalera
Copy link
Member Author

@cogutvalera do you have time to continue the work on this?

yes sure

@pmconrad
Copy link

yes sure

@cogutvalera any estimate when you do this?

@cogutvalera
Copy link
Member Author

I will try to do it till the end of this week, was unplanned busy

Copy link

@pmconrad pmconrad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@pmconrad pmconrad merged commit e2c5160 into bitshares:master Jun 5, 2019
@abitmore abitmore added this to the core-release-3.2.0 milestone Aug 21, 2019
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.

3 participants