Upgrade to v2 of github.com/alexedwards/scs

[jamestoyer]

There is a new major version of github.com/alexedwards/scs which looks like it has some breaking changes. We should either update to it or consider moving to a new package as this one appears to have not been touched in over a year

[bluekeyes]

If there's a reason to modify the session handling, I think we should look for alternate libraries. scs/v2 makes two major changes that I found difficult to work with compared to scs/v1:

• No support for cookies as a storage mechanism. While this is generally good for security, there are situations where cookies are convenient and I think make sense, like for the OAuth state parameter here. The scs.Store interface actually makes it impossible to use cookies for state because it does not give access to the http.ResponseWriter or a context.Context, so I can't think of a way to get an updated cookie into a response.
• Heavily encourages the use of the LoadAndSave middleware, which buffers responses and must appear at the highest-level of the middleware stack to work with other middleware that might write responses, like error handlers. This is easy to mess up and makes it hard to only use sessions on certain routes (at least given how we set things up in go-baseapp.) There's a lower-level interface for saving sessions, but using it requires you to reimplement all the logic to generate and write the session ID cookie.

I think gorilla/sessions is the most popular session library for Go, but I've also had problems with it (although it was probably user error that I didn't properly debug at the time.) And there might other options to consider.

[erikpaasonen]

the policy-bot repo depends on the oauth2 pkg in this repo, and the dependency on v1.4 of github.com/alexedwards/scs is causing PolicyBot to get flagged with a CVSS 9 vulnerability according to Jfrog Xray:

just providing motivation to move this issue forward.

[asvoboda]

Hi @erikpaasonen ,

Thanks for that information. What is the text of the CVE? I don't see any public information about it online. It is possible that it directly affects this repo and downstream consumers (like policy-bot), or potentially not at all.

[erikpaasonen]

summary:

rqlitestore Session Token Handling SQL Injection Authentication Bypass

description:

rqlitestore contains a flaw that may allow carrying out an SQL injection attack. The issue is due to the program not properly sanitizing input to session tokens. This may allow a remote attacker to inject or manipulate SQL queries in the back-end database, allowing for the manipulation or disclosure of arbitrary data or a bypass of authentication. Once authenticated, the attacker has access to the application with the same privileges as the admin account used during the authentication bypass.

vulnerable versions: < 2.5.1

refs:

• https://github.com/alexedwards/scs/commit/d93ace5be94bc476d79a2b818ae6579fa76e5a59
• https://huntr.dev/bounties/fa7caf37-7fe7-4c22-80eb-3a3daee7eb5a/

the screenshot above is from an Artifactory scan of our policy-bot Docker image.

policy-bot uses the oauth2 package which is hosted here (but ironically does not use the rqlitestore store).

[bluekeyes]

Thanks for the details. It looks like the rqlitestore was never included in a tagged release of scs and was introduced in between the 2.5.0 and 2.5.1 releases. I think Xray's information about the affected versions of this library is incorrect. There should be no way it could impact scs 1.4.1 and as you noted, Policy Bot only uses the cookiestore.

All that said, I am thinking about this issue again as part of building a new application, so there may be an update shortly.

[erikpaasonen]

Since realizing that the cookiestore code that PolicyBot depends on seems to have been deleted in v2, scrutiny on the embedded dependency of PolicyBot is impeding our efforts to get the Artifactory detection updated.

Are there any opportunities to move quickly off of v1.4.1 while a more permanent solution is engineered?

[asvoboda]

While we are working on a potential replacement for this internally at the moment, I'm not sure if there are any "quick wins" here.

The fact that cookiestore was removed in v2 is one of the problems outlined in https://github.com/palantir/go-githubapp/issues/103#issuecomment-910488484. I don't think its worth porting to another session library right away as a response. policy-bot is not affected by this CVE, and I don't believe the CVE is relevant unless clients use the rqlitestore.