Summary

Implement replay attack mitigation for SAML auth.

Review Request #12476 — Created July 18, 2022 and submitted Aug. 9, 2022, 12:59 p.m.

Information

Owner

david

Repository

Review Board

Branch

release-5.0.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

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.

Commits

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

Summary

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

Issues

Description	From	Last Updated
Can get rid of this TODO. Should we be checking the request ID too?	maubin	July 21, 2022, 10:28 a.m.
The docstring says POST but the method is GET.	maubin	July 21, 2022, 10:31 a.m.
Here we're checking the message and request, but in the other view we check the message and assertion. What's the …	maubin	July 21, 2022, 10:31 a.m.
Can get rid of this.	maubin	July 21, 2022, 10:43 a.m.
True is a literal here, but False is not.	chipx86	Aug. 3, 2022, 2:28 p.m.
Can we go with the usual form of Testing <thing> with <conditions>? Same below.	chipx86	Aug. 3, 2022, 2:28 p.m.

flake8 passed.

JSHint passed.

reviewboard/accounts/sso/backends/saml/views.py (Diff revision 1)

Show all issues

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

david July 21, 2022, 10:35 a.m.

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.

reviewboard/accounts/sso/backends/saml/views.py (Diff revision 1)
Show all issues
```
The docstring says POST but the method is GET.
```

reviewboard/accounts/sso/backends/saml/views.py (Diff revision 1)

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?

david July 21, 2022, 10:35 a.m.

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

Commits:

	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.	6bca24e462426e7467deb486235a2cd112f49e74
	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.	2c51fb62c47a4ad4613791ee7d50abd730bb9a29

Diff:

Revision 2 (+284 -20)

Show changes

	reviewboard/accounts/sso/backends/saml/views.py
	reviewboard/accounts/tests/test_saml_views.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

reviewboard/accounts/tests/test_saml_views.py (Diff revisions 1 - 2)
Show all issues
```
Can get rid of this.
```

Commits:

	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.	2c51fb62c47a4ad4613791ee7d50abd730bb9a29
	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.	326f734aaebef26c736d1e8cc03d8186417c7a94

Diff:

Revision 3 (+278 -20)

Show changes

	reviewboard/accounts/sso/backends/saml/views.py
	reviewboard/accounts/tests/test_saml_views.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/accounts/sso/backends/saml/views.py (Diff revision 3)
Show all issues
```
True is a literal here, but False is not.
```

reviewboard/accounts/sso/backends/saml/views.py (Diff revision 3)

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

david Aug. 3, 2022, 2:30 p.m.

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.

reviewboard/accounts/tests/test_saml_views.py (Diff revision 3)

Show all issues

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

Commits:

	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.	326f734aaebef26c736d1e8cc03d8186417c7a94
	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

Diff:

Revision 4 (+278 -20)

Show changes

	reviewboard/accounts/sso/backends/saml/views.py
	reviewboard/accounts/tests/test_saml_views.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-5.0.x (65840cc)