PowerDNS / pdns

PowerDNS Authoritative, PowerDNS Recursor, dnsdist
https://www.powerdns.com/
GNU General Public License v2.0
3.65k stars 906 forks source link

scrypt hashing overly sensitive to format #12187

Open nneul opened 1 year ago

nneul commented 1 year ago

Short description

scrypt handling for api-key and webserver-password is picky about it's format, in particular:

  1. It demands that the parameters (ln, p, r) be in exactly that order, and does not tolerate ln, r, p
  2. It requires that the base64 encoded salt and hash be padded, and will not accept a hashed pw generated without the padding

Environment

Steps to reproduce

generate salted/hashed pw with python:

from passlib.hash import scrypt
scrypt.using(rounds=10,parallelism=1,salt_size=16).hash("hello")

# example result is: 
# $scrypt$ln=10,r=8,p=1$OqcUIqR0DoEwJsQ4x5iTsg$dXw2H1XQnOrNOYhw9ywJ5Dux8JDaaZJP8GmNzoU9Idw

# example pdnsutil hash-password result:
# $scrypt$ln=10,p=1,r=8$13H0KckkRlTm/fFeEO0njw==$3fNoiUMMnhATUfiG1N65Qxm8iqx56hYoVyiG1CWo7Bk=

Try using above python generated scrypt in the config and it will fail to start.

Expected behaviour

Should accept a valid scrypt pw hash, and not enforce specific format that is not required.

Actual behaviour

Failed to accept it, service won't start.

Other information

rgacogne commented 1 year ago

I'm not sure we should accept parameters in any order, as the PHC specifications state that:

The function MUST specify the order in which parameters may appear. Producers MUST NOT allow parameters to appear in any other order.

I cannot find the definition for scrypt, but it looks like recent versions of passlib, as well as nodejs, indeed expects ln, r then p. So I guess we need to fix what we produce, and also be more lenient in what we accept.

rgacogne commented 1 year ago

As for the base64 padding, it's unfortunate that the specification decided to remove it, because most of the existing base64 decoders expect it. I guess we will have to do what we already do for incoming DoH, where padding is optional (sigh), and add the padding ourselves before passing the base64 payload to the decoder.