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

Add spoofing protection (MITM/NO-CPS detection) #180

Merged
merged 5 commits into from
May 10, 2020

Conversation

alexhauser
Copy link
Member

Closes #117.

Description:

This adds detection and mitigation for the following scenarios:

  • The server reports an IP address mismatch (TIF of 0x40, "IPs matched" flag off)
    In this case, we show a warning message and fail hard (just like the GRC client).

  • The server fails to engage CPS
    In this case, we show a warning message and give the user the choice of aborting the authentication or continuing on (just like the GRC client).

Additional changes:

  • Hardening of the PathConf.LoadConfig() method since I was seeing a NPE in one particular case

  • As discussed in Detect No CPS/ MITM Mitigation according to Identity Specs #117, I've changed it so that we are now only sending the cps option if the server has already engaged a CPS session. What I was seeing when I reported that the GRC client sends cps right from the start makes sense, since CPS is usually so quickly engaged, that it will come in well before the "sqrl://" invocation, and so CPS is already there when we start sending commands to the server.
    On the spoofing demo site however, one can see that the GRC client in fact does NOT send cps if the server fails to engage CPS. So that's what we're now doing as well.

Preview:

mitm_mitigation

@alexhauser alexhauser added this to the v0.1.3.0-beta milestone May 10, 2020
@alexhauser alexhauser requested a review from josegomez May 10, 2020 08:28
@alexhauser alexhauser self-assigned this May 10, 2020
@alexhauser alexhauser added client Affects the Client project enhancement New feature or request security Security-related issue or pull request labels May 10, 2020
Copy link
Collaborator

@josegomez josegomez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice work, if you get a chance could you fix my last commits localization stuff since you are already in there?
Otherwise we can do it later no big

Merge away!!

@alexhauser
Copy link
Member Author

if you get a chance could you fix my last commits localization stuff since you are already in there?

I have a few other translations that need some attention as well (mainly the PDF stuff), I think I'll tackle these toghether with yours in a separate PR.

@alexhauser alexhauser merged commit 373ed99 into sqrldev:master May 10, 2020
@alexhauser alexhauser deleted the mitm_warning branch May 13, 2020 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client Affects the Client project enhancement New feature or request security Security-related issue or pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Detect No CPS/ MITM Mitigation according to Identity Specs
2 participants