Skip to content

Commit

Permalink
🔊 Replace fatal error with logging
Browse files Browse the repository at this point in the history
There appear to be ~1 incident/day caused by this IP address check,
and from the looks of it they all seems like genuine, non-malicious
usage.

In office spaces where VPNs and network changes (which result in
different IP addresses) are common, this check leads to false positives
and we've assessed the likelihood of actual attacks to be low.

* Session cookies are typically flagged as HttpOnly, so XSS to steal
  credentials/cookies is not a viable exploit path
* If an attacker can intercept the session cookie via MITM over plain
  HTTP, the DigiD conditions are not met and you really, really should
  not run anything over plain HTTP.
* If an attacker can MITM an HTTPS connection, you have far bigger
  issues, since then interecepting/modifying the actual HTTP traffic is
  much more interesting than hijacking a DigiD session.
* Initial HTTP connections (first-ever) are possible, even when using
  HSTS (which you should deploy), but can be mitigated by making sure
  your URLs are added to the preload lists vendored in browsers.
  • Loading branch information
sergei-maertens committed Dec 4, 2023
1 parent c78718b commit e423f10
Showing 1 changed file with 18 additions and 5 deletions.
23 changes: 18 additions & 5 deletions digid_eherkenning/saml2/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -170,11 +170,24 @@ def verify_saml2_response(self, response, client_ip_address):
#
authn_ip_address = authn_request["client_ip_address"]
if authn_ip_address != client_ip_address:
raise OneLogin_Saml2_ValidationError(
f"A different IP address ({authn_ip_address})"
f"was used when retrieving the AuthNRequest then for retrieving"
f" the request to the ACS ({client_ip_address}).",
OneLogin_Saml2_ValidationError.WRONG_INRESPONSETO,
# Relaxed validation - IP address changes between mobile towers are not
# necessarily a common occurrence (typically the mobile provider manages
# your mobile IP address), but what does happen is switching between wifi/VPN
# or mobile network in the user's office (or even on the go, with a Dual SIM
# set up for example). So instead of complicating the error messages for
# these edge cases and given the very low likelyhook an attacker is able to
# steal the session cookie/data, we opt for detection and logging instead.
logger.warning(
"A different IP address (%s) was used when retrieving the AuthNRequest "
"than for resolving the SAML artifact in the ACS (%s).",
authn_ip_address,
client_ip_address,
# record meta information for potential audit trails
extra={
"authn_ip_address": authn_ip_address,
"client_ip_address": client_ip_address,
"security": True,
},
)

# I remember reading somewhere that the assurance level on the response
Expand Down

0 comments on commit e423f10

Please sign in to comment.