Implement replay attack mitigation for SAML auth.

Review Request #12476 — Created July 18, 2022 and submitted

Information

Review Board
release-5.0.x

Reviewers

SAML is generally quite secure by design, but if there's a man in the
middle, it is susceptible to replay attacks (within a short limited time
frame). The way this is mitigated is by giving every message and
assertion a unique ID. The service provider can then store these IDs and
make sure that they haven't already been used.

This change implements that. The used IDs are kept in the cache, and
checked before we proceed. While using the cache for this isn't 100%
reliable, given the very short message lifetime, it's good enough for
our purposes.

  • Ran unit tests.
  • Verified that I could still log in with SAML.
  • Crafted a replay and saw that it was successfully blocked.
Summary ID
Implement replay attack mitigation for SAML auth.
SAML is generally quite secure by design, but if there's a man in the middle, it is susceptible to replay attacks (within a short limited time frame). The way this is mitigated is by giving every message and assertion a unique ID. The service provider can then store these IDs and make sure that they haven't already been used. This change implements that. The used IDs are kept in the cache, and checked before we proceed. While using the cache for this isn't 100% reliable, given the very short message lifetime, it's good enough for our purposes. Testing Done: - Ran unit tests. - Verified that I could still log in with SAML. - Crafted a replay and saw that it was successfully blocked. Reviewed at https://reviews.reviewboard.org/r/12476/
d2a809773ca78f6fc3ebd1360bb521711c33b842
Description From Last Updated

Can get rid of this TODO. Should we be checking the request ID too?

maubinmaubin

The docstring says POST but the method is GET.

maubinmaubin

Here we're checking the message and request, but in the other view we check the message and assertion. What's the …

maubinmaubin

Can get rid of this.

maubinmaubin

True is a literal here, but False is not.

chipx86chipx86

Can we go with the usual form of Testing <thing> with <conditions>? Same below.

chipx86chipx86
maubin
  1. 
      
  2. Show all issues

    Can get rid of this TODO. Should we be checking the request ID too?

    1. Based on my reading of the onelogin.saml2 library code (since there's no docs on this), the request ID doesn't get set in this code path, only the message and assertion.

  3. Show all issues

    The docstring says POST but the method is GET.

  4. Show all issues

    Here we're checking the message and request, but in the other view we check the message and assertion. What's the reason for the difference?

    Also should we have a test for this view?

    1. Just a difference in the implementation. Message happens everywhere, but request is for SLS and assertion is for ACS.

  5. 
      
david
maubin
  1. 
      
  2. reviewboard/accounts/tests/test_saml_views.py (Diff revisions 1 - 2)
     
     
     
    Show all issues

    Can get rid of this.

  3. 
      
david
chipx86
  1. 
      
  2. Show all issues

    True is a literal here, but False is not.

  3. Is there anything more we could log here that would help and in other similar logging statements?

    1. I don't think so. There are message/assertion IDs but from what I can tell that's pretty much kept as an internal implementation detail within the SAML protocol and there's not really a log of them generally. This should be enough that people can match it up to requests in their access log and track down the offending host.

  4. Show all issues

    Can we go with the usual form of Testing <thing> with <conditions>?

    Same below.

  5. 
      
david
chipx86
  1. Ship It!
  2. 
      
david
Review request changed
Status:
Completed
Change Summary:
Pushed to release-5.0.x (65840cc)