konsultaner / connectanum-dart

This is a WAMP client (Web Application Messaging Protocol) implementation for the dart language and flutter projects.
MIT License
22 stars 14 forks source link

WAMPCRA: Only derive key when `salt` present in Challenge #35

Closed om26er closed 3 years ago

om26er commented 3 years ago

When using WAMPCRA, if the server does not send a salt in the challenge the Client should not try to derive the key, this is broken. It seems connectanum-dart does not check if salt is present in server's response and always tries to derive the key. This of course means the authentication fails

Here is the relevant code https://github.com/konsultaner/connectanum-dart/blob/910038b106c571abfbe789fd17b7d72eefcbc337/lib/src/authentication/cra_authentication.dart#L50

om26er commented 3 years ago

Below is a basic crossbar config that can be used to reproduce the bug

{
    "$schema": "https://raw.githubusercontent.com/crossbario/crossbar/master/crossbar.json",
    "version": 2,
    "controller": {
    },
    "workers": [
        {
            "type": "router",
            "realms": [
                {
                    "name": "realm1",
                    "roles": [
                        {
                            "name": "anonymous",
                            "permissions": [
                                {
                                    "uri": "",
                                    "match": "prefix",
                                    "allow": {
                                        "call": true,
                                        "register": true,
                                        "publish": true,
                                        "subscribe": true
                                    },
                                    "disclose": {
                                        "caller": false,
                                        "publisher": false
                                    },
                                    "cache": true
                                }
                            ]
                        }
                    ]
                }
            ],
            "transports": [
                {
                    "type": "websocket",
                    "endpoint": {
                        "type": "tcp",
                        "port": 8080,
                        "backlog": 1024
                    },
                    "serializers": [
                        "cbor", "msgpack", "json"
                    ],
                    "auth": {
                        "wampcra": {
                           "type": "static",
                           "users": {
                              "john": {
                                 "secret": "williamsburg",
                                 "role": "anonymous"
                              }
                           }
                        }
                    }
                }
            ]
        }
    ]
}
om26er commented 3 years ago

Here is how different libraries deal with that

autobahn-java: https://github.com/crossbario/autobahn-java/blob/6ab8a3f389c9d46706fd7a41434b0f3ea01d3642/autobahn/src/main/java/io/crossbar/autobahn/wamp/auth/ChallengeResponseAuth.java#L72

autobahn-python: https://github.com/crossbario/autobahn-python/blob/a35f22eeaafca7568f1deb35c4a1b82ae78f77d4/autobahn/wamp/auth.py#L359

konsultaner commented 3 years ago

@oberstet why do you allow unsalted cra authentication in WAMP?

https://github.com/crossbario/autobahn-python/blob/a35f22eeaafca7568f1deb35c4a1b82ae78f77d4/autobahn/wamp/auth.py#L359

In autobahn python, the client does not use pbkdf2 when the server did not send a salt. Shouldn't that result into an error? I'm not sure how to handle that. In this lib I use a blank salt and the will eventually fail the authentication. Which @om26er thinks is a broken implementation.

This is probably somehow related to https://github.com/wamp-proto/wamp-proto/issues/385

om26er commented 3 years ago

To add some clarity: This issue is different from the linked wamp-proto issue.

Unsalted CRA is part of the WAMP proto, it is obviously "hack-prone" given passwords are saved plaintext and a database-level hack could reveal passwords. However on the wire, it's safe because the secret never travels, it's the signed challenge that the client returns.

oberstet commented 3 years ago

@konsultaner

why do you allow unsalted cra authentication in WAMP?

historical reasons (that's what's been implemented first .. unsalted plain password use for challenge signing) and choice (it is a user decision which method to use)

Shouldn't that result into an error?

no. it should just compute the signature without any pbkdf2 involved (compute the HMAC-SHA256 using the shared secret over the challenge)

when salt/iterations/keylen are present, the secret in use in above is transformed through pbkdf2 using the salt before using in signing the challenge

https://github.com/wamp-proto/wamp-proto/blob/master/rfc/text/advanced/ap_authentication_cra.md#password-salting

I think the text could be made much more straight and direct: we should add 2 formulas for unsalted / salted for signature. only the salted version refers pbkdf2(salt, keylen, iterations)

@om26er

Unsalted CRA is part of the WAMP proto, it is obviously "hack-prone"

konsultaner commented 3 years ago

ok, I'll fix it then. @oberstet @om26er Thanks for clearing!

oberstet commented 3 years ago

also, found it, here are some notes rgd salted/unsalted and the sec aspects. also scram. not sure, if haven't yet, such text would also be good to have in the spec IMO

https://github.com/wamp-proto/wamp-proto/issues/128#issuecomment-74268272

konsultaner commented 3 years ago

@oberstet thats true. Its way more detailed than the current docs.

konsultaner commented 3 years ago

@om26er could you confim that this is fixed in 1.1.8. I don't have a test vector for unsalted cra. so I assumed my test is right now.

oberstet commented 3 years ago

sounds great! fwiw, here are some test vectors we use for PBKDF2 - that is what is used with salted WAMP-CRA

https://github.com/crossbario/autobahn-python/blob/a35f22eeaafca7568f1deb35c4a1b82ae78f77d4/autobahn

we should add WAMP-CRA level tests (salted and unsalted) using the vectors Richard added here ..

konsultaner commented 3 years ago

we should add WAMP-CRA level tests (salted and unsalted) using the vectors

That would be great!