diff options
Diffstat (limited to 'doc/development/secure_coding_guidelines.md')
| -rw-r--r-- | doc/development/secure_coding_guidelines.md | 83 |
1 files changed, 78 insertions, 5 deletions
diff --git a/doc/development/secure_coding_guidelines.md b/doc/development/secure_coding_guidelines.md index 8a86a46d1d3..3e46891d20e 100644 --- a/doc/development/secure_coding_guidelines.md +++ b/doc/development/secure_coding_guidelines.md @@ -461,7 +461,9 @@ References: ### Description -Path Traversal vulnerabilities grant attackers access to arbitrary directories and files on the server that is executing an application, including data, code or credentials. +Path Traversal vulnerabilities grant attackers access to arbitrary directories and files on the server that is executing an application. This data can include data, code or credentials. + +Traversal can occur when a path includes directories. A typical malicious example includes one or more `../`, which tells the file system to look in the parent directory. Supplying many of them in a path, for example `../../../../../../../etc/passwd`, usually resolves to `/etc/passwd`. If the file system is instructed to look back to the root directory and can't go back any further, then extra `../` are ignored. The file system then looks from the root, resulting in `/etc/passwd` - a file you definitely do not want exposed to a malicious attacker! ### Impact @@ -510,6 +512,44 @@ requires :file_path, type: String, file_path: true Absolute paths are not allowed by default. If allowing an absolute path is required, you need to provide an array of paths to the parameter `allowlist`. +### Misleading behavior + +Some methods used to construct file paths can have non-intuitive behavior. To properly validate user input, be aware +of these behaviors. + +#### Ruby + +The Ruby method [`Pathname.join`](https://ruby-doc.org/stdlib-2.7.4/libdoc/pathname/rdoc/Pathname.html#method-i-join) +joins path names. Using methods in a specific way can result in a path name typically prohibited in +normal use. In the examples below, we see attempts to access `/etc/passwd`, which is a sensitive file: + +```ruby +require 'pathname' + +p = Pathname.new('tmp') +print(p.join('log', 'etc/passwd', 'foo')) +# => tmp/log/etc/passwd/foo +``` + +Assuming the second parameter is user-supplied and not validated, submitting a new absolute path +results in a different path: + +```ruby +print(p.join('log', '/etc/passwd', '')) +# renders the path to "/etc/passwd", which is not what we expect! +``` + +#### Golang + +Golang has similar behavior with [`path.Clean`](https://pkg.go.dev/path#example-Clean). Remember that with many file systems, using `../../../../` traverses up to the root directory. Any remaining `../` are ignored. This example may give an attacker access to `/etc/passwd`: + +```golang +path.Clean("/../../etc/passwd") +// renders the path to "etc/passwd"; the file path is relative to whatever the current directory is +path.Clean("../../etc/passwd") +// renders the path to "../../etc/passwd"; the file path will look back up to two parent directories! +``` + ## OS command injection guidelines Command injection is an issue in which an attacker is able to execute arbitrary commands on the host @@ -620,7 +660,7 @@ cfg := &tls.Config{ } ``` -For **Ruby**, you can use [HTTParty](https://github.com/jnunemaker/httparty) and specify TLS 1.3 version as well as ciphers: +For **Ruby**, you can use [`HTTParty`](https://github.com/jnunemaker/httparty) and specify TLS 1.3 version as well as ciphers: Whenever possible this example should be **avoided** for security purposes: @@ -665,7 +705,7 @@ tls.Config{ This example was taken [here](https://gitlab.com/gitlab-org/cluster-integration/gitlab-agent/-/blob/871b52dc700f1a66f6644fbb1e78a6d463a6ff83/internal/tool/tlstool/tlstool.go#L72). -For **Ruby**, you can use again [HTTParty](https://github.com/jnunemaker/httparty) and specify this time TLS 1.2 version alongside with the recommended ciphers: +For **Ruby**, you can use again [`HTTParty`](https://github.com/jnunemaker/httparty) and specify this time TLS 1.2 version alongside with the recommended ciphers: ```ruby response = GitLab::HTTP.perform_request(Net::HTTP::Get, 'https://gitlab.com', ssl_version: :TLSv1_2, ciphers: ['ECDHE-ECDSA-AES128-GCM-SHA256', 'ECDHE-RSA-AES128-GCM-SHA256', 'ECDHE-ECDSA-AES256-GCM-SHA384', 'ECDHE-RSA-AES256-GCM-SHA384', 'ECDHE-ECDSA-CHACHA20-POLY1305', 'ECDHE-RSA-CHACHA20-POLY1305']) @@ -833,7 +873,7 @@ If a vulnerable application extracts an archive file with any of these file name #### Ruby -For zip files, the [rubyzip](https://rubygems.org/gems/rubyzip) Ruby gem is already patched against the Zip Slip vulnerability and will refuse to extract files that try to perform directory traversal, so for this vulnerable example we will extract a `tar.gz` file with `Gem::Package::TarReader`: +For zip files, the [`rubyzip`](https://rubygems.org/gems/rubyzip) Ruby gem is already patched against the Zip Slip vulnerability and will refuse to extract files that try to perform directory traversal, so for this vulnerable example we will extract a `tar.gz` file with `Gem::Package::TarReader`: ```ruby # Vulnerable tar.gz extraction example! @@ -1032,7 +1072,7 @@ Symlink attacks makes it possible for an attacker to read the contents of arbitr #### Ruby -For zip files, the [rubyzip](https://rubygems.org/gems/rubyzip) Ruby gem is already patched against symlink attacks as it simply ignores symbolic links, so for this vulnerable example we will extract a `tar.gz` file with `Gem::Package::TarReader`: +For zip files, the [`rubyzip`](https://rubygems.org/gems/rubyzip) Ruby gem is already patched against symlink attacks as it simply ignores symbolic links, so for this vulnerable example we will extract a `tar.gz` file with `Gem::Package::TarReader`: ```ruby # Vulnerable tar.gz extraction example! @@ -1210,3 +1250,36 @@ An example of well implemented `Gitlab::UrlBlocker.validate!` call that prevents ### Resources - [CWE-367: Time-of-check Time-of-use (TOCTOU) Race Condition](https://cwe.mitre.org/data/definitions/367.html) + +## Handling credentials + +Credentials can be: + +- Login details like username and password. +- Private keys. +- Tokens (PAT, runner tokens, JWT token, CSRF tokens, project access tokens, etc). +- Session cookies. +- Any other piece of information that can be used for authentication or authorization purposes. + +This sensitive data must be handled carefully to avoid leaks which could lead to unauthorized access. If you have questions or need help with any of the following guidance, talk to the GitLab AppSec team on Slack (`#sec-appsec`). + +### At rest + +- Credentials must be encrypted while at rest (database or file) with `attr_encrypted`. See [issue #26243](https://gitlab.com/gitlab-org/gitlab/-/issues/26243) before using `attr_encrypted`. + - Store the encryption keys separately from the encrypted credentials with proper access control. For instance, store the keys in a vault, KMS, or file. Here is an [example](https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/models/user.rb#L70-74) use of `attr_encrypted` for encryption with keys stored in separate access controlled file. + - When the intention is to only compare secrets, store only the salted hash of the secret instead of the encrypted value. +- Never commit credentials to repositories. + - The [Gitleaks Git hook](https://gitlab.com/gitlab-com/gl-security/security-research/gitleaks-endpoint-installer) is recommended for preventing credentials from being committed. +- Never log credentials under any circumstance. Issue [#353857](https://gitlab.com/gitlab-org/gitlab/-/issues/353857) is an example of credential leaks through log file. +- When credentials are required in a CI/CD job, use [masked variables](../ci/variables/index.md#mask-a-cicd-variable) to help prevent accidental exposure in the job logs. Be aware that when [debug logging](../ci/variables/index.md#debug-logging) is enabled, all masked CI/CD variables are visible in job logs. Also consider using [protected variables](../ci/variables/index.md#protected-cicd-variables) when possible so that sensitive CI/CD variables are only available to pipelines running on protected branches or protected tags. +- Proper scanners must be enabled depending on what data those credentials are protecting. See the [Application Security Inventory Policy](https://about.gitlab.com/handbook/engineering/security/security-engineering-and-research/application-security/inventory.html#policies) and our [Data Classification Standards](https://about.gitlab.com/handbook/engineering/security/data-classification-standard.html#data-classification-standards). +- To store and/or share credentials between teams, refer to [1Password for Teams](https://about.gitlab.com/handbook/security/#1password-for-teams) and follow [the 1Password Guidelines](https://about.gitlab.com/handbook/security/#1password-guidelines). +- If you need to share a secret with a team member, use 1Password. Do not share a secret over email, Slack, or other service on the Internet. + +### In transit + +- Use an encrypted channel like TLS to transmit credentials. See [our TLS minimum recommendation guidelines](#tls-minimum-recommended-version). +- Avoid including credentials as part of an HTTP response unless it is absolutely necessary as part of the workflow. For example, generating a PAT for users. +- Avoid sending credentials in URL parameters, as these can be more easily logged inadvertently during transit. + +In the event of credential leak through an MR, issue, or any other medium, [reach out to SIRT team](https://about.gitlab.com/handbook/engineering/security/security-operations/sirt/#-engaging-sirt). |