YunoHost-Apps / adguardhome_ynh

AdGuard Home package for YunoHost: Network-wide ads & trackers blocking DNS server
https://adguard.com/adguard-home.html
GNU General Public License v3.0
15 stars 15 forks source link

Password hashing and validation problems #188

Open aAlexanderLeben opened 4 months ago

aAlexanderLeben commented 4 months ago

Describe the bug

There is no pre-validation of the main.extra.new_password from config_panel.toml which leads to errors at installation step.

Context

Steps to reproduce

  1. Navigate to https:///yunohost/admin/#/apps/install/adguardhome in your browser
  2. Fill in the required fields
  3. In the password field, enter a password containing non-ASCII characters, e.g. VWÎQF¯ê6Ò´3ÜÂû°QOÔQà8±©¨6àaÕ¨Há§ïN¨ZµÆøYpsVçiUBò¼T¸H²NügÙÒ¸ûÃ6¤0
  4. Try to install the app
  5. Watch installation fail way into the installation

Expected behavior

A check of the password should be performed before attempting an installation.

Logs

https://paste.yunohost.org/raw/uxiduferob

aAlexanderLeben commented 4 months ago

Problems occur in install#L102 when calling

password=$(python3 -c "import bcrypt; print(bcrypt.hashpw(b\"$password\", bcrypt.gensalt(rounds=10)).decode())")

Problem 1: Non-ASCII characters

Trying to construct a byte literal using b\"$password\" produces a syntax error, e.g.

>>> test_byte = b"ê"
  File "<stdin>", line 1
    test_byte = b"ê"
                ^^^^
SyntaxError: bytes can only contain ASCII literal characters

From https://docs.python.org/3/reference/lexical_analysis.html#string-and-bytes-literals

Bytes literals [...] may only contain ASCII characters;

Problem 2: Passwords longer than 72 characters are truncated

See https://github.com/pyca/bcrypt/tree/main?tab=readme-ov-file#maximum-password-length

>>> import bcrypt
>>> salt = bcrypt.gensalt(rounds=10)

>>> ok_password = b"2wvKFD7C0FbfATxP9aK5eWa2BbiwEuaFjRkM4tSo6c72atn20CJYV2NRBDrfVvg451ZHbaYK"
>>> problematic_password = ok_password + b"padding"

>>> bcrypt.hashpw(ok_password, salt) == bcrypt.hashpw(problematic_password, salt)
True

Possible Solution

Add validation to config_panel.toml, not very sure about the syntax, but you should get the idea. Maybe a lower limit should be introduced as well. And maybe there are more perks to it that I did not notice.

Eplaination of the regexp [ -~] can be found on https://catonmat.net/my-favorite-regex

[main.extra.new_password]
pattern.regexp = '[ -~]{,72}'
pattern.error.en = "Your password must only contain ASCII characters and cannot have more than 72 characters