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

SASL SCRAM Downgrade Protection, support to inject specific value in optional parameter #12

Closed
edhelas opened this issue Mar 6, 2024 · 11 comments

Comments

@edhelas
Copy link

edhelas commented Mar 6, 2024

Hi, I'm currently implementing https://xmpp.org/extensions/xep-0474.html

In the flow I actually need to reuse the same mechanism used for the password for a specific string that will then be appended as an optional 'd=' parameter at the end (see https://xmpp.org/extensions/xep-0474.html#hash).

To do so I'd need to have a specific accessor to your library to re-use the mechanism that you're running internally to generate the responses.

Do you have some good ideas of how this could be done ? Or other ways that I could use properly your lib to implement this standard ?

Thanks for the good work !

Regards,

edhelas

Related to movim/movim#1275

@edhelas edhelas changed the title SASL SCRAM Downgrade Protection, support to inject specific value in optionnal parameter SASL SCRAM Downgrade Protection, support to inject specific value in optional parameter Mar 6, 2024
@fabiang
Copy link
Owner

fabiang commented Mar 7, 2024

I assume we need to pass the possible values for the mechanism and channel-binding list to Fabiang\Sasl\Authentication\SCRAM::createResponse(). When Fabiang\Sasl\Authentication\SCRAM::generateResponse() is called and d= is provided we simply return false if the SHA1 doesn't match.

In the example the order of mechanisms/channel-binding list is "SCRAM-SHA-1, SCRAM-SHA-1-PLUS, tls-server-end-point, tls-exporter", but in it says: "Attribute "d" contains base64 encoded SHA-1 hash of 'SCRAM-SHA-1,SCRAM-SHA-1-PLUS|tls-exporter,tls-server-end-point'". What is the logic here how to order this list?

@edhelas
Copy link
Author

edhelas commented Mar 18, 2024

So basically it could be your library that can take care of injecting the proper d= element ? That would be great indeed :)

For the sorting the XEP actually specifiy this

Note: All sorting operations MUST be performed using "i;octet" collation as specified in Section 9.3 of RFC 4790 [9].

@fabiang
Copy link
Owner

fabiang commented Mar 19, 2024

Yes this library should be responsible for validating the d= parameter.

I'm still unsure if we should pass the mechanism and channel-binding list via the Options class or by parameter to createResponse(). And secondly backward compatibility: if no allowed mechanism and channel-binding list is provided, ignore d= completely or return false?

@edhelas
Copy link
Author

edhelas commented Mar 19, 2024

I think it would be simpler when you actually instanciate it ? You can have a look at my usage of your library there https://github.com/movim/movim/blob/master/src/Moxl/Authentication.php

I am not sure about the second one

@fabiang
Copy link
Owner

fabiang commented Mar 25, 2024

I think it the better solution would be, to initiate the Options class with the expected values and force users of this library to create a new object for each authentication try (exactly how Movim does).

I'm gonna pick this issue up in the following days. @edhelas I assume you can test the changes?

@edhelas
Copy link
Author

edhelas commented Mar 25, 2024

Yes for sure ! :)

@fabiang
Copy link
Owner

fabiang commented Mar 27, 2024

I've implemented downgrade protection (dp) for SCRAM in branch scram-downgrade-protection. It's still optional, so if you don't setup dp correctly, you're still able to connect. The README.md given more info on how to set it up: https://github.com/fabiang/sasl/tree/scram-downgrade-protection?tab=readme-ov-file#scram-downgrade-protection

@edhelas
Copy link
Author

edhelas commented Mar 30, 2024

Thanks, I'll have a look at it !

@edhelas
Copy link
Author

edhelas commented Mar 30, 2024

Seems to work for me :) ! I made a PR with your branch integration in Movim there movim/movim#1303

I also added support for the channel-binding feature even though it is not supported in PHP for now.

Be careful you have a typo at allowed_machanisms.

Tell me if I forgot something in my implementation. I'll amend my PR once you fixed the typo, merged and taggued a new release.

Thanks a lot for the awesome work !

@fabiang
Copy link
Owner

fabiang commented Apr 2, 2024

Created a new release v1.4.0

@Neustradamus
Copy link

Neustradamus commented Apr 22, 2024

@fabiang: Thanks a lot for the new release build with your "XEP-0474: SASL SCRAM Downgrade Protection" fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants