subspacecommunity / subspace

A fork of the simple WireGuard VPN server GUI community maintained
MIT License
1.8k stars 131 forks source link

saml@0.3.0 vulnerability: Authentication Bypass #167

Closed asabirov closed 3 years ago

asabirov commented 3 years ago

I checked the dependencies through a vulnerability scanner:

✗ High severity vulnerability found in github.com/russellhaering/goxmldsig
  Description: Signature Validation Bypass
  Info: https://snyk.io/vuln/SNYK-GOLANG-GITHUBCOMRUSSELLHAERINGGOXMLDSIG-1014528
  Introduced through: github.com/crewjam/saml@0.3.1, github.com/crewjam/saml/samlsp@0.3.1
  From: github.com/crewjam/saml@0.3.1 > github.com/russellhaering/goxmldsig@#7acd5e4a6ef7
  From: github.com/crewjam/saml/samlsp@0.3.1 > github.com/crewjam/saml@0.3.1 > github.com/russellhaering/goxmldsig@#7acd5e4a6ef7
  Fixed in: 1.1.0

✗ High severity vulnerability found in github.com/dgrijalva/jwt-go
  Description: Access Restriction Bypass
  Info: https://snyk.io/vuln/SNYK-GOLANG-GITHUBCOMDGRIJALVAJWTGO-596515
  Introduced through: github.com/crewjam/saml/samlsp@0.3.1
  From: github.com/crewjam/saml/samlsp@0.3.1 > github.com/dgrijalva/jwt-go@3.2.0
  Fixed in: 4.0.0-preview1

✗ High severity vulnerability found in github.com/crewjam/saml/samlsp
  Description: Authentication Bypass
  Info: https://snyk.io/vuln/SNYK-GOLANG-GITHUBCOMCREWJAMSAMLSAMLSP-1052759
  Introduced through: github.com/crewjam/saml/samlsp@0.3.1
  From: github.com/crewjam/saml/samlsp@0.3.1
  Fixed in: 0.4.3

✗ High severity vulnerability found in github.com/crewjam/saml
  Description: Authentication Bypass
  Info: https://snyk.io/vuln/SNYK-GOLANG-GITHUBCOMCREWJAMSAML-1052715
  Introduced through: github.com/crewjam/saml@0.3.1, github.com/crewjam/saml/samlsp@0.3.1
  From: github.com/crewjam/saml@0.3.1
  From: github.com/crewjam/saml/samlsp@0.3.1 > github.com/crewjam/saml@0.3.1
  Fixed in: 0.4.3

It's better to upgrade crewjam/saml to 0.4.3+. There are several significant changes in API, so it wont be easy https://github.com/crewjam/saml/compare/v0.3.1...v0.4.5

metalcated commented 3 years ago

SAML has an issue with authentication as it is. I brought it up almost a year ago and it’s not been addressed. Hoping someone takes this seriously.

gchamon commented 3 years ago

that is unfortunate. I am rolling out subspace in pre-production (only for the development team).

I will try to investigate this issue on my free time, but if I find myself unable to come up with an upgrade solution, I think I will be forced to move to a less feature-rich solution. SSO support was the main selling point of subspace.

I am trying to volunteer to help with subspace but I have zero response from the current maintainers. This feels like an abandoned project without formal acknowledgement.

gchamon commented 3 years ago

from https://pkg.go.dev/github.com/crewjam/saml#readme-breaking-changes

Public fields of samlsp.Middleware have changed, so some usages may require adjustment. See issue 231 for details.

https://github.com/crewjam/saml/issues/231

Subspace re-implements the SAML metadata fetching (now in ParseMetadata) and instantiates samlsp.New() with Options. This usage will be simplified by these changes, but looks like it won't break.

It makes it sound like the changes shouldn't break, but I have been investigating a little and many methods indeed cease to exist. I am still investigating how extensive should the changes be, but my lack of experience with go is hindering me a little.

gchamon commented 3 years ago

@asabirov I invite you and everyone interested in the issue to review my pull request https://github.com/subspacecommunity/subspace/pull/169

Tested the saml sso auth flow with jumpcloud after upgrading the dependency to 0.4.5 and was able to login and logout.