-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Code] stop persist random path in es #47923
Conversation
💔 Build Failed |
Pinging @elastic/code (Team:Code) |
💚 Build Succeeded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to get rid of the random string in ES. Unclear whether this causes it to re-initiate the workspace all the time. Doesn't look like it, but don't have the cycles to verify. :)
@@ -110,6 +110,12 @@ export class WorkspaceHandler { | |||
}; | |||
} | |||
|
|||
private randomPath() { | |||
return Math.random() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unlikely to be a problem in the context, but this is not a secure random source. Might as well use one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
were you suggesting using lib like this?
https://www.npmjs.com/package/secure-random
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexbrasetvik Any suggestions ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use https://nodejs.org/dist/latest-v12.x/docs/api/crypto.html#crypto_crypto_randombytes_size_callback to generate random bytes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, thanks.
@elasticmachine merge upstream |
💚 Build Succeeded |
💚 Build Succeeded |
Summary
Resolve https://github.com/elastic/code/issues/1649
Prior to this PR, we generated a random string for each repo after cloning and stored it in the ES. Later, we use this random string as part of the workspace checkout path. Using this method, we can somehow improve the difficulty of a bad user using an RCE vulnerability to exploit Code to inject code. But this method won't help for those who have access to ES data.
Now we no longer save the random string in the ES, but use the internal storage of git. Note that this mechanism itself does not guarantee that the Code is not used as an exploit in the case of any other RCE is found, just to improve the difficulty a little.
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.~- [ ] This was checked for cross-browser compatibility, including a check against IE11~~
- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support- [ ] Documentation was added for features that require explanation or tutorials- [ ] Unit or functional tests were updated or added to match the most common scenarios- [ ] This was checked for keyboard-only and screenreader accessibilityFor maintainers
- [ ] This was checked for breaking API changes and was labeled appropriately- [ ] This includes a feature addition or change that requires a release note and was labeled appropriately