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

Enforce SAML assertion ID uniqueness and notValidOnOrAfter attribute [FD-37019] #14170

Merged
merged 3 commits into from
Jan 25, 2024

Conversation

uberbrady
Copy link
Collaborator

Our SAML system was ignoring the fact that assertionID's in SAML are meant to only be used once. Additionally, there's an attribute in each assertion called notOnOrAfter which needs to be respected, and we weren't.

These weren't really exploitable issues - if an attacker can sniff TLS traffic, you're already gonna have a pretty bad time. And the IdP's themselves won't certify older SAML assertions anyways. But they're issues that will come up on some customer's pen-tests, and it just doesn't look good for us to fail those. Now we shouldn't anymore.

This handles both of those, and includes a new Artisan command to clear old assertions from the table. Additionally, I added it to the kernel so that the regular scheduler will run the cleaning command weekly (which seems like enough; this is a pretty narrow table).

There was also some stuff where we were throwing a brand-new exception in our exception handler - when we already have a perfectly serviceable one that we can re-throw ourselves. So I just used that. I also made it so that these errors - which are client-side errors, not server-side errors, will now use a 400-series HTTP status code rather than 500.

Copy link

what-the-diff bot commented Jan 25, 2024

PR Summary

  • Introduction of the SAML Assertion Cleaner
    A new procedure is added which removes outdated Security Assertion Markup Language (SAML) assertions from our system. The role of such assertions is similar to a security badge to enter a building, but in our case, they're used for logging into the app.

  • SAML Assertion Cleaner's Periodic Execution
    The new cleanup procedure is scheduled to run at regular intervals - it's added to a list of tasks our app performs periodically, which ensures our system remains clean and efficient.

  • Handling the Security Assertions
    Additional steps are added during user login process. The app can now check if a SAML assertion has expired or has been used already, tie it to the specific user, and keeps track of them using log messages for easier troubleshooting.

  • Structure for Security Assertion Records
    In order to store the assertions, a new database structure is created. This structure or 'table' is made up of two parts: 'nonce' which is the actual assertion and 'not_on_or_after' which defines its expiry.

  • Changes to Data Extraction Process
    Data extracted during user login process has been extended to include details on the last security assertion used and its time-based validity.

  • Creating the Database Table for Verifications
    To implement these changes in our actual functioning database, a new update procedure or 'migration' is created. This sets up a new table composed of 'nonce' (the actual security assertion) and 'not_valid_after' (the expiry date of the assertion). These new elements within the table will be used to store and index the SAML assertions for users. This will help in efficient data retrieval and overall system performance.

@snipe
Copy link
Owner

snipe commented Jan 25, 2024

Excellent, excellent work here @uberbrady - I know this was a beast to get through.

@snipe snipe merged commit 85a158e into snipe:develop Jan 25, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants