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.

Sign up for GitHub

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

Jump to bottom

[FIX] SAML assertion signature enforcement #17278

Merged
merged 7 commits into from
Apr 14, 2020
Merged

[FIX] SAML assertion signature enforcement #17278

merged 7 commits into from
Apr 14, 2020

Conversation

pierre-lehnen-rc
Copy link
Contributor

@pierre-lehnen-rc pierre-lehnen-rc commented Apr 13, 2020 •
edited
Loading

Closes #17277

PR #16922 improved the security of our SAML implementation after some possible vulnerabilities were reported, but in the process it also broke compatibility with IdPs that don't sign assertions.

This PR adds a new setting to determine which signatures need to be validated. It defaults to both Response's and Assertion's, but a migration changes all existing instances to default to either one.

@sampaiodiego sampaiodiego added this to the 3.1.1 milestone Apr 13, 2020
@Exordian
Copy link
Contributor

Thanks for the fast PR !
Taking a look, I wouldn't add 'None' as a valid option. 'None' would only be fine if Rocket.Chat would support Artifact binding (which is not supported to be best of my knowledge). When using 'None' with redirect or POST binding there is no validation at all, leading to a huge security risk for people who "just make it work".

@Exordian
Copy link
Contributor

I'm also recommending rocketchat to others for various reasons. However, since the first fix seems to be the result of a private messaged vulnerability, I'm a bit shocked about the communication.
While I suspected a possible vulnerability this fix could also have fixed some sort of other behavior. If it was possible to bypass authentication I would suggest to notify your community and to update ASAP. Blackhats are looking for changes similar to the one mentioned in 3.1.0 and try to reverse your changes anyway. So I think your community is safer if they get informed asap rather then hoping that they update faster than exploits will spread - at least I would favour this. Futhermore, as an admin I rather watch CVEs than typical release changelogs. I would appreciate if RocketChat would issue CVE numbers for vulnerabilies.

I'm happy with your work and try to favor Rocket.Chat over alternative solutions. Yet, security and privacy are part of the reasons to choose Rocket.Chat.

@pierre-lehnen-rc
Copy link
Contributor Author

I've mentioned your concerns to the team and started a discussion about it. I'll keep you updated.

@cb3inco
Copy link

cb3inco commented Apr 14, 2020

Just had this blow up on my too, glad to see the pull request accepted, but hope it's released very soon as all our user accounts are SAML based.

@cb3inco
Copy link

cb3inco commented Apr 14, 2020

Just had this blow up on my too, glad to see the pull request accepted, but hope it's released very soon as all our user accounts are SAML based.

Resolved this by signing the response as our assertions were signed already. Apparently the assertions were not enough.

@makibras
Copy link

Hi @Exordian
a quick comment about your valid point about notification of the community: we want to notify as soon as possible. We will soon implement a notification banner inside RC to inform admins (not all users) when a new update is released for their version, linking to release notes.
About CVEs: we have had internal meetings and are considering it seriously. As a drawback, it is an additional administrative process and we have to align it with responsible disclosure periods from our bug bounty program.
Thank you very much for your valuable comments! 🙂

@sampaiodiego sampaiodiego changed the title [FIX] SAML assertion signature required after 3.1.0 [FIX] SAML assertion signature enforcement Apr 14, 2020
@sampaiodiego sampaiodiego merged commit b9f7161 into develop Apr 14, 2020
@sampaiodiego sampaiodiego deleted the fix.saml-assertion-signature branch April 14, 2020 20:00
sampaiodiego pushed a commit that referenced this pull request Apr 14, 2020
@pierre-lehnen-rc @sampaiodiego
[FIX] SAML assertion signature enforcement (#17278)
e12a3e6
@sampaiodiego sampaiodiego mentioned this pull request Apr 14, 2020
Merged
gabriellsh added a commit that referenced this pull request Apr 15, 2020
@gabriellsh
Merge branch 'develop' of github.com:RocketChat/Rocket.Chat into adm/…
8739a89 
…mailer

* 'develop' of github.com:RocketChat/Rocket.Chat: (93 commits)
  [FIX] Red color error outline is not removed after password update (#16536)
  [FIX] Change wording to start DM from info panel (#8799)
  New hooks for RouterContext (#17305)
  [FIX] SAML assertion signature enforcement (#17278)
  [FIX] LDAP users lose session on refresh (#17302)
  [NEW] Add MMS support to Voxtelesys (#17217)
  [FIX] Popover component doesn't have scroll (#17198)
  [FIX] Omnichannel SMS / WhatsApp integration errors due to missing location data (#17288)
  [FIX] User search on directory not working correctly (#17299)
  [FIX] Can not save Unread Tray Icon Alert user preference (#16288) (#16313)
  [FIX] Variable rendering problem on Import recent history page (#15997)
  [FIX] Admin panel custom sounds, multiple sound playback fix and added single play/pause button (#16215)
  [FIX] Discussions created from inside DMs were not working (#17282)
  [FIX] translation for nl (#16742)
  [FIX] No maxlength defined for custom user status (#16534)
  [FIX] Directory users email sort button (#16606)
  [FIX] In Create a New Channel, input should be focused on channel name instead of invite users (#16405)
  [FIX] Email not verified message (#16236)
  [FIX] Directory default tab (#17283)
  Update ru.i18n.json (#16869)
  ...
gabriellsh added a commit that referenced this pull request Apr 15, 2020
@gabriellsh
Merge branch 'develop' of github.com:RocketChat/Rocket.Chat into adm/…
e792cba 
…users_and_rooms

* 'develop' of github.com:RocketChat/Rocket.Chat:
  [FIX] "Invalid Invite" message when registration is disabled (#17226)
  [FIX] Red color error outline is not removed after password update (#16536)
  [FIX] Change wording to start DM from info panel (#8799)
  New hooks for RouterContext (#17305)
  [FIX] SAML assertion signature enforcement (#17278)
  [FIX] LDAP users lose session on refresh (#17302)
  [NEW] Add MMS support to Voxtelesys (#17217)
  [FIX] Popover component doesn't have scroll (#17198)
ggazzo added a commit that referenced this pull request Apr 15, 2020
@ggazzo
Merge branch 'adm/mailer' of github.com:RocketChat/Rocket.Chat into a…
8ba9fb0 
…dm/mailer

* 'adm/mailer' of github.com:RocketChat/Rocket.Chat: (79 commits)
  removed unecessary prop
  Fixed sendMail function
  Removed Log, Success message
  Permissions
  added route component
  [FIX] "Invalid Invite" message when registration is disabled (#17226)
  Added route, error handling.
  [FIX] Red color error outline is not removed after password update (#16536)
  [FIX] Change wording to start DM from info panel (#8799)
  New hooks for RouterContext (#17305)
  [FIX] SAML assertion signature enforcement (#17278)
  [FIX] LDAP users lose session on refresh (#17302)
  [NEW] Add MMS support to Voxtelesys (#17217)
  [FIX] Popover component doesn't have scroll (#17198)
  [FIX] Omnichannel SMS / WhatsApp integration errors due to missing location data (#17288)
  [FIX] User search on directory not working correctly (#17299)
  [FIX] Can not save Unread Tray Icon Alert user preference (#16288) (#16313)
  [FIX] Variable rendering problem on Import recent history page (#15997)
  [FIX] Admin panel custom sounds, multiple sound playback fix and added single play/pause button (#16215)
  [FIX] Discussions created from inside DMs were not working (#17282)
  ...
dudizilla added a commit that referenced this pull request Apr 21, 2020
@dudizilla
Merge branch 'develop' of https://github.com/RocketChat/Rocket.Chat i…
960d7dd 
…nto view-logs

* 'develop' of https://github.com/RocketChat/Rocket.Chat: (31 commits)
  Fixed componentes width (#17322)
  [IMPROVE] Administration -> Mailer Rewrite to React. (#17191)
  Regression: Storybook  added flowrouter group mock (#17321)
  [NEW] Feature/custom oauth mail field and interpolation for mapped fields (#15690)
  [FIX] "Invalid Invite" message when registration is disabled (#17226)
  [FIX] Red color error outline is not removed after password update (#16536)
  [FIX] Change wording to start DM from info panel (#8799)
  New hooks for RouterContext (#17305)
  [FIX] SAML assertion signature enforcement (#17278)
  [FIX] LDAP users lose session on refresh (#17302)
  [NEW] Add MMS support to Voxtelesys (#17217)
  [FIX] Popover component doesn't have scroll (#17198)
  [FIX] Omnichannel SMS / WhatsApp integration errors due to missing location data (#17288)
  [FIX] User search on directory not working correctly (#17299)
  [FIX] Can not save Unread Tray Icon Alert user preference (#16288) (#16313)
  [FIX] Variable rendering problem on Import recent history page (#15997)
  [FIX] Admin panel custom sounds, multiple sound playback fix and added single play/pause button (#16215)
  [FIX] Discussions created from inside DMs were not working (#17282)
  [FIX] translation for nl (#16742)
  [FIX] No maxlength defined for custom user status (#16534)
  ...
@sampaiodiego sampaiodiego mentioned this pull request Apr 27, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rodrigok rodrigok rodrigok approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.1.1
Development

Successfully merging this pull request may close these issues.

SAML assertion signature required after 3.1.0
6 participants
@pierre-lehnen-rc @Exordian @cb3inco @makibras @rodrigok @sampaiodiego