Dgraph: security: Password type stored as clear text

[Scapal]

Even if the Password type is secured by being unreadable, only verifiable, it is stored as plain text.

By just altering the schema an changing the type back to string, you can dump all the passwords. This is very wrong.

You should scrypt the password using the node uid and the predicate name as source for the salt.

This can be done without changing the usage behavior.

[MichelDiz]

Password type is encrypted. You have the ability to transform any Type to other types so you free turn Password type to plain text tho.

Check: https://github.com/dgraph-io/dgraph/blob/06ea4c545bc3cf0ed730327557dbff96406e75f8/types/password.go#L25

If you need to have full control over Password, please use other approaches and write with String type. This feature is just to make the DB user's life easier. If the user need something more complex better by DIY. More safe and controleble by their end.

@manishrjain Quick question, though it is encrypted. Is there a need to do something for this case? Otherwise it can be closed.

[Scapal]

Reproducing the bug:

Alter the schema to add a secret predicate:

/ALTER secret: password .

Mutate to create an entry:

{
  "set": [
    {
      "uid": "0x7",
      "secret": "Password1234"
    }
  ]
}

Then test the secret:

{
  everyone(func: has(secret)) {
    name,
    checkpwd(secret, "Password1234")
  }
}

It works as intended.

Now I alter the schema to set secret as a string:

/ALTER secret: string .

Query for the secrets:

{
  everyone(func: has(secret)) {
        name,
    secret
  }
}

Output with clear password:

"data": {
    "everyone": [
      {
        "name": "Alice",
        "secret": "Password1234",
        "uid": "0x7"
      }
    ]
  }

So, it seems there is a bug !

[MichelDiz]

@Scapal I do not believe it to be Bug. Well, it's expected behavior. For me could be an enhancement (a Lock?). Only Mrjn could confirm.

Ps. In practice no one should have access to Schema. Users should not have the ability to convert Types.

Check https://github.com/dgraph-io/dgraph/blob/06ea4c545bc3cf0ed730327557dbff96406e75f8/types/conversion.go#L29

Line 78:

    case PasswordID:
        *res = string(data)

Line 302:

    case PasswordID:
        {
            vc := string(data)
            switch toID {
            case BinaryID:
                // Marshal Binary
                *res = []byte(vc)
            case StringID:
                *res = vc
            case PasswordID:
                *res = vc
            default:
                return to, cantConvert(fromID, toID)
            }
        }

[srfrog]

Perhaps not let passwords to be altered to other types ? It would be a new feature, and a good one IMO.

[Scapal]

If it was encrypted using bcrypt like you showed me in password.go, it couldn’t be reversed to clear text. Changing the schema wouldn’t be an issue.

[MichelDiz]

@srfrog Gus, I think we should have a Lock to alter. Something like a PIN or a password. That might help. But also prevent from using Convert func from PasswordID to string. For in theory not even the DB maintainer should have access to the plain text. For that would be an invasion of privacy.

What do you think Gus?

@Scapal It was encrypted (not so sure now), the problem is that you're free to convert tho. Dgraph is able to freely convert a Password type, since it is the one that generates the same ones. Soon it is not a Bug, as we can see clearly in the code the open possibility. So it's not a glitch. Whoever wrote the code knew what he was doing.

So that's why I think it's a feature.

Cheers.

[manishrjain]

Hey guys, I want to weigh in here:

The existing functionality is right (looks buggy) but needs tweaks.

What's right: We never store the password in plain text (or at least that's the logic, disregarding any bugs for now). We're not going to change that behavior, but fix if there's a bug here.

What we should tweak: We should not allow a password type to be altered, i.e. a default type can't be converted to passwd type, and vice-versa. It can only be "created" and "dropped", i.e. either a predicate should be passwd type to begin with (before the first mutation), or it should be deleted. It should not be converted. Having said that, this bug needs an investigation to figure out how is it possible for us to store data encrypted, and it getting returned as plaintext.

[Scapal]

When creating a secret predicate of type Password and perform an export, here is what I get:

rdf file:

<_:uid7> <name> "Alice"^^<xs:string> .
<_:uid7> <secret> "Password1234"^^<xs:string> .

schema file:

secret:password . 

It seems the secret is never going through type conversion.

[Scapal]

ok, the <xs:string> is legit in the export but it should be the encrypted Base64 value. The Convert()from password to string just copies the string.
https://github.com/dgraph-io/dgraph/blob/06ea4c545bc3cf0ed730327557dbff96406e75f8/types/conversion.go#L312

[Scapal]

The problem only occurs with json mutations.

{
  set {
    <0x7> <secret> "Password1234" .
  }
}

properly encodes the secret:

<_:uid7> <secret> "$2a$10$FWbP4pN0jcr5mRU7iLtEAexR/heMaKRNARGoswOW2njHKYPSKSkT."^^<xs:string> .

[srfrog]

@Scapal I have a PR to fix this. see if it resolves your issue #2627