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

Hash API password and comparison #5715

Merged
merged 10 commits into from Feb 21, 2018
Merged

Hash API password and comparison #5715

merged 10 commits into from Feb 21, 2018

Conversation

Crunsher
Copy link
Contributor

@Crunsher Crunsher commented Nov 2, 2017

This change requires (afaik, TODO: test) openssl version 1.X to work. To my knowledge this is supported by even Sles 11, Solaris 10 and AIX.

The patch adds the option to save passwords as hashes, the new api user command can create them, and hashes clear text passwords at startup.

This should protect icinga2 against timing attacks and improperly secured config files. It is backwards compatible and should not break any other applications.

refs #4920

@Crunsher Crunsher added area/api REST API enhancement New feature or request labels Nov 2, 2017
@Crunsher
Copy link
Contributor Author

Crunsher commented Nov 2, 2017

TODO:

  1. Tests are currently VERY ugly. Could be solved with a friend class (Edit: they also fail outside of Debug builds atm)
  2. Documentation
  3. Clean up code a bit
  4. Possibly give the api user command the option to update existing users

@Crunsher
Copy link
Contributor Author

Crunsher commented Nov 3, 2017

Tested with openssl 1.0.1g-0.30.1 on sles 11.4. Thx @N-o-X

@gunnarbeutner
Copy link
Contributor

Maybe we should make the --passwd argument optional and have Icinga generate a random password when it's omitted. What do you think? 🤔

@Crunsher
Copy link
Contributor Author

Crunsher commented Nov 9, 2017

I thought about this as well, but then we'd have to print the password out to console. Which would then make the workflow icinga2 api user ... > .../api_user.conf impossible

@Crunsher Crunsher force-pushed the feature/security-features branch 2 times, most recently from d7016f3 to f02ad93 Compare December 14, 2017 15:51
@Crunsher
Copy link
Contributor Author

Ready for review from my side

tl;dr:

  1. Clear text ApiUser passwords are now hashed at startup and kept with their salt in memory
  2. Passwords can be stored as hashed_passwords, this way read access to the config is no security risk
  3. Documentation exists

String apiUsersPath = GetConfdPath() + "/api-users.conf";
String api_username = "root"; // TODO make this available as cli parameter?
String api_password = RandomString(8);
String apiUsersPath = GetConfdPath() + "/api-users.conf";
Copy link
Contributor

Choose a reason for hiding this comment

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

Styleguide.

@@ -169,7 +169,7 @@ bool ApiSetupUtility::SetupMasterApiUser(void)
<< " * The APIUser objects are used for authentication against the API.\n"
<< " */\n"
<< "object ApiUser \"" << api_username << "\" {\n"
<< " password = \"" << api_password << "\"\n"
<< " password: \"" << api_password << "\"\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

This will break the config.


String ApiUserCommand::GetDescription(void) const
{
return "Create a hashed user + password string for the Icinga 2 API";
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: Replace + with and.


String user = vm["user"].as<std::string>();
String passwd = vm["passwd"].as<std::string>();
String salt = vm.count("salt") ? String(vm["salt"].as<std::string>()) : RandomString(8);
Copy link
Contributor

Choose a reason for hiding this comment

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

Style.

String passwd = vm["passwd"].as<std::string>();
String salt = vm.count("salt") ? String(vm["salt"].as<std::string>()) : RandomString(8);

String hashed_passwd = ApiUser::CreateHashedPasswordString(passwd, salt, true);
Copy link
Contributor

Choose a reason for hiding this comment

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

Variable name should be hashedPassword. We will apply this through the entire code base soon.

void SetPasswd(String s) {
m_Hashed_passwd = s;
}
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

Where's that block implemented/used?

#endif
private:
String m_Salt;
String m_Hashed_passwd;
Copy link
Contributor

Choose a reason for hiding this comment

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

Style.

if (password.IsEmpty())
user.reset();
else if (user && user->GetPassword() != password)
if (password.IsEmpty() || !user->ComparePassword(password))
Copy link
Contributor

Choose a reason for hiding this comment

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

This will terminate with a SIGSEGV if user is not defined. The check should include if (user && ..) and was split for that reason before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wouldn't user.reset() fail as well if user not defined ?


user->SetSalt("BBBP");
BOOST_CHECK(!user->ComparePassword("icinga2icinga"));
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

Any specific reason not to run this in release builds?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The getter and setters for passwords are only available in a debug build. To prevent other modules from setting them in the future. I tried to solve this by using friend classes but failed dramatically.

So tests are still an open todo

{
unsigned char digest[SHA256_DIGEST_LENGTH];
PKCS5_PBKDF2_HMAC(password.CStr(), password.GetLength(), reinterpret_cast<const unsigned char *>(salt.CStr()),
salt.GetLength(), iterations, EVP_sha256(), SHA256_DIGEST_LENGTH, digest);
Copy link
Contributor

Choose a reason for hiding this comment

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

Style, use 4 spaces for indent.

@dnsmichi
Copy link
Contributor

I've now understood its purpose, I feared that this would need a local storage (which it does not). The variable names should apply our coding style, similar to some other locations marked in the review.
Config rendering and API password comparison needs to be fixed too. Once that's done, I'll test the PR :)

@Crunsher
Copy link
Contributor Author

Additional idea by Gunnar:

  1. Make Password a non-config object
  2. Make HashedPassword a Config object
  3. If there is no HashedPassword, hash Password with salt and write it to HashedPassword
  4. Clear Password
  5. Only the HashedPassowrd will show up in object list and we won't have the clear text password in RAM

@Crunsher
Copy link
Contributor Author

Requesting re-review

--oneline can now be used to print out only the password hash string.
This can be used to update ApiUser passwords through the API. There is
also now a validation to make use salt does not contain a '$' which
would break verification.
@Crunsher
Copy link
Contributor Author

Refs CVE-2018-6535

@Crunsher
Copy link
Contributor Author

This should be merged after #6103 to easier resolve merge conflicts

@Crunsher Crunsher added this to the 2.8.2 milestone Feb 21, 2018
@Crunsher Crunsher merged commit fae7f17 into master Feb 21, 2018
@Crunsher Crunsher added the backported Fix was included in a bugfix release label Feb 23, 2018
@julianbrost
Copy link
Contributor

julianbrost commented Mar 15, 2018

I just stumbled across this PR, sorry for commenting on this long after it was merged.

Maybe we should make the --passwd argument optional and have Icinga generate a random password when it's omitted. What do you think?

This sounds like a good idea to me. Passing the password as a command line argument leaks it to other users on the system via the process list. IMHO there should be some alternative method like reading it from the console, using an environment variable, generating a random password, etc.

I thought about this as well, but then we'd have to print the password out to console. Which would then make the workflow icinga2 api user ... > .../api_user.conf impossible

You could print the config snippet to stdout and the cleartext password to stderr. Then redirecting stdout to a file would still show the password in the terminal.

@Crunsher
Copy link
Contributor Author

@julianbrost Both are additions which can be added in a future version. Could you make a new issue for those? We want to release 2.8.2 as soon as possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api REST API backported Fix was included in a bugfix release enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants