Improve File System Path Handling

[moloch--]

What type of PR is this? (check all applicable)

• Refactor
• Feature
• Bug Fix
• Optimization
• Documentation Update
• Go Version Update
• Dependency Update

Added/updated tests?

• Yes
• No, and this is why: no additional tests needed, small fix
• I need help with writing tests

Run verifications and test

• make verify is passing
• make test is passing

PASS
coverage: 78.1% of statements
ok  	github.com/gorilla/sessions	1.155s	coverage: 78.1% of statements

[hdm]

Related: https://labs.watchtowr.com/palo-alto-putting-the-protecc-in-globalprotect-cve-2024-3400/

thanks @moloch-- !

Lots of projects are using FilesystemStore: https://github.com/search?q=sessions.NewFilesystemStore+language%3Ago+&type=code

[coreydaley]

@AlexVulaj @apoorvajagtap ptal, this is a fix for a CVE that needs addressed asap.

[moloch--]

I'm not sure what the project's stance is on adding dependencies, but long term it may be a good idea to switch to using Google's safe open

[FiloSottile]

This might still be a worthwhile robustness fix, but note that the FilesystemStore implementation uses securecookie to authenticate the session ID, so the issue is not actually exploitable, and there might actually not be a vulnerability in gorilla/sessions at all.

sessions/store.go

Line 191 in 3eed1c4

err = securecookie.DecodeMulti(name, c.Value, &session.ID, s.Codecs...)

The Palo Alto vulnerability described in https://labs.watchtowr.com/palo-alto-putting-the-protecc-in-globalprotect-cve-2024-3400/ seems to be in a different internal Store implementation called SessDiskStore, which works similarly (maybe derived from FilesystemStore) but does not authenticate the session ID.

[apoorvajagtap]

Thank you everyone, for the efforts on this issue, and for bringing that to our attention via multiple channels. Appreciate it!
The changes LGTM. I'll await another review from @AlexVulaj &/or @jaitaiwan, and post that we'll proceed with the merging & next steps.

[GossiTheDog]

Do you have a path to getting a CVE assigned?

[jaitaiwan]

I'm not aware of any path for that, this side of Repo Maint. is fairly new to me. The other maintainers may have a better idea.

[SISheogorath]

It's easy to get a CVE from GitHub. Just open a security Security Advisory in the security tab and you can request a CVE :)

https://docs.github.com/en/code-security/security-advisories/working-with-repository-security-advisories/creating-a-repository-security-advisory

[FiloSottile]

Do you have a path to getting a CVE assigned?

There is no vulnerability in github.com/gorilla/sessions. The vulnerability is in Palo Alto's SessDiskStore. That Store implementation looks similar to FilesystemStore, so it led early analysis to the mistaken conclusion that the vulnerable path was in FilesystemStore, but it's not. No CVE should be assigned for github.com/gorilla/sessions.

[GossiTheDog]

Do you have a path to getting a CVE assigned?

There is no vulnerability in github.com/gorilla/sessions. The vulnerability is in Palo Alto's SessDiskStore. That Store implementation looks similar to FilesystemStore, so it led early analysis to the mistaken conclusion that the vulnerable path was in FilesystemStore, but it's not. No CVE should be assigned for github.com/gorilla/sessions.

If there’s no vulnerability why are we merging a pull called “Fix path traversal”?

[FiloSottile]

Because the name of the PR was presumably based on that early analysis.

If we filed CVEs for every API that could be misused (with no evidence of that happening in the wild) everything would have active CVEs. OTOH, if we refused to merge PRs that make APIs more robust without a CVE, there would eventually be more downstream CVEs, so merging this is good.

[SISheogorath]

You are both right.

To our current knowledge the library wasn't really vulnerable, if used even remotely correctly.

However, if my understanding is correct, an attacker that managed to work around the correct usage of the library e.g. due to a default session key, they might be able to smuggle a path traversal into the session store.

Therefore adding a CVE for this CWE might still be appropriate. Also CVEs don't hurt, they are tools to peovide visibility for people who don't know much about the library and adding information into risk assessments.

See the recent changes on how the linux kernel handles CVEs.

[FiloSottile]

I generally believe over-broad or imprecise CVEs do hurt, because they add noise that can drown the signal necessary for risk assessment. I agree that for vulnerabilities it's better to lean on the side of filing a CVE, but this one isn't one.

When you say a "default session key" you mean passing a fixed compromised key to NewFilesystemStore? By that argument all of securecookie would be vulnerable to a myriad of CVEs, because if you pass bad keys to securecookie.CodecsFromPairs an attacker can falsify cookies, but I wouldn't file a CVE in securecookie that notifies all users of the package.

Again, this whole thread started because we initially thought the issue being fixed led to the real-world impact on Palo Alto machines, but we later realized that's not the case.

Anyway, ultimately I am neither a maintainer not the reporter nor a CNA, so this is just my opinion, based on wanting to avoid unnecessary alarm.

[tmcqueen-materials]

As an observer chiming in here. Even though FileSystemStore uses securecookie, if the key used for the securecookie is "well-known" or "fixed", then the code using gorilla/sessions will be vulnerable to this same type of path traversal (the provided SESSID for an attack just needs to be the malicious path run through the securecookie encoder with the "well-known"/"fixed" key). A quick perusal of the github projects using NewFileSystemStore suggests that this is not uncommon, e.g.:

https://github.com/meshery/meshery/blob/4365cdf5d2b71e27475ce17a401339c06e3699c8/server/cmd/main.go#L155

https://github.com/gen0cide/laforge/blob/cecf012af35deef239c84ea2d5fc0c760a900411/graphql/auth/gothic.go#L47

Many do get it right, but quite a number do not. I have not checked whether instances such as these are actually exploitable, but they appear to be so since the securecookie's are forgeable for them (which is arguably the actual underlying vulnerability). So I know this fix is already merged, but definitely a good defense in depth measure.

Edit: DOH! I see this was commented on in the last two comments above.

[hdm]

My 0.02 is that this probably doesn't need a CVE since it requires some other misconfiguration to exploit (leaked session secret or intentionally removed session ID validation). Those application-specific misconfigurations should receive a CVE, but not the defense-in-depth improvement covered in this PR.

[rhowe]

As the person who led to this getting attention in the first place, I don't think it warrants a CVE and the title of the MR should probably have been reworded prior to being merged but so be it - it wouldn't be the first git commit to have a misleading commit message in the world and it won't be the last.
Sorry for the noise, folks.

[schooldropout1337]

OK Solved, thanks gents.