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

Fix vulnerability in authenticated secrets API #71

Merged
merged 3 commits into from
Apr 29, 2020

Conversation

alexellis
Copy link
Member

This patch fixes a vulnerability in the secrets API, however
it is important to stress that the user must be authenticated
as the admin user on the REST API before they can attempt this.

Reported by Appsecco via email. @LucasRoesler, Appsecco and
myself believe this to be of low severity.

The fix prevents directory traversal characters from being
used in secret names. If a secret name such as:
../../root/.ssh/authorized_keys were to be used, an attacker
could remove the value and write their own.

Signed-off-by: Alex Ellis (OpenFaaS Ltd) alexellis2@gmail.com

This patch fixes a vulnerability in the secrets API, however
it is important to stress that the user must be authenticated
as the admin user on the REST API before they can attempt this.

Reported by Appsecco via email. @LucasRoesler, Appsecco and
myself believe this to be of low severity.

The fix prevents directory traversal characters from being
used in secret names. If a secret name such as:
../../root/.ssh/authorized_keys were to be used, an attacker
could remove the value and write their own.

Tested with unit tests and tests are now made to run
via the CI and a new Makefile target.

Signed-off-by: Alex Ellis (OpenFaaS Ltd) <alexellis2@gmail.com>
@alexellis
Copy link
Member Author

@LucasRoesler please could you look at this broken test? It's currently breaking the CI, unfortunately this error wasn't detected previously because CI wasn't running "go test ./...".

Thanks

Screenshot 2020-04-29 at 09 56 06

Signed-off-by: Alex Ellis (OpenFaaS Ltd) <alexellis2@gmail.com>
Signed-off-by: Alex Ellis (OpenFaaS Ltd) <alexellis2@gmail.com>
Copy link

@abhisek abhisek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Path traversal fix looks good.

But I think the utility function which is checking an input string for path traversal characters may be required in other places in code as well, either now or in future. It might make sense to keep it in its own namespace.

@alexellis
Copy link
Member Author

alexellis commented Apr 29, 2020

Thanks for the suggestion. The way I see it is that when we have a need for other code to use the check, it can be exported or moved to another package.

@LucasRoesler has a test that needs patching for the build to pass, I'm going to merge this so that he can go ahead and send a PR for the log format.

The release will follow that PR.

@alexellis alexellis merged commit e54da61 into master Apr 29, 2020
@alexellis alexellis deleted the openfaasltd/fix-traversal branch April 29, 2020 11:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants