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

Enforce 160-bit HOTP secret length and verify OTP secret length on login. #6222

Merged
merged 4 commits into from
Jan 12, 2022

Conversation

zenmonkeykstop
Copy link
Contributor

Status

Ready for review - note that this was already reviewed in #6191, but the PR base was not updated before merging.

Description of Changes

Fixes #6189 .

  • Enforces a 160-bit (40 hex chars in Admin Interface, 32 ascii-encoded chars in DB) requirement for HOTP secrets on new user accounts and edits to existing accounts' HOTP creds
    • Requires that a 40-char HOTP secret be provided when the is_hotp checkbox is selected on the New User form.
    • Requires that a 40-char HOTP secret be provided on the Change Secret admin page.
  • Checks for a minimum OTP secret length on all journalist accounts (TOTP and HOTP) on login, with the login failing if said length is less than 80 bits (20 hex, 16 ascii-encoded)

The motivation behind said 80-bit minimum is that there may still be 80-bit OTP secrets associated with journalist accounts using TOTP in production instances, as the 160-bit TOTP requirement was recently added and not applied retroactively. The minimum should be set to 160-bit at some future date, once it's reasonable to assume that the majority of users have re-rolled their OTP secret - or once 80-bit secrets are no longer considered viable security-wise.

Testing

(all tests can be performed in a dev environment. A Yubikey and associated HOTP secret would be best for testing, but CLI commands to generate HOTP tokens like oathtool -c <counter> --hotp "<hex secret here>" will also work. (in most cases below <counter> will be 0, but increment this field every time you use the same hotp secret. )

Adding new users

  • Run make dev against this branch and log in to the JI at localhost:8081 with an admin-level account
  • On the Admin page, choose Add User to add a new user
  • add a TOTP user (leaving is using a Yubikey unchecked) - specify a username and click Add User
    • confirm that the user is created
    • confirm that the TOTP setup page is displayed with a valid QR code and a 32-char secret
  • Go back to the Admin page, and choose Add User to add another new user
  • enter a new username and select the is using a Yubiky checkbox. Then click Add User
    • confirm that the user is not created and the error message The "otp_secret" field is required when "is_hotp" is set. is displayed
  • enter an arbitrary number of spaces in the HOTP Secret field and click Add User
    • confirm that the user is not created and the error message The "otp_secret" field is required when "is_hotp" is set. is displayed
  • enter a 40-char (excluding spaces) string including non-hexadecimal characters in the HOTP Secret field and click Add User
    • confirm that the user is not created and the flash error message Invalid HOTP secret format: please only submit letters A-F and numbers 0-9. is displayed
  • enter an incomplete OTP secret in hex format (any string of <num> hex chars with less than 40 non-space chars) in the HOTP Secret field and click Add User
    • confirm that the user is not created and the error message The HOTP secrets are 40 characters long - you have entered <num>. is displayed
  • enter a complete OTP secret in hex format (40 hex chars with arbitrary spaces) in the HOTP Secret field and click Add User
    • confirm that the user is created and the HOTP secret can be used to generate a valid confirmation code on the next page.

Editing existing users

  • As an admin user, log onto the JI, go to the Admin page, and click the edit button beside a user other than the admin user.

  • Click Reset Mobile App Credentials and click OK.

    • Confirm that the TOTP page is displayed with a valid QR code and 32-char ascii-encoded secret
    • Confirm that the secret produces a valid token using the form on this page.
  • Go back to the Admin page and click the edit button beside the user again

  • Click Reset Security Key Credentials and click OK.

    • Confirm that the Change HOTP Secret page is displayed.
    • Click Continue without entering anything in the field, and confirm that the page is reloaded without any change.
    • Add some spaces in the field and click Continue - confirm that the page reloads with a length error message
    • Add a string of less than 40 hex digits, with arbitrary spaces, in the field and click Continue - confirm that the page reloads with a length error message
    • Add a string of 40 chars including non-hex chars in the field and click Continue - confirm that the page reloads with an error specifying the correct string formatting.
    • Add a valid secret of 40 hex digits with arbitrary spaces in the field and click Continue - confirm that the Enable Yubikey (OATH-HOTP) page loads, and that the token generated by the valid secret works.
  • Repeat the above checks for the logged-in user's account and confirm that they pass.

Logging in as a user with an invalid OTP secret

  • connect to the docker container with docker exec -it securedrop-dev-0 sqlite3 /var/lib/securedrop/db.sqlite
  • pick a HOTP user to update - select username, otp_secret from journalists where is_hotp=1 - and change their otp secret to a valid ascii-encoded but less-than-16-char string, ie. GARBAGE with update journalists set otp_secret='GARBAGE' where username='<your user here>'
  • attempt to log in as the chosen user, generating a valid OTP token at the command line with oathtool -b --hotp "GARBAGE"
    • Verify that the user is not logged in and that an error is displayed instructing them to contact an admin to reset their 2FA.
  • log in as an admin-privilege user and update the chosen user's HOTP secret with a valid 40-char hex secret.
  • log in as the chosen user using the new secret and confirm that the user can log in.
  • log in as a user with an 80-bit (16-char in the DB, ie JHCOGO7VCER3EJ4L) secret and confirm that the user can log in
  • create a new user and verify that they can log in.

Deployment

No deployment-sensitive changes were introduced in this

Checklist

If you made changes to the server application code:

  • Linting (make lint) and tests (make test) pass in the development container

If you made changes to securedrop-admin:

  • Linting and tests (make -C admin test) pass in the admin development container

If you made changes to the system configuration:

If you added or removed a file deployed with the application:

  • I have updated AppArmor rules to include the change

If you made non-trivial code changes:

  • I have written a test plan and validated it for this PR

Choose one of the following:

  • I have opened a PR in the docs repo for these changes, or will do so later
  • I would appreciate help with the documentation (docs for HOTP only describe Yubikeys, which already have 160-bit HOTP secrets, so no change there, but it could be worthwhile to document the new failed login state when the account's OTP secret is invalid.)
  • These changes do not require documentation

- Updated tests to check for 40 char hexadecimal input on user adds/edits
- Added a RequiredIf validator to force validation of otp_secret field when is_hotp
field is checked in new user form
- Added a length check in function validating input for HOTP updates.
@zenmonkeykstop zenmonkeykstop requested a review from a team as a code owner January 12, 2022 16:58
@cfm cfm self-assigned this Jan 12, 2022
@cfm cfm added this to Under Review in SecureDrop Team Board Jan 12, 2022
Copy link
Member

@cfm cfm left a comment

Choose a reason for hiding this comment

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

Approving based on visual review: this pull request shares its branch with the previously-approved #6191 and therefore its head at 0acee78.

@cfm cfm merged commit f768a1a into develop Jan 12, 2022
SecureDrop Team Board automation moved this from Under Review to Done Jan 12, 2022
@zenmonkeykstop zenmonkeykstop deleted the 6189-enforce-hotp-length branch January 12, 2022 18:52
@eaon eaon mentioned this pull request Feb 11, 2022
35 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

HOTP secrets are not validated correctly.
2 participants