From 79e5c89eb78e4b0347839480864975cc703852f3 Mon Sep 17 00:00:00 2001 From: Ameya Darshan Date: Wed, 17 Sep 2025 20:09:04 +0530 Subject: [PATCH 1/9] Split guidelines per language --- .../_index.md} | 858 +---------------- .../secure_coding_guidelines/go.md | 407 +++++++++ .../secure_coding_guidelines/ruby.md | 864 ++++++++++++++++++ 3 files changed, 1273 insertions(+), 856 deletions(-) rename doc/development/{secure_coding_guidelines.md => secure_coding_guidelines/_index.md} (69%) create mode 100644 doc/development/secure_coding_guidelines/go.md create mode 100644 doc/development/secure_coding_guidelines/ruby.md diff --git a/doc/development/secure_coding_guidelines.md b/doc/development/secure_coding_guidelines/_index.md similarity index 69% rename from doc/development/secure_coding_guidelines.md rename to doc/development/secure_coding_guidelines/_index.md index c46ad02c7b35d5..151f6043f5d4c6 100644 --- a/doc/development/secure_coding_guidelines.md +++ b/doc/development/secure_coding_guidelines/_index.md @@ -108,107 +108,6 @@ When developing features that interact with or trigger pipelines, it's essential The [CI/CD development guidelines](cicd/_index.md) are essential reading material. No SAST or RuboCop rules enforce these guidelines. -## Regular Expressions guidelines - -### Anchors / Multi line in Ruby - -Unlike other programming languages (for example, Perl or Python) Regular Expressions are matching multi-line by default in Ruby. Consider the following example in Python: - -```python -import re -text = "foo\nbar" -matches = re.findall("^bar$",text) -print(matches) -``` - -The Python example will output an empty array (`[]`) as the matcher considers the whole string `foo\nbar` including the newline (`\n`). In contrast Ruby's Regular Expression engine acts differently: - -```ruby -text = "foo\nbar" -p text.match /^bar$/ -``` - -The output of this example is `#`, as Ruby treats the input `text` line by line. To match the whole **string**, the Regex anchors `\A` and `\z` should be used. - -#### Impact - -This Ruby Regex specialty can have security impact, as often regular expressions are used for validations or to impose restrictions on user-input. - -#### Examples - -GitLab-specific examples can be found in the following [path traversal](https://gitlab.com/gitlab-org/gitlab/-/issues/36029#note_251262187) -and [open redirect](https://gitlab.com/gitlab-org/gitlab/-/issues/33569) issues. - -Another example would be this fictional Ruby on Rails controller: - -```ruby -class PingController < ApplicationController - def ping - if params[:ip] =~ /^\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}$/ - render :text => `ping -c 4 #{params[:ip]}` - else - render :text => "Invalid IP" - end - end -end -``` - -Here `params[:ip]` should not contain anything else but numbers and dots. However this restriction can be easily bypassed as the Regex anchors `^` and `$` are being used. Ultimately this leads to a shell command injection in `ping -c 4 #{params[:ip]}` by using newlines in `params[:ip]`. - -#### Mitigation - -In most cases the anchors `\A` for beginning of text and `\z` for end of text should be used instead of `^` and `$`. - -### Escape sequences in Go - -When a character in a string literal or regular expression literal is preceded by a backslash, it is interpreted as part of an escape sequence. For example, the escape sequence `\n` in a string literal corresponds to a single `newline` character, and not the ` \ ` and `n` characters. - -There are two Go escape sequences that could produce surprising results. First, `regexp.Compile("\a")` matches the bell character, whereas `regexp.Compile("\\A")` matches the start of text and `regexp.Compile("\\a")` is a Vim (but not Go) regular expression matching any alphabetic character. Second, `regexp.Compile("\b")` matches a backspace, whereas `regexp.Compile("\\b")` matches the start of a word. Confusing one for the other could lead to a regular expression passing or failing much more often than expected, with potential security consequences. - -#### Examples - -The following example code fails to check for a forbidden word in an input string: - -```go -package main - -import "regexp" - -func broken(hostNames []byte) string { - var hostRe = regexp.MustCompile("\bforbidden.host.org") - if hostRe.Match(hostNames) { - return "Must not target forbidden.host.org" - } else { - // This will be reached even if hostNames is exactly "forbidden.host.org", - // because the literal backspace is not matched - return "" - } -} -``` - -#### Mitigation - -The above check does not work, but can be fixed by escaping the backslash: - -```go -package main - -import "regexp" - -func fixed(hostNames []byte) string { - var hostRe = regexp.MustCompile(`\bforbidden.host.org`) - if hostRe.Match(hostNames) { - return "Must not target forbidden.host.org" - } else { - // hostNames definitely doesn't contain a word "forbidden.host.org", as "\\b" - // is the start-of-word anchor, not a literal backspace. - return "" - } -} -``` - -Alternatively, you can use backtick-delimited raw string literals. For example, the `\b` in ``regexp.Compile(`hello\bworld`)`` matches a word boundary, not a backspace character, as within backticks `\b` is not an escape sequence. - ## Denial of Service (ReDoS) / Catastrophic Backtracking When a regular expression (regex) is used to search for a string and can't find a match, @@ -267,40 +166,6 @@ end ### Mitigation -#### Ruby from 3.2.0 - -Ruby released [Regexp improvements against ReDoS in 3.2.0](https://www.ruby-lang.org/en/news/2022/12/25/ruby-3-2-0-released/). ReDoS will no longer be an issue, with the exception of _"some kind of regular expressions, such as those including advanced features (like back-references or look-around), or with a huge fixed number of repetitions"_. - -[Until GitLab enforces a global Regexp timeout](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/145679) you should pass an explicit timeout parameter, particularly when using advanced features or a large number of repetitions. For example: - -```ruby -Regexp.new('^a*b?a*()\1$', timeout: 1) # timeout in seconds -``` - -#### Ruby before 3.2.0 - -GitLab has [`Gitlab::UntrustedRegexp`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/lib/gitlab/untrusted_regexp.rb) - which internally uses the [`re2`](https://github.com/google/re2/wiki/Syntax) library. -`re2` does not support backtracking so we get constant execution time, and a smaller subset of available regex features. - -All user-provided regular expressions should use `Gitlab::UntrustedRegexp`. - -For other regular expressions, here are a few guidelines: - -- If there's a clean non-regex solution, such as `String#start_with?`, consider using it -- Ruby supports some advanced regex features like [atomic groups](https://www.regular-expressions.info/atomic.html) - and [possessive quantifiers](https://www.regular-expressions.info/possessive.html) that eliminate backtracking -- Avoid nested quantifiers if possible (for example `(a+)+`) -- Try to be as precise as possible in your regex and avoid the `.` if there's an alternative - - For example, Use `_[^_]+_` instead of `_.*_` to match `_text here_` -- Use reasonable ranges (for example, `{1,10}`) for repeating patterns instead of unbounded `*` and `+` matchers -- When possible, perform simple input validation such as maximum string length checks before using regular expressions -- If in doubt, don't hesitate to ping `@gitlab-com/gl-security/appsec` - -#### Go - -Go's [`regexp`](https://pkg.go.dev/regexp) package uses `re2` and isn't vulnerable to backtracking issues. - #### Python Regular Expression Denial of Service (ReDoS) Prevention Python offers three main regular expression libraries: @@ -490,26 +355,11 @@ The preferred SSRF mitigations within GitLab are: #### GitLab HTTP Library -The [`Gitlab::HTTP`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/lib/gitlab/http.rb) wrapper library has grown to include mitigations for all of the GitLab-known SSRF vectors. It is also configured to respect the -`Outbound requests` options that allow instance administrators to block all internal connections, or limit the networks to which connections can be made. -The `Gitlab::HTTP` wrapper library delegates the requests to the [`gitlab-http`](https://gitlab.com/gitlab-org/gitlab/-/tree/master/gems/gitlab-http) gem. - -In some cases, it has been possible to configure `Gitlab::HTTP` as the HTTP -connection library for 3rd-party gems. This is preferable over re-implementing -the mitigations for a new feature. - -- [More details](https://dev.gitlab.org/gitlab/gitlabhq/-/merge_requests/2530/diffs) +Refer to Ruby docs #### URL blocker & validation libraries -[`Gitlab::HTTP_V2::UrlBlocker`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/gems/gitlab-http/lib/gitlab/http_v2/url_blocker.rb) can be used to validate that a -provided URL meets a set of constraints. Importantly, when `dns_rebind_protection` is `true`, the method returns a known-safe URI where the hostname -has been replaced with an IP address. This prevents DNS rebinding attacks, because the DNS record has been resolved. However, if we ignore this returned -value, we **will not** be protected against DNS rebinding. - -This is the case with validators such as the `AddressableUrlValidator` (called with `validates :url, addressable_url: {opts}` or `public_url: {opts}`). -Validation errors are only raised when validations are called, for example when a record is created or saved. If we ignore the value returned by the validation -when persisting the record, **we need to recheck** its validity before using it. For more information, see [Time of check to time of use bugs](#time-of-check-to-time-of-use-bugs). +Refer to Ruby docs #### Feature-specific mitigations @@ -714,140 +564,6 @@ References: - [Allowlist input validation](https://youtu.be/2VFavqfDS6w?t=7816) - [Content Security Policy](https://www.youtube.com/watch?v=2VFavqfDS6w&t=12991s) -## XML external entities - -### Description - -XML external entity (XXE) injection is a type of attack against an application that parses XML input. This attack occurs when XML input containing a reference to an external entity is processed by a weakly configured XML parser. It can lead to disclosure of confidential data, denial of service, server-side request forgery, port scanning from the perspective of the machine where the parser is located, and other system impacts. - -### XXE mitigation in Ruby - -The two main ways we can prevent XXE vulnerabilities in our codebase are: - -Use a safe XML parser: We prefer using Nokogiri when coding in Ruby. Nokogiri is a great option because it provides secure defaults that protect against XXE attacks. For more information, see the [Nokogiri documentation on parsing an HTML / XML Document](https://nokogiri.org/tutorials/parsing_an_html_xml_document.html#parse-options). - -When using Nokogiri, be sure to use the default or safe parsing settings, especially when working with unsanitized user input. Do not use the following unsafe Nokogiri settings ⚠️: - -| Setting | Description | -| ------ | ------ | -| `dtdload` | Tries to validate DTD validity of the object which is unsafe when working with unsanitized user input. | -| `huge` | Unsets maximum size/depth of objects that could be used for denial of service. | -| `nononet` | Allows network connections. | -| `noent` | Allows the expansion of XML entities and could result in arbitrary file reads. | - -### Safe XML Library - -```ruby -require 'nokogiri' - -# Safe by default -doc = Nokogiri::XML(xml_string) -``` - -### Unsafe XML Library, file system leak - -```ruby -require 'rexml/document' - -# Vulnerable code -xml = <<-EOX - - - ]> -&xxe; -EOX - -# Parsing XML without proper safeguards -doc = REXML::Document.new(xml) -puts doc.root.text -# This could output /etc/passwd -``` - -### Noent unsafe setting initialized, potential file system leak - -```ruby -require 'nokogiri' - -# Vulnerable code -xml = <<-EOX - - - ]> -&xxe; -EOX - -# noent substitutes entities, unsafe when parsing XML -po = Nokogiri::XML::ParseOptions.new.huge.noent -doc = Nokogiri::XML::Document.parse(xml, nil, nil, po) -puts doc.root.text # This will output the contents of /etc/passwd - -## -# User Database -# -# Note that this file is consulted directly only when the system is running -... -``` - -### Nononet unsafe setting initialized, potential malware execution - -```ruby -require 'nokogiri' - -# Vulnerable code -xml = <<-EOX - - - ]> -&xxe; -EOX - -# In this example we use `ParseOptions` but select insecure options. -# NONONET allows network connections while parsing which is unsafe, as is DTDLOAD! -options = Nokogiri::XML::ParseOptions.new(Nokogiri::XML::ParseOptions::NONONET, Nokogiri::XML::ParseOptions::DTDLOAD) - -# Parsing the xml above would allow `untrustedhost` to run arbitrary code on our server. -# See the "Impact" section for more. -doc = Nokogiri::XML::Document.parse(xml, nil, nil, options) -``` - -### Noent unsafe setting set, potential file system leak - -```ruby -require 'nokogiri' - -# Vulnerable code -xml = <<-EOX - - - ]> -&xxe; -EOX - -# setting options may also look like this, NONET disallows network connections while parsing safe -options = Nokogiri::XML::ParseOptions::NOENT | Nokogiri::XML::ParseOptions::NONET - -doc = Nokogiri::XML(xml, nil, nil, options) do |config| - config.nononet # Allows network access - config.noent # Enables entity expansion - config.dtdload # Enables DTD loading -end - -puts doc.to_xml -# This could output the contents of /etc/passwd -``` - -### Impact - -XXE attacks can lead to multiple critical and high severity issues, like arbitrary file read, remote code execution, or information disclosure. - -### When to consider - -When working with XML parsing, particularly with user-controlled inputs. - ## Path Traversal guidelines ### Description @@ -979,79 +695,6 @@ References: - [Safe Go Libraries Announcement](https://bughunters.google.com/blog/4925068200771584/the-family-of-safe-golang-libraries-is-growing) - [OWASP Path Traversal Cheat Sheet](https://owasp.org/www-community/attacks/Path_Traversal) -## OS command injection guidelines - -Command injection is an issue in which an attacker is able to execute arbitrary commands on the host -operating system through a vulnerable application. Such attacks don't always provide feedback to a -user, but the attacker can use simple commands like `curl` to obtain an answer. - -### Impact - -The impact of command injection greatly depends on the user context running the commands, as well as -how data is validated and sanitized. It can vary from low impact because the user running the -injected commands has limited rights, to critical impact if running as the root user. - -Potential impacts include: - -- Execution of arbitrary commands on the host machine. -- Unauthorized access to sensitive data, including passwords and tokens in secrets or configuration - files. -- Exposure of sensitive system files on the host machine, such as `/etc/passwd/` or `/etc/shadow`. -- Compromise of related systems and services gained through access to the host machine. - -You should be aware of and take steps to prevent command injection when working with user-controlled -data that are used to run OS commands. - -### Mitigation and prevention - -To prevent OS command injections, user-supplied data shouldn't be used within OS commands. In cases -where you can't avoid this: - -- Validate user-supplied data against an allowlist. -- Ensure that user-supplied data only contains alphanumeric characters (and no syntax or whitespace - characters, for example). -- Always use `--` to separate options from arguments. - -#### Ruby - -Consider using `system("command", "arg0", "arg1", ...)` whenever you can. This prevents an attacker -from concatenating commands. - -For more examples on how to use shell commands securely, consult -[Guidelines for shell commands in the GitLab codebase](shell_commands.md). -It contains various examples on how to securely call OS commands. - -#### Go - -Go has built-in protections that usually prevent an attacker from successfully injecting OS commands. - -Consider the following example: - -```go -package main - -import ( - "fmt" - "os/exec" -) - -func main() { - cmd := exec.Command("echo", "1; cat /etc/passwd") - out, _ := cmd.Output() - fmt.Printf("%s", out) -} -``` - -This echoes `"1; cat /etc/passwd"`. - -**Do not** use `sh`, as it bypasses internal protections: - -```go -out, _ = exec.Command("sh", "-c", "echo 1 | cat /etc/passwd").Output() -``` - -This outputs `1` followed by the content of `/etc/passwd`. - ## General recommendations ### TLS minimum recommended version @@ -1274,382 +917,6 @@ In the example above, the `is_admin?` method is overwritten when passing it to t Consider creating an allowlist of values, and validating the user input against that. - When extending classes that use metaprogramming, make sure you don't inadvertently override any method definition safety checks. -## Working with archive files - -Working with archive files like `zip`, `tar`, `jar`, `war`, `cpio`, `apk`, `rar` and `7z` presents an area where potentially critical security vulnerabilities can sneak into an application. - -### Utilities for safely working with archive files - -There are common utilities that can be used to securely work with archive files. - -#### Ruby - -| Archive type | Utility | -|--------------|-------------| -| `zip` | `SafeZip` | - -#### `SafeZip` - -SafeZip provides a safe interface to extract specific directories or files within a `zip` archive through the `SafeZip::Extract` class. - -Example: - -```ruby -Dir.mktmpdir do |tmp_dir| - SafeZip::Extract.new(zip_file_path).extract(files: ['index.html', 'app/index.js'], to: tmp_dir) - SafeZip::Extract.new(zip_file_path).extract(directories: ['src/', 'test/'], to: tmp_dir) -rescue SafeZip::Extract::EntrySizeError - raise Error, "Path `#{file_path}` has invalid size in the zip!" -end -``` - -### Zip Slip - -In 2018, the security company Snyk [released a blog post](https://security.snyk.io/research/zip-slip-vulnerability) describing research into a widespread and critical vulnerability present in many libraries and applications which allows an attacker to overwrite arbitrary files on the server file system which, in many cases, can be leveraged to achieve remote code execution. The vulnerability was dubbed Zip Slip. - -A Zip Slip vulnerability happens when an application extracts an archive without validating and sanitizing the filenames inside the archive for directory traversal sequences that change the file location when the file is extracted. - -Example malicious filenames: - -- `../../etc/passwd` -- `../../root/.ssh/authorized_keys` -- `../../etc/gitlab/gitlab.rb` - -If a vulnerable application extracts an archive file with any of these filenames, the attacker can overwrite these files with arbitrary content. - -### Insecure archive extraction examples - -#### 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`: - -```ruby -# Vulnerable tar.gz extraction example! - -begin - tar_extract = Gem::Package::TarReader.new(Zlib::GzipReader.open("/tmp/uploaded.tar.gz")) -rescue Errno::ENOENT - STDERR.puts("archive file does not exist or is not readable") - exit(false) -end -tar_extract.rewind - -tar_extract.each do |entry| - next unless entry.file? # Only process files in this example for simplicity. - - destination = "/tmp/extracted/#{entry.full_name}" # Oops! We blindly use the entry filename for the destination. - File.open(destination, "wb") do |out| - out.write(entry.read) - end -end -``` - -#### Go - -```go -// unzip INSECURELY extracts source zip file to destination. -func unzip(src, dest string) error { - r, err := zip.OpenReader(src) - if err != nil { - return err - } - defer r.Close() - - os.MkdirAll(dest, 0750) - - for _, f := range r.File { - if f.FileInfo().IsDir() { // Skip directories in this example for simplicity. - continue - } - - rc, err := f.Open() - if err != nil { - return err - } - defer rc.Close() - - path := filepath.Join(dest, f.Name) // Oops! We blindly use the entry filename for the destination. - os.MkdirAll(filepath.Dir(path), f.Mode()) - f, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, f.Mode()) - if err != nil { - return err - } - defer f.Close() - - if _, err := io.Copy(f, rc); err != nil { - return err - } - } - - return nil -} -``` - -#### Best practices - -Always expand the destination file path by resolving all potential directory traversals and other sequences that can alter the path and refuse extraction if the final destination path does not start with the intended destination directory. - -##### Ruby - -```ruby -# tar.gz extraction example with protection against Zip Slip attacks. - -begin - tar_extract = Gem::Package::TarReader.new(Zlib::GzipReader.open("/tmp/uploaded.tar.gz")) -rescue Errno::ENOENT - STDERR.puts("archive file does not exist or is not readable") - exit(false) -end -tar_extract.rewind - -tar_extract.each do |entry| - next unless entry.file? # Only process files in this example for simplicity. - - # safe_destination will raise an exception in case of Zip Slip / directory traversal. - destination = safe_destination(entry.full_name, "/tmp/extracted") - - File.open(destination, "wb") do |out| - out.write(entry.read) - end -end - -def safe_destination(filename, destination_dir) - raise "filename cannot start with '/'" if filename.start_with?("/") - - destination_dir = File.realpath(destination_dir) - destination = File.expand_path(filename, destination_dir) - - raise "filename is outside of destination directory" unless - destination.start_with?(destination_dir + "/")) - - destination -end -``` - -```ruby -# zip extraction example using rubyzip with built-in protection against Zip Slip attacks. -require 'zip' - -Zip::File.open("/tmp/uploaded.zip") do |zip_file| - zip_file.each do |entry| - # Extract entry to /tmp/extracted directory. - entry.extract("/tmp/extracted") - end -end -``` - -##### Go - -You are encouraged to use the secure archive utilities provided by [LabSec](https://gitlab.com/gitlab-com/gl-security/appsec/labsec) which will handle Zip Slip and other types of vulnerabilities for you. The LabSec utilities are also context aware which makes it possible to cancel or timeout extractions: - -```go -package main - -import "gitlab-com/gl-security/appsec/labsec/archive/zip" - -func main() { - f, err := os.Open("/tmp/uploaded.zip") - if err != nil { - panic(err) - } - defer f.Close() - - fi, err := f.Stat() - if err != nil { - panic(err) - } - - if err := zip.Extract(context.Background(), f, fi.Size(), "/tmp/extracted"); err != nil { - panic(err) - } -} -``` - -In case the LabSec utilities do not fit your needs, here is an example for extracting a zip file with protection against Zip Slip attacks: - -```go -// unzip extracts source zip file to destination with protection against Zip Slip attacks. -func unzip(src, dest string) error { - r, err := zip.OpenReader(src) - if err != nil { - return err - } - defer r.Close() - - os.MkdirAll(dest, 0750) - - for _, f := range r.File { - if f.FileInfo().IsDir() { // Skip directories in this example for simplicity. - continue - } - - rc, err := f.Open() - if err != nil { - return err - } - defer rc.Close() - - path := filepath.Join(dest, f.Name) - - // Check for Zip Slip / directory traversal - if !strings.HasPrefix(path, filepath.Clean(dest) + string(os.PathSeparator)) { - return fmt.Errorf("illegal file path: %s", path) - } - - os.MkdirAll(filepath.Dir(path), f.Mode()) - f, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, f.Mode()) - if err != nil { - return err - } - defer f.Close() - - if _, err := io.Copy(f, rc); err != nil { - return err - } - } - - return nil -} -``` - -### Symlink attacks - -Symlink attacks makes it possible for an attacker to read the contents of arbitrary files on the server of a vulnerable application. While it is a high-severity vulnerability that can often lead to remote code execution and other critical vulnerabilities, it is only exploitable in scenarios where a vulnerable application accepts archive files from the attacker and somehow displays the extracted contents back to the attacker without any validation or sanitization of symbolic links inside the archive. - -### Insecure archive symlink extraction examples - -#### Ruby - -For zip files, the [`rubyzip`](https://rubygems.org/gems/rubyzip) Ruby gem is already patched against symlink attacks as it 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! - -begin - tar_extract = Gem::Package::TarReader.new(Zlib::GzipReader.open("/tmp/uploaded.tar.gz")) -rescue Errno::ENOENT - STDERR.puts("archive file does not exist or is not readable") - exit(false) -end -tar_extract.rewind - -# Loop over each entry and output file contents -tar_extract.each do |entry| - next if entry.directory? - - # Oops! We don't check if the file is actually a symbolic link to a potentially sensitive file. - puts entry.read -end -``` - -#### Go - -```go -// printZipContents INSECURELY prints contents of files in a zip file. -func printZipContents(src string) error { - r, err := zip.OpenReader(src) - if err != nil { - return err - } - defer r.Close() - - // Loop over each entry and output file contents - for _, f := range r.File { - if f.FileInfo().IsDir() { - continue - } - - rc, err := f.Open() - if err != nil { - return err - } - defer rc.Close() - - // Oops! We don't check if the file is actually a symbolic link to a potentially sensitive file. - buf, err := ioutil.ReadAll(rc) - if err != nil { - return err - } - - fmt.Println(buf.String()) - } - - return nil -} -``` - -#### Best practices - -Always check the type of the archive entry before reading the contents and ignore entries that are not plain files. If you absolutely must support symbolic links, ensure that they only point to files inside the archive and nowhere else. - -##### Ruby - -```ruby -# tar.gz extraction example with protection against symlink attacks. - -begin - tar_extract = Gem::Package::TarReader.new(Zlib::GzipReader.open("/tmp/uploaded.tar.gz")) -rescue Errno::ENOENT - STDERR.puts("archive file does not exist or is not readable") - exit(false) -end -tar_extract.rewind - -# Loop over each entry and output file contents -tar_extract.each do |entry| - next if entry.directory? - - # By skipping symbolic links entirely, we are sure they can't cause any trouble! - next if entry.symlink? - - puts entry.read -end -``` - -##### Go - -You are encouraged to use the secure archive utilities provided by [LabSec](https://gitlab.com/gitlab-com/gl-security/appsec/labsec) which will handle Zip Slip and symlink vulnerabilities for you. The LabSec utilities are also context aware which makes it possible to cancel or timeout extractions. - -In case the LabSec utilities do not fit your needs, here is an example for extracting a zip file with protection against symlink attacks: - -```go -// printZipContents prints contents of files in a zip file with protection against symlink attacks. -func printZipContents(src string) error { - r, err := zip.OpenReader(src) - if err != nil { - return err - } - defer r.Close() - - // Loop over each entry and output file contents - for _, f := range r.File { - if f.FileInfo().IsDir() { - continue - } - - // By skipping all irregular file types (including symbolic links), we are sure they can't cause any trouble! - if !zf.Mode().IsRegular() { - continue - } - - rc, err := f.Open() - if err != nil { - return err - } - defer rc.Close() - - buf, err := ioutil.ReadAll(rc) - if err != nil { - return err - } - - fmt.Println(buf.String()) - } - - return nil -} -``` - ## Time of check to time of use bugs Time of check to time of use, or TOCTOU, is a class of error which occur when the state of something changes unexpectedly partway during a process. @@ -1994,127 +1261,6 @@ Logging helps track events for debugging. Logging also allows the application to - [Security logging overview](https://handbook.gitlab.com/handbook/security/security-operations/security-logging/) - [OWASP logging cheat sheet](https://cheatsheetseries.owasp.org/cheatsheets/Logging_Cheat_Sheet.html) -## URL Spoofing - -We want to protect our users from bad actors who might try to use GitLab -features to redirect other users to malicious sites. - -Many features in GitLab allow users to post links to external websites. It is -important that the destination of any user-specified link is made very clear -to the user. - -### `external_redirect_path` - -When presenting links provided by users, if the actual URL is hidden, use the `external_redirect_path` -helper method to redirect the user to a warning page first. For example: - -```ruby -# Bad :( -# This URL comes from User-Land and may not be safe... -# We need the user to see where they are going. -link_to foo_social_url(@user), title: "Foo Social" do - sprite_icon('question-o') -end - -# Good :) -# The external_redirect "leaving GitLab" page will show the URL to the user -# before they leave. -link_to external_redirect_path(url: foo_social_url(@user)), title: "Foo" do - sprite_icon('question-o') -end -``` - -Also see this [real-life usage](https://gitlab.com/gitlab-org/gitlab/-/blob/bdba5446903ff634fb12ba695b2de99b6d6881b5/app/helpers/application_helper.rb#L378) as an example. - -## Email and notifications - -Ensure that only intended recipients get emails and notifications. Even if your -code is secure when it merges, it's better practice to use the defense-in-depth -"single recipient" check just before sending the email. This prevents a vulnerability -if otherwise-vulnerable code is committed at a later date. For example: - -### Example: Ruby - -```ruby -# Insecure if email is user-controlled -def insecure_email(email) - mail(to: email, subject: 'Password reset email') -end - -# A single recipient, just as a developer expects -insecure_email("person@example.com") - -# Multiple emails sent when an array is passed -insecure_email(["person@example.com", "attacker@evil.com"]) - -# Multiple emails sent even when a single string is passed -insecure_email("person@example.com, attacker@evil.com") -``` - -### Prevention and defense - -- Use `Gitlab::Email::SingleRecipientValidator` when adding new emails intended for a single recipient -- Strongly type your code by calling `.to_s` on values, or check its class with `value.kind_of?(String)` - -## Request Parameter Typing - -This Secure Code Guideline is enforced by the `StrongParams` RuboCop. - -In our Rails Controllers you must use `ActionController::StrongParameters`. This ensures that we explicitly define the keys and types of input we expect in a request. It is critical for avoiding Mass Assignment in our Models. It should also be used when parameters are passed to other areas of the GitLab codebase such as Services. - -Using `params[:key]` can lead to vulnerabilities when one part of the codebase expects a type like `String`, but gets passed (and handles unsafely and without error) an `Array`. - -{{< alert type="note" >}} - -This only applies to Rails Controllers. Our API and GraphQL endpoints enforce strong typing, and Go is statically typed. - -{{< /alert >}} - -### Example - -```ruby -class MyMailer - def reset(user, email) - mail(to: email, subject: 'Password reset email', body: user.reset_token) - end -end - -class MyController - - # Bad - email could be an array of values - # ?user[email]=VALUE will find a single user and email a single user - # ?user[email][]=victim@example.com&user[email][]=attacker@example.com will email the victim's token to the victim and user - def dangerously_reset_password - user = User.find_by(email: params[:user][:email]) - MyMailer.reset(user, params[:user][:email]) - end - - # Good - we use StrongParams which doesn't permit the Array type - # ?user[email]=VALUE will find a single user and email a single user - # ?user[email][]=victim@example.com&user[email][]=attacker@example.com will fail because there is no permitted :email key - def safely_reset_password - user = User.find_by(email: email_params[:email]) - MyMailer.reset(user, email_params[:email]) - end - - # This returns a new ActionController::Parameters that includes only the permitted attributes - def email_params - params.require(:user).permit(:email) - end -end -``` - -This class of issue applies to more than just email; other examples might include: - -- Allowing multiple One Time Password attempts in a single request: `?otp_attempt[]=000000&otp_attempt[]=000001&otp_attempt[]=000002...` -- Passing unexpected parameters like `is_admin` that are later `.merged` in a Service class - -### Related topics - -- [Watch a walkthrough video](https://www.youtube.com/watch?v=ydg95R2QKwM) for an instance of this issue causing vulnerability CVE-2023-7028. - The video covers what happened, how it worked, and what you need to know for the future. -- Rails documentation for [ActionController::StrongParameters](https://api.rubyonrails.org/classes/ActionController/StrongParameters.html) and [ActionController::Parameters](https://api.rubyonrails.org/classes/ActionController/Parameters.html) - ## Paid tiers for vulnerability mitigation Secure code must not rely on subscription tiers (Premium/Ultimate) or diff --git a/doc/development/secure_coding_guidelines/go.md b/doc/development/secure_coding_guidelines/go.md new file mode 100644 index 00000000000000..4fbd9a818d8e23 --- /dev/null +++ b/doc/development/secure_coding_guidelines/go.md @@ -0,0 +1,407 @@ +## Regular Expressions guidelines + +### Escape sequences in Go + +When a character in a string literal or regular expression literal is preceded by a backslash, it is interpreted as part of an escape sequence. For example, the escape sequence `\n` in a string literal corresponds to a single `newline` character, and not the ` \ ` and `n` characters. + +There are two Go escape sequences that could produce surprising results. First, `regexp.Compile("\a")` matches the bell character, whereas `regexp.Compile("\\A")` matches the start of text and `regexp.Compile("\\a")` is a Vim (but not Go) regular expression matching any alphabetic character. Second, `regexp.Compile("\b")` matches a backspace, whereas `regexp.Compile("\\b")` matches the start of a word. Confusing one for the other could lead to a regular expression passing or failing much more often than expected, with potential security consequences. + +#### Examples + +The following example code fails to check for a forbidden word in an input string: + +```go +package main + +import "regexp" + +func broken(hostNames []byte) string { + var hostRe = regexp.MustCompile("\bforbidden.host.org") + if hostRe.Match(hostNames) { + return "Must not target forbidden.host.org" + } else { + // This will be reached even if hostNames is exactly "forbidden.host.org", + // because the literal backspace is not matched + return "" + } +} +``` + +#### Mitigation + +The above check does not work, but can be fixed by escaping the backslash: + +```go +package main + +import "regexp" + +func fixed(hostNames []byte) string { + var hostRe = regexp.MustCompile(`\bforbidden.host.org`) + if hostRe.Match(hostNames) { + return "Must not target forbidden.host.org" + } else { + // hostNames definitely doesn't contain a word "forbidden.host.org", as "\\b" + // is the start-of-word anchor, not a literal backspace. + return "" + } +} +``` + +Alternatively, you can use backtick-delimited raw string literals. For example, the `\b` in ``regexp.Compile(`hello\bworld`)`` matches a word boundary, not a backspace character, as within backticks `\b` is not an escape sequence. + +## Path Traversal guidelines + +### Description + +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 + +Path Traversal attacks can lead to multiple critical and high severity issues, like arbitrary file read, remote code execution, or information disclosure. + +### When to consider + +When working with user-controlled filenames/paths and file system APIs. + +### Mitigation and prevention + +In order to prevent Path Traversal vulnerabilities, user-controlled filenames or paths should be validated before being processed. + +- Comparing user input against an allowlist of allowed values or verifying that it only contains allowed characters. +- After validating the user supplied input, it should be appended to the base directory and the path should be canonicalized using the file system API. + + +Go has unintuitive 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`: + +```go +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! +``` + +#### Safe File Operations in Go + +The Go standard library provides basic file operations like `os.Open`, `os.ReadFile`, `os.WriteFile`, and `os.Readlink`. However, these functions do not prevent path traversal attacks, where user-supplied paths can escape the intended directory and access sensitive system files. + +Example of unsafe usage: + +```go +// Vulnerable: user input is directly used in the path +os.Open(filepath.Join("/app/data", userInput)) +os.ReadFile(filepath.Join("/app/data", userInput)) +os.WriteFile(filepath.Join("/app/data", userInput), []byte("data"), 0644) +os.Readlink(filepath.Join("/app/data", userInput)) +``` + +To mitigate these risks, use the [`safeopen`](https://pkg.go.dev/github.com/google/safeopen) library functions. These functions enforce a secure root directory and sanitize file paths: + +Example of safe usage: + +```go +safeopen.OpenBeneath("/app/data", userInput) +safeopen.ReadFileBeneath("/app/data", userInput) +safeopen.WriteFileBeneath("/app/data", []byte("data"), 0644) +safeopen.ReadlinkBeneath("/app/data", userInput) +``` + +Benefits: + +- Prevents path traversal attacks (`../` sequences). +- Restricts file operations to trusted root directories. +- Secures against unauthorized file reads, writes, and symlink resolutions. +- Provides simple, developer-friendly replacements. + +References: + +- [Go Standard Library os Package](https://pkg.go.dev/os) +- [Safe Go Libraries Announcement](https://bughunters.google.com/blog/4925068200771584/the-family-of-safe-golang-libraries-is-growing) +- [OWASP Path Traversal Cheat Sheet](https://owasp.org/www-community/attacks/Path_Traversal) + +## OS command injection guidelines + +Command injection is an issue in which an attacker is able to execute arbitrary commands on the host +operating system through a vulnerable application. Such attacks don't always provide feedback to a +user, but the attacker can use simple commands like `curl` to obtain an answer. + +### Impact + +The impact of command injection greatly depends on the user context running the commands, as well as +how data is validated and sanitized. It can vary from low impact because the user running the +injected commands has limited rights, to critical impact if running as the root user. + +Potential impacts include: + +- Execution of arbitrary commands on the host machine. +- Unauthorized access to sensitive data, including passwords and tokens in secrets or configuration + files. +- Exposure of sensitive system files on the host machine, such as `/etc/passwd/` or `/etc/shadow`. +- Compromise of related systems and services gained through access to the host machine. + +You should be aware of and take steps to prevent command injection when working with user-controlled +data that are used to run OS commands. + +### Mitigation and prevention + +To prevent OS command injections, user-supplied data shouldn't be used within OS commands. In cases +where you can't avoid this: + +- Validate user-supplied data against an allowlist. +- Ensure that user-supplied data only contains alphanumeric characters (and no syntax or whitespace + characters, for example). +- Always use `--` to separate options from arguments. + +#### Go + +Go has built-in protections that usually prevent an attacker from successfully injecting OS commands. + +Consider the following example: + +```go +package main + +import ( + "fmt" + "os/exec" +) + +func main() { + cmd := exec.Command("echo", "1; cat /etc/passwd") + out, _ := cmd.Output() + fmt.Printf("%s", out) +} +``` + +This echoes `"1; cat /etc/passwd"`. + +**Do not** use `sh`, as it bypasses internal protections: + +```go +out, _ = exec.Command("sh", "-c", "echo 1 | cat /etc/passwd").Output() +``` + +This outputs `1` followed by the content of `/etc/passwd`. + +## Working with archive files + +Working with archive files like `zip`, `tar`, `jar`, `war`, `cpio`, `apk`, `rar` and `7z` presents an area where potentially critical security vulnerabilities can sneak into an application. + +### Zip Slip + +In 2018, the security company Snyk [released a blog post](https://security.snyk.io/research/zip-slip-vulnerability) describing research into a widespread and critical vulnerability present in many libraries and applications which allows an attacker to overwrite arbitrary files on the server file system which, in many cases, can be leveraged to achieve remote code execution. The vulnerability was dubbed Zip Slip. + +A Zip Slip vulnerability happens when an application extracts an archive without validating and sanitizing the filenames inside the archive for directory traversal sequences that change the file location when the file is extracted. + +Example malicious filenames: + +- `../../etc/passwd` +- `../../root/.ssh/authorized_keys` +- `../../etc/gitlab/gitlab.rb` + +If a vulnerable application extracts an archive file with any of these filenames, the attacker can overwrite these files with arbitrary content. + +### Insecure archive extraction examples + +```go +// unzip INSECURELY extracts source zip file to destination. +func unzip(src, dest string) error { + r, err := zip.OpenReader(src) + if err != nil { + return err + } + defer r.Close() + + os.MkdirAll(dest, 0750) + + for _, f := range r.File { + if f.FileInfo().IsDir() { // Skip directories in this example for simplicity. + continue + } + + rc, err := f.Open() + if err != nil { + return err + } + defer rc.Close() + + path := filepath.Join(dest, f.Name) // Oops! We blindly use the entry filename for the destination. + os.MkdirAll(filepath.Dir(path), f.Mode()) + f, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, f.Mode()) + if err != nil { + return err + } + defer f.Close() + + if _, err := io.Copy(f, rc); err != nil { + return err + } + } + + return nil +} +``` + +#### Best practices + +Always expand the destination file path by resolving all potential directory traversals and other sequences that can alter the path and refuse extraction if the final destination path does not start with the intended destination directory. + +You are encouraged to use the secure archive utilities provided by [LabSec](https://gitlab.com/gitlab-com/gl-security/appsec/labsec) which will handle Zip Slip and other types of vulnerabilities for you. The LabSec utilities are also context aware which makes it possible to cancel or timeout extractions: + +```go +package main + +import "gitlab-com/gl-security/appsec/labsec/archive/zip" + +func main() { + f, err := os.Open("/tmp/uploaded.zip") + if err != nil { + panic(err) + } + defer f.Close() + + fi, err := f.Stat() + if err != nil { + panic(err) + } + + if err := zip.Extract(context.Background(), f, fi.Size(), "/tmp/extracted"); err != nil { + panic(err) + } +} +``` + +In case the LabSec utilities do not fit your needs, here is an example for extracting a zip file with protection against Zip Slip attacks: + +```go +// unzip extracts source zip file to destination with protection against Zip Slip attacks. +func unzip(src, dest string) error { + r, err := zip.OpenReader(src) + if err != nil { + return err + } + defer r.Close() + + os.MkdirAll(dest, 0750) + + for _, f := range r.File { + if f.FileInfo().IsDir() { // Skip directories in this example for simplicity. + continue + } + + rc, err := f.Open() + if err != nil { + return err + } + defer rc.Close() + + path := filepath.Join(dest, f.Name) + + // Check for Zip Slip / directory traversal + if !strings.HasPrefix(path, filepath.Clean(dest) + string(os.PathSeparator)) { + return fmt.Errorf("illegal file path: %s", path) + } + + os.MkdirAll(filepath.Dir(path), f.Mode()) + f, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, f.Mode()) + if err != nil { + return err + } + defer f.Close() + + if _, err := io.Copy(f, rc); err != nil { + return err + } + } + + return nil +} +``` + +### Symlink attacks + +Symlink attacks makes it possible for an attacker to read the contents of arbitrary files on the server of a vulnerable application. While it is a high-severity vulnerability that can often lead to remote code execution and other critical vulnerabilities, it is only exploitable in scenarios where a vulnerable application accepts archive files from the attacker and somehow displays the extracted contents back to the attacker without any validation or sanitization of symbolic links inside the archive. + +### Insecure archive symlink extraction examples + +```go +// printZipContents INSECURELY prints contents of files in a zip file. +func printZipContents(src string) error { + r, err := zip.OpenReader(src) + if err != nil { + return err + } + defer r.Close() + + // Loop over each entry and output file contents + for _, f := range r.File { + if f.FileInfo().IsDir() { + continue + } + + rc, err := f.Open() + if err != nil { + return err + } + defer rc.Close() + + // Oops! We don't check if the file is actually a symbolic link to a potentially sensitive file. + buf, err := ioutil.ReadAll(rc) + if err != nil { + return err + } + + fmt.Println(buf.String()) + } + + return nil +} +``` + +#### Best practices + +Always check the type of the archive entry before reading the contents and ignore entries that are not plain files. If you absolutely must support symbolic links, ensure that they only point to files inside the archive and nowhere else. + +You are encouraged to use the secure archive utilities provided by [LabSec](https://gitlab.com/gitlab-com/gl-security/appsec/labsec) which will handle Zip Slip and symlink vulnerabilities for you. The LabSec utilities are also context aware which makes it possible to cancel or timeout extractions. + +In case the LabSec utilities do not fit your needs, here is an example for extracting a zip file with protection against symlink attacks: + +```go +// printZipContents prints contents of files in a zip file with protection against symlink attacks. +func printZipContents(src string) error { + r, err := zip.OpenReader(src) + if err != nil { + return err + } + defer r.Close() + + // Loop over each entry and output file contents + for _, f := range r.File { + if f.FileInfo().IsDir() { + continue + } + + // By skipping all irregular file types (including symbolic links), we are sure they can't cause any trouble! + if !zf.Mode().IsRegular() { + continue + } + + rc, err := f.Open() + if err != nil { + return err + } + defer rc.Close() + + buf, err := ioutil.ReadAll(rc) + if err != nil { + return err + } + + fmt.Println(buf.String()) + } + + return nil +} +``` diff --git a/doc/development/secure_coding_guidelines/ruby.md b/doc/development/secure_coding_guidelines/ruby.md new file mode 100644 index 00000000000000..4ef75e43c4869a --- /dev/null +++ b/doc/development/secure_coding_guidelines/ruby.md @@ -0,0 +1,864 @@ +## Regular Expressions guidelines + +### Anchors / Multi line in Ruby + +Unlike other programming languages (for example, Perl or Python) Regular Expressions are matching multi-line by default in Ruby. Consider the following example in Python: + +```python +import re +text = "foo\nbar" +matches = re.findall("^bar$",text) +print(matches) +``` + +The Python example will output an empty array (`[]`) as the matcher considers the whole string `foo\nbar` including the newline (`\n`). In contrast Ruby's Regular Expression engine acts differently: + +```ruby +text = "foo\nbar" +p text.match /^bar$/ +``` + +The output of this example is `#`, as Ruby treats the input `text` line by line. To match the whole **string**, the Regex anchors `\A` and `\z` should be used. + +#### Impact + +This Ruby Regex specialty can have security impact, as often regular expressions are used for validations or to impose restrictions on user-input. + +#### Examples + +GitLab-specific examples can be found in the following [path traversal](https://gitlab.com/gitlab-org/gitlab/-/issues/36029#note_251262187) +and [open redirect](https://gitlab.com/gitlab-org/gitlab/-/issues/33569) issues. + +Another example would be this fictional Ruby on Rails controller: + +```ruby +class PingController < ApplicationController + def ping + if params[:ip] =~ /^\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}$/ + render :text => `ping -c 4 #{params[:ip]}` + else + render :text => "Invalid IP" + end + end +end +``` + +Here `params[:ip]` should not contain anything else but numbers and dots. However this restriction can be easily bypassed as the Regex anchors `^` and `$` are being used. Ultimately this leads to a shell command injection in `ping -c 4 #{params[:ip]}` by using newlines in `params[:ip]`. + +#### Mitigation + +In most cases the anchors `\A` for beginning of text and `\z` for end of text should be used instead of `^` and `$`. + +## Denial of Service (ReDoS) / Catastrophic Backtracking + +When a regular expression (regex) is used to search for a string and can't find a match, +it may then backtrack to try other possibilities. + +For example when the regex `.*!$` matches the string `hello!`, the `.*` first matches +the entire string but then the `!` from the regex is unable to match because the +character has already been used. In that case, the Ruby regex engine _backtracks_ +one character to allow the `!` to match. + +[ReDoS](https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS) +is an attack in which the attacker knows or controls the regular expression used. +The attacker may be able to enter user input that triggers this backtracking behavior in a +way that increases execution time by several orders of magnitude. + +### Impact + +The resource, for example Puma, or Sidekiq, can be made to hang as it takes +a long time to evaluate the bad regex match. The evaluation time may require manual +termination of the resource. + +### Examples + +Here are some GitLab-specific examples. + +User inputs used to create regular expressions: + +- [User-controlled filename](https://gitlab.com/gitlab-org/gitlab/-/issues/257497) +- [User-controlled domain name](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/25314) +- [User-controlled email address](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/25122#note_289087459) + +Hardcoded regular expressions with backtracking issues: + +- [Repository name validation](https://gitlab.com/gitlab-org/gitlab/-/issues/220019) +- [Link validation](https://gitlab.com/gitlab-org/gitlab/-/issues/218753), and [a bypass](https://gitlab.com/gitlab-org/gitlab/-/issues/273771) +- [Entity name validation](https://gitlab.com/gitlab-org/gitlab/-/issues/289934) +- [Validating color codes](https://gitlab.com/gitlab-org/gitlab/-/commit/717824144f8181bef524592eab882dd7525a60ef) + +Consider the following example application, which defines a check using a regular expression. A user entering `user@aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa!.com` as the email on a form will hang the web server. + +```ruby +# For ruby versions < 3.2.0 +# Press ctrl+c to terminate a hung process +class Email < ApplicationRecord + DOMAIN_MATCH = Regexp.new('([a-zA-Z0-9]+)+\.com') + + validates :domain_matches + + private + + def domain_matches + errors.add(:email, 'does not match') if email =~ DOMAIN_MATCH + end +end +``` + +### Mitigation + +#### Ruby from 3.2.0 + +Ruby released [Regexp improvements against ReDoS in 3.2.0](https://www.ruby-lang.org/en/news/2022/12/25/ruby-3-2-0-released/). ReDoS will no longer be an issue, with the exception of _"some kind of regular expressions, such as those including advanced features (like back-references or look-around), or with a huge fixed number of repetitions"_. + +[Until GitLab enforces a global Regexp timeout](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/145679) you should pass an explicit timeout parameter, particularly when using advanced features or a large number of repetitions. For example: + +```ruby +Regexp.new('^a*b?a*()\1$', timeout: 1) # timeout in seconds +``` + +#### Ruby before 3.2.0 + +GitLab has [`Gitlab::UntrustedRegexp`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/lib/gitlab/untrusted_regexp.rb) + which internally uses the [`re2`](https://github.com/google/re2/wiki/Syntax) library. +`re2` does not support backtracking so we get constant execution time, and a smaller subset of available regex features. + +All user-provided regular expressions should use `Gitlab::UntrustedRegexp`. + +For other regular expressions, here are a few guidelines: + +- If there's a clean non-regex solution, such as `String#start_with?`, consider using it +- Ruby supports some advanced regex features like [atomic groups](https://www.regular-expressions.info/atomic.html) + and [possessive quantifiers](https://www.regular-expressions.info/possessive.html) that eliminate backtracking +- Avoid nested quantifiers if possible (for example `(a+)+`) +- Try to be as precise as possible in your regex and avoid the `.` if there's an alternative + - For example, Use `_[^_]+_` instead of `_.*_` to match `_text here_` +- Use reasonable ranges (for example, `{1,10}`) for repeating patterns instead of unbounded `*` and `+` matchers +- When possible, perform simple input validation such as maximum string length checks before using regular expressions +- If in doubt, don't hesitate to ping `@gitlab-com/gl-security/appsec` + +## Server Side Request Forgery (SSRF) + +### Description + +A Server-side Request Forgery (SSRF) is an attack in which an attacker +is able coerce a application into making an outbound request to an unintended +resource. This resource is usually internal. In GitLab, the connection most +commonly uses HTTP, but an SSRF can be performed with any protocol, such as +Redis or SSH. + +With an SSRF attack, the UI may or may not show the response. The latter is +called a Blind SSRF. While the impact is reduced, it can still be useful for +attackers, especially for mapping internal network services as part of recon. + +### Impact + +The impact of an SSRF can vary, depending on what the application server +can communicate with, how much the attacker can control of the payload, and +if the response is returned back to the attacker. Examples of impact that +have been reported to GitLab include: + +- Network mapping of internal services + - This can help an attacker gather information about internal services + that could be used in further attacks. [More details](https://gitlab.com/gitlab-org/gitlab-foss/-/issues/51327). +- Reading internal services, including cloud service metadata. + - The latter can be a serious problem, because an attacker can obtain keys that allow control of the victim's cloud infrastructure. (This is also a good reason + to give only necessary privileges to the token.). [More details](https://gitlab.com/gitlab-org/gitlab-foss/-/issues/51490). +- When combined with CRLF vulnerability, remote code execution. [More details](https://gitlab.com/gitlab-org/gitlab-foss/-/issues/41293). + +### When to Consider + +- When the application makes any outbound connection + +### Mitigations + +In order to mitigate SSRF vulnerabilities, it is necessary to validate the destination of the outgoing request, especially if it includes user-supplied information. + +The preferred SSRF mitigations within GitLab are: + +1. Only connect to known, trusted domains/IP addresses. +1. Use the [`Gitlab::HTTP`](#gitlab-http-library) library +1. Implement [feature-specific mitigations](#feature-specific-mitigations) + +#### GitLab HTTP Library + +The [`Gitlab::HTTP`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/lib/gitlab/http.rb) wrapper library has grown to include mitigations for all of the GitLab-known SSRF vectors. It is also configured to respect the +`Outbound requests` options that allow instance administrators to block all internal connections, or limit the networks to which connections can be made. +The `Gitlab::HTTP` wrapper library delegates the requests to the [`gitlab-http`](https://gitlab.com/gitlab-org/gitlab/-/tree/master/gems/gitlab-http) gem. + +In some cases, it has been possible to configure `Gitlab::HTTP` as the HTTP +connection library for 3rd-party gems. This is preferable over re-implementing +the mitigations for a new feature. + +- [More details](https://dev.gitlab.org/gitlab/gitlabhq/-/merge_requests/2530/diffs) + +#### URL blocker & validation libraries + +[`Gitlab::HTTP_V2::UrlBlocker`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/gems/gitlab-http/lib/gitlab/http_v2/url_blocker.rb) can be used to validate that a +provided URL meets a set of constraints. Importantly, when `dns_rebind_protection` is `true`, the method returns a known-safe URI where the hostname +has been replaced with an IP address. This prevents DNS rebinding attacks, because the DNS record has been resolved. However, if we ignore this returned +value, we **will not** be protected against DNS rebinding. + +This is the case with validators such as the `AddressableUrlValidator` (called with `validates :url, addressable_url: {opts}` or `public_url: {opts}`). +Validation errors are only raised when validations are called, for example when a record is created or saved. If we ignore the value returned by the validation +when persisting the record, **we need to recheck** its validity before using it. For more information, see [Time of check to time of use bugs](#time-of-check-to-time-of-use-bugs). + +#### Feature-specific mitigations + +There are many tricks to bypass common SSRF validations. If feature-specific mitigations are necessary, they should be reviewed by the AppSec team, or a developer who has worked on SSRF mitigations previously. + +For situations in which you can't use an allowlist or `GitLab:HTTP`, you must implement mitigations +directly in the feature. It's best to validate the destination IP addresses themselves, not just +domain names, as the attacker can control DNS. Below is a list of mitigations that you should +implement. + +- Block connections to all localhost addresses + - `127.0.0.1/8` (IPv4 - note the subnet mask) + - `::1` (IPv6) +- Block connections to networks with private addressing (RFC 1918) + - `10.0.0.0/8` + - `172.16.0.0/12` + - `192.168.0.0/24` +- Block connections to link-local addresses (RFC 3927) + - `169.254.0.0/16` + - In particular, for GCP: `metadata.google.internal` -> `169.254.169.254` +- For HTTP connections: Disable redirects or validate the redirect destination +- To mitigate DNS rebinding attacks, validate and use the first IP address received. + +See [`url_blocker_spec.rb`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/spec/lib/gitlab/url_blocker_spec.rb) for examples of SSRF payloads. For more information about the DNS-rebinding class of bugs, see [Time of check to time of use bugs](#time-of-check-to-time-of-use-bugs). + +Don't rely on methods like `.start_with?` when validating a URL, or make assumptions about which +part of a string maps to which part of a URL. Use the `URI` class to parse the string, and validate +each component (scheme, host, port, path, and so on). Attackers can create valid URLs which look +safe, but lead to malicious locations. + +```ruby +user_supplied_url = "https://my-safe-site.com@my-evil-site.com" # Content before an @ in a URL is usually for basic authentication +user_supplied_url.start_with?("https://my-safe-site.com") # Don't trust with start_with? for URLs! +=> true +URI.parse(user_supplied_url).host +=> "my-evil-site.com" + +user_supplied_url = "https://my-safe-site.com-my-evil-site.com" +user_supplied_url.start_with?("https://my-safe-site.com") # Don't trust with start_with? for URLs! +=> true +URI.parse(user_supplied_url).host +=> "my-safe-site.com-my-evil-site.com" + +# Here's an example where we unsafely attempt to validate a host while allowing for +# subdomains +user_supplied_url = "https://my-evil-site-my-safe-site.com" +user_supplied_host = URI.parse(user_supplied_url).host +=> "my-evil-site-my-safe-site.com" +user_supplied_host.end_with?("my-safe-site.com") # Don't trust with end_with? +=> true +``` + +## XSS guidelines + +### Description + +Cross site scripting (XSS) is an issue where malicious JavaScript code gets injected into a trusted web application and executed in a client's browser. The input is intended to be data, but instead gets treated as code by the browser. + +XSS issues are commonly classified in three categories, by their delivery method: + +- [Persistent XSS](https://owasp.org/www-community/Types_of_Cross-Site_Scripting#stored-xss-aka-persistent-or-type-i) +- [Reflected XSS](https://owasp.org/www-community/Types_of_Cross-Site_Scripting#reflected-xss-aka-non-persistent-or-type-ii) +- [DOM XSS](https://owasp.org/www-community/Types_of_Cross-Site_Scripting#dom-based-xss-aka-type-0) + +### Impact + +The injected client-side code is executed on the victim's browser in the context of their current session. This means the attacker could perform any same action the victim would typically be able to do through a browser. The attacker would also have the ability to: + +- [log victim keystrokes](https://youtu.be/2VFavqfDS6w?t=1367) +- launch a network scan from the victim's browser +- potentially [obtain the victim's session tokens](https://youtu.be/2VFavqfDS6w?t=739) +- perform actions that lead to data loss/theft or account takeover + +Much of the impact is contingent upon the function of the application and the capabilities of the victim's session. For further impact possibilities, check out [the beef project](https://beefproject.com/). + +For a demonstration of the impact on GitLab with a realistic attack scenario, see [this video on the GitLab Unfiltered channel](https://www.youtube.com/watch?v=t4PzHNycoKo) (internal, it requires being logged in with the GitLab Unfiltered account). + +## XSS mitigation and prevention in Rails + +By default, Rails automatically escapes strings when they are inserted into HTML templates. Avoid the +methods used to keep Rails from escaping strings, especially those related to user-controlled values. +Specifically, the following options are dangerous because they mark strings as trusted and safe: + +| Method | Avoid these options | +|----------------------|-------------------------------| +| HAML templates | `html_safe`, `raw`, `!=` | +| Embedded Ruby (ERB) | `html_safe`, `raw`, `<%== %>` | + +In case you want to sanitize user-controlled values against XSS vulnerabilities, you can use +[`ActionView::Helpers::SanitizeHelper`](https://api.rubyonrails.org/classes/ActionView/Helpers/SanitizeHelper.html). +Calling `link_to` and `redirect_to` with user-controlled parameters can also lead to cross-site scripting. + +Do also sanitize and validate URL schemes. + +References: + +- [XSS Defense in Rails](https://youtu.be/2VFavqfDS6w?t=2442) +- [XSS Defense with HAML](https://youtu.be/2VFavqfDS6w?t=2796) +- [Validating Untrusted URLs in Ruby](https://youtu.be/2VFavqfDS6w?t=3936) +- [RoR Model Validators](https://youtu.be/2VFavqfDS6w?t=7636) + +#### XSS mitigation and prevention in JavaScript and Vue + +- When updating the content of an HTML element using JavaScript, mark user-controlled values as `textContent` or `nodeValue` instead of `innerHTML`. +- Avoid using `v-html` with user-controlled data, use [`v-safe-html`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/assets/javascripts/vue_shared/directives/safe_html.js) instead. +- Render unsafe or unsanitized content using [`dompurify`](fe_guide/security.md#sanitize-html-output). +- Consider using [`gl-sprintf`](i18n/externalization.md#interpolation) to interpolate translated strings securely. +- Avoid `__()` with translations that contain user-controlled values. +- When working with `postMessage`, ensure the `origin` of the message is allowlisted. +- Consider using the [Safe Link Directive](https://gitlab-org.gitlab.io/gitlab-ui/?path=/story/directives-safe-link-directive--default) to generate secure hyperlinks by default. + +## XML external entities + +### Description + +XML external entity (XXE) injection is a type of attack against an application that parses XML input. This attack occurs when XML input containing a reference to an external entity is processed by a weakly configured XML parser. It can lead to disclosure of confidential data, denial of service, server-side request forgery, port scanning from the perspective of the machine where the parser is located, and other system impacts. + +### XXE mitigation in Ruby + +The two main ways we can prevent XXE vulnerabilities in our codebase are: + +Use a safe XML parser: We prefer using Nokogiri when coding in Ruby. Nokogiri is a great option because it provides secure defaults that protect against XXE attacks. For more information, see the [Nokogiri documentation on parsing an HTML / XML Document](https://nokogiri.org/tutorials/parsing_an_html_xml_document.html#parse-options). + +When using Nokogiri, be sure to use the default or safe parsing settings, especially when working with unsanitized user input. Do not use the following unsafe Nokogiri settings ⚠️: + +| Setting | Description | +| ------ | ------ | +| `dtdload` | Tries to validate DTD validity of the object which is unsafe when working with unsanitized user input. | +| `huge` | Unsets maximum size/depth of objects that could be used for denial of service. | +| `nononet` | Allows network connections. | +| `noent` | Allows the expansion of XML entities and could result in arbitrary file reads. | + +### Safe XML Library + +```ruby +require 'nokogiri' + +# Safe by default +doc = Nokogiri::XML(xml_string) +``` + +### Unsafe XML Library, file system leak + +```ruby +require 'rexml/document' + +# Vulnerable code +xml = <<-EOX + + + ]> +&xxe; +EOX + +# Parsing XML without proper safeguards +doc = REXML::Document.new(xml) +puts doc.root.text +# This could output /etc/passwd +``` + +### Noent unsafe setting initialized, potential file system leak + +```ruby +require 'nokogiri' + +# Vulnerable code +xml = <<-EOX + + + ]> +&xxe; +EOX + +# noent substitutes entities, unsafe when parsing XML +po = Nokogiri::XML::ParseOptions.new.huge.noent +doc = Nokogiri::XML::Document.parse(xml, nil, nil, po) +puts doc.root.text # This will output the contents of /etc/passwd + +## +# User Database +# +# Note that this file is consulted directly only when the system is running +... +``` + +### Nononet unsafe setting initialized, potential malware execution + +```ruby +require 'nokogiri' + +# Vulnerable code +xml = <<-EOX + + + ]> +&xxe; +EOX + +# In this example we use `ParseOptions` but select insecure options. +# NONONET allows network connections while parsing which is unsafe, as is DTDLOAD! +options = Nokogiri::XML::ParseOptions.new(Nokogiri::XML::ParseOptions::NONONET, Nokogiri::XML::ParseOptions::DTDLOAD) + +# Parsing the xml above would allow `untrustedhost` to run arbitrary code on our server. +# See the "Impact" section for more. +doc = Nokogiri::XML::Document.parse(xml, nil, nil, options) +``` + +### Noent unsafe setting set, potential file system leak + +```ruby +require 'nokogiri' + +# Vulnerable code +xml = <<-EOX + + + ]> +&xxe; +EOX + +# setting options may also look like this, NONET disallows network connections while parsing safe +options = Nokogiri::XML::ParseOptions::NOENT | Nokogiri::XML::ParseOptions::NONET + +doc = Nokogiri::XML(xml, nil, nil, options) do |config| + config.nononet # Allows network access + config.noent # Enables entity expansion + config.dtdload # Enables DTD loading +end + +puts doc.to_xml +# This could output the contents of /etc/passwd +``` + +### Impact + +XXE attacks can lead to multiple critical and high severity issues, like arbitrary file read, remote code execution, or information disclosure. + +### When to consider + +When working with XML parsing, particularly with user-controlled inputs. + +## Path Traversal guidelines + +### Description + +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 + +Path Traversal attacks can lead to multiple critical and high severity issues, like arbitrary file read, remote code execution, or information disclosure. + +### When to consider + +When working with user-controlled filenames/paths and file system APIs. + +### Mitigation and prevention + +In order to prevent Path Traversal vulnerabilities, user-controlled filenames or paths should be validated before being processed. + +- Comparing user input against an allowlist of allowed values or verifying that it only contains allowed characters. +- After validating the user supplied input, it should be appended to the base directory and the path should be canonicalized using the file system API. + +#### GitLab specific validations + +The methods `Gitlab::PathTraversal.check_path_traversal!()` and `Gitlab::PathTraversal.check_allowed_absolute_path!()` +can be used to validate user-supplied paths and prevent vulnerabilities. +`check_path_traversal!()` will detect their Path Traversal payloads and accepts URL-encoded paths. +`check_allowed_absolute_path!()` will check if a path is absolute and whether it is inside the allowed path list. By default, absolute +paths are not allowed, so you need to pass a list of allowed absolute paths to the `path_allowlist` +parameter when using `check_allowed_absolute_path!()`. + +To use a combination of both checks, follow the example below: + +```ruby +Gitlab::PathTraversal.check_allowed_absolute_path_and_path_traversal!(path, path_allowlist) +``` + +In the REST API, we have the [`FilePath`](https://gitlab.com/gitlab-org/security/gitlab/-/blob/master/lib/api/validations/validators/file_path.rb) +validator that can be used to perform the checking on any file path argument the endpoints have. +It can be used as follows: + +```ruby +requires :file_path, type: String, file_path: { allowlist: ['/foo/bar/', '/home/foo/', '/app/home'] } +``` + +The Path Traversal check can also be used to forbid any absolute path: + +```ruby +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 +typical 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! +``` + +## OS command injection guidelines + +Command injection is an issue in which an attacker is able to execute arbitrary commands on the host +operating system through a vulnerable application. Such attacks don't always provide feedback to a +user, but the attacker can use simple commands like `curl` to obtain an answer. + +### Impact + +The impact of command injection greatly depends on the user context running the commands, as well as +how data is validated and sanitized. It can vary from low impact because the user running the +injected commands has limited rights, to critical impact if running as the root user. + +Potential impacts include: + +- Execution of arbitrary commands on the host machine. +- Unauthorized access to sensitive data, including passwords and tokens in secrets or configuration + files. +- Exposure of sensitive system files on the host machine, such as `/etc/passwd/` or `/etc/shadow`. +- Compromise of related systems and services gained through access to the host machine. + +You should be aware of and take steps to prevent command injection when working with user-controlled +data that are used to run OS commands. + +### Mitigation and prevention + +To prevent OS command injections, user-supplied data shouldn't be used within OS commands. In cases +where you can't avoid this: + +- Validate user-supplied data against an allowlist. +- Ensure that user-supplied data only contains alphanumeric characters (and no syntax or whitespace + characters, for example). +- Always use `--` to separate options from arguments. + +#### Ruby + +Consider using `system("command", "arg0", "arg1", ...)` whenever you can. This prevents an attacker +from concatenating commands. + +For more examples on how to use shell commands securely, consult +[Guidelines for shell commands in the GitLab codebase](shell_commands.md). +It contains various examples on how to securely call OS commands. + +## Working with archive files + +Working with archive files like `zip`, `tar`, `jar`, `war`, `cpio`, `apk`, `rar` and `7z` presents an area where potentially critical security vulnerabilities can sneak into an application. + +### Utilities for safely working with archive files + +There are common utilities that can be used to securely work with archive files. + +| Archive type | Utility | +|--------------|-------------| +| `zip` | `SafeZip` | + +#### `SafeZip` + +SafeZip provides a safe interface to extract specific directories or files within a `zip` archive through the `SafeZip::Extract` class. + +Example: + +```ruby +Dir.mktmpdir do |tmp_dir| + SafeZip::Extract.new(zip_file_path).extract(files: ['index.html', 'app/index.js'], to: tmp_dir) + SafeZip::Extract.new(zip_file_path).extract(directories: ['src/', 'test/'], to: tmp_dir) +rescue SafeZip::Extract::EntrySizeError + raise Error, "Path `#{file_path}` has invalid size in the zip!" +end +``` + +### Zip Slip + +In 2018, the security company Snyk [released a blog post](https://security.snyk.io/research/zip-slip-vulnerability) describing research into a widespread and critical vulnerability present in many libraries and applications which allows an attacker to overwrite arbitrary files on the server file system which, in many cases, can be leveraged to achieve remote code execution. The vulnerability was dubbed Zip Slip. + +A Zip Slip vulnerability happens when an application extracts an archive without validating and sanitizing the filenames inside the archive for directory traversal sequences that change the file location when the file is extracted. + +Example malicious filenames: + +- `../../etc/passwd` +- `../../root/.ssh/authorized_keys` +- `../../etc/gitlab/gitlab.rb` + +If a vulnerable application extracts an archive file with any of these filenames, the attacker can overwrite these files with arbitrary content. + +### Insecure archive extraction examples + +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! + +begin + tar_extract = Gem::Package::TarReader.new(Zlib::GzipReader.open("/tmp/uploaded.tar.gz")) +rescue Errno::ENOENT + STDERR.puts("archive file does not exist or is not readable") + exit(false) +end +tar_extract.rewind + +tar_extract.each do |entry| + next unless entry.file? # Only process files in this example for simplicity. + + destination = "/tmp/extracted/#{entry.full_name}" # Oops! We blindly use the entry filename for the destination. + File.open(destination, "wb") do |out| + out.write(entry.read) + end +end +``` + +#### Best practices + +Always expand the destination file path by resolving all potential directory traversals and other sequences that can alter the path and refuse extraction if the final destination path does not start with the intended destination directory. + +```ruby +# tar.gz extraction example with protection against Zip Slip attacks. + +begin + tar_extract = Gem::Package::TarReader.new(Zlib::GzipReader.open("/tmp/uploaded.tar.gz")) +rescue Errno::ENOENT + STDERR.puts("archive file does not exist or is not readable") + exit(false) +end +tar_extract.rewind + +tar_extract.each do |entry| + next unless entry.file? # Only process files in this example for simplicity. + + # safe_destination will raise an exception in case of Zip Slip / directory traversal. + destination = safe_destination(entry.full_name, "/tmp/extracted") + + File.open(destination, "wb") do |out| + out.write(entry.read) + end +end + +def safe_destination(filename, destination_dir) + raise "filename cannot start with '/'" if filename.start_with?("/") + + destination_dir = File.realpath(destination_dir) + destination = File.expand_path(filename, destination_dir) + + raise "filename is outside of destination directory" unless + destination.start_with?(destination_dir + "/")) + + destination +end +``` + +```ruby +# zip extraction example using rubyzip with built-in protection against Zip Slip attacks. +require 'zip' + +Zip::File.open("/tmp/uploaded.zip") do |zip_file| + zip_file.each do |entry| + # Extract entry to /tmp/extracted directory. + entry.extract("/tmp/extracted") + end +end +``` + +### Symlink attacks + +Symlink attacks makes it possible for an attacker to read the contents of arbitrary files on the server of a vulnerable application. While it is a high-severity vulnerability that can often lead to remote code execution and other critical vulnerabilities, it is only exploitable in scenarios where a vulnerable application accepts archive files from the attacker and somehow displays the extracted contents back to the attacker without any validation or sanitization of symbolic links inside the archive. + +### Insecure archive symlink extraction examples + +For zip files, the [`rubyzip`](https://rubygems.org/gems/rubyzip) Ruby gem is already patched against symlink attacks as it 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! + +begin + tar_extract = Gem::Package::TarReader.new(Zlib::GzipReader.open("/tmp/uploaded.tar.gz")) +rescue Errno::ENOENT + STDERR.puts("archive file does not exist or is not readable") + exit(false) +end +tar_extract.rewind + +# Loop over each entry and output file contents +tar_extract.each do |entry| + next if entry.directory? + + # Oops! We don't check if the file is actually a symbolic link to a potentially sensitive file. + puts entry.read +end +``` + +#### Best practices + +Always check the type of the archive entry before reading the contents and ignore entries that are not plain files. If you absolutely must support symbolic links, ensure that they only point to files inside the archive and nowhere else. + +```ruby +# tar.gz extraction example with protection against symlink attacks. + +begin + tar_extract = Gem::Package::TarReader.new(Zlib::GzipReader.open("/tmp/uploaded.tar.gz")) +rescue Errno::ENOENT + STDERR.puts("archive file does not exist or is not readable") + exit(false) +end +tar_extract.rewind + +# Loop over each entry and output file contents +tar_extract.each do |entry| + next if entry.directory? + + # By skipping symbolic links entirely, we are sure they can't cause any trouble! + next if entry.symlink? + + puts entry.read +end +``` + +## URL Spoofing + +We want to protect our users from bad actors who might try to use GitLab +features to redirect other users to malicious sites. + +Many features in GitLab allow users to post links to external websites. It is +important that the destination of any user-specified link is made very clear +to the user. + +### `external_redirect_path` + +When presenting links provided by users, if the actual URL is hidden, use the `external_redirect_path` +helper method to redirect the user to a warning page first. For example: + +```ruby +# Bad :( +# This URL comes from User-Land and may not be safe... +# We need the user to see where they are going. +link_to foo_social_url(@user), title: "Foo Social" do + sprite_icon('question-o') +end + +# Good :) +# The external_redirect "leaving GitLab" page will show the URL to the user +# before they leave. +link_to external_redirect_path(url: foo_social_url(@user)), title: "Foo" do + sprite_icon('question-o') +end +``` + +Also see this [real-life usage](https://gitlab.com/gitlab-org/gitlab/-/blob/bdba5446903ff634fb12ba695b2de99b6d6881b5/app/helpers/application_helper.rb#L378) as an example. + +## Email and notifications + +Ensure that only intended recipients get emails and notifications. Even if your +code is secure when it merges, it's better practice to use the defense-in-depth +"single recipient" check just before sending the email. This prevents a vulnerability +if otherwise-vulnerable code is committed at a later date. For example: + +### Example: Ruby + +```ruby +# Insecure if email is user-controlled +def insecure_email(email) + mail(to: email, subject: 'Password reset email') +end + +# A single recipient, just as a developer expects +insecure_email("person@example.com") + +# Multiple emails sent when an array is passed +insecure_email(["person@example.com", "attacker@evil.com"]) + +# Multiple emails sent even when a single string is passed +insecure_email("person@example.com, attacker@evil.com") +``` + +### Prevention and defense + +- Use `Gitlab::Email::SingleRecipientValidator` when adding new emails intended for a single recipient +- Strongly type your code by calling `.to_s` on values, or check its class with `value.kind_of?(String)` + +## Request Parameter Typing + +This Secure Code Guideline is enforced by the `StrongParams` RuboCop. + +In our Rails Controllers you must use `ActionController::StrongParameters`. This ensures that we explicitly define the keys and types of input we expect in a request. It is critical for avoiding Mass Assignment in our Models. It should also be used when parameters are passed to other areas of the GitLab codebase such as Services. + +Using `params[:key]` can lead to vulnerabilities when one part of the codebase expects a type like `String`, but gets passed (and handles unsafely and without error) an `Array`. + +{{< alert type="note" >}} + +This only applies to Rails Controllers. Our API and GraphQL endpoints enforce strong typing, and Go is statically typed. + +{{< /alert >}} + +### Example + +```ruby +class MyMailer + def reset(user, email) + mail(to: email, subject: 'Password reset email', body: user.reset_token) + end +end + +class MyController + + # Bad - email could be an array of values + # ?user[email]=VALUE will find a single user and email a single user + # ?user[email][]=victim@example.com&user[email][]=attacker@example.com will email the victim's token to the victim and user + def dangerously_reset_password + user = User.find_by(email: params[:user][:email]) + MyMailer.reset(user, params[:user][:email]) + end + + # Good - we use StrongParams which doesn't permit the Array type + # ?user[email]=VALUE will find a single user and email a single user + # ?user[email][]=victim@example.com&user[email][]=attacker@example.com will fail because there is no permitted :email key + def safely_reset_password + user = User.find_by(email: email_params[:email]) + MyMailer.reset(user, email_params[:email]) + end + + # This returns a new ActionController::Parameters that includes only the permitted attributes + def email_params + params.require(:user).permit(:email) + end +end +``` + +This class of issue applies to more than just email; other examples might include: + +- Allowing multiple One Time Password attempts in a single request: `?otp_attempt[]=000000&otp_attempt[]=000001&otp_attempt[]=000002...` +- Passing unexpected parameters like `is_admin` that are later `.merged` in a Service class + +### Related topics + +- [Watch a walkthrough video](https://www.youtube.com/watch?v=ydg95R2QKwM) for an instance of this issue causing vulnerability CVE-2023-7028. + The video covers what happened, how it worked, and what you need to know for the future. +- Rails documentation for [ActionController::StrongParameters](https://api.rubyonrails.org/classes/ActionController/StrongParameters.html) and [ActionController::Parameters](https://api.rubyonrails.org/classes/ActionController/Parameters.html) -- GitLab From d77b35cf2820ea4217327d9234c28ee0c103f13c Mon Sep 17 00:00:00 2001 From: Ameya Darshan Date: Thu, 18 Sep 2025 08:14:58 +0530 Subject: [PATCH 2/9] Remove path traversal guidelines from index --- .../secure_coding_guidelines/_index.md | 130 +----------------- 1 file changed, 1 insertion(+), 129 deletions(-) diff --git a/doc/development/secure_coding_guidelines/_index.md b/doc/development/secure_coding_guidelines/_index.md index 151f6043f5d4c6..86084eefb3145b 100644 --- a/doc/development/secure_coding_guidelines/_index.md +++ b/doc/development/secure_coding_guidelines/_index.md @@ -488,27 +488,7 @@ After you've [determined when and where](#setting-expectations) the user submitt #### XSS mitigation and prevention in Rails -By default, Rails automatically escapes strings when they are inserted into HTML templates. Avoid the -methods used to keep Rails from escaping strings, especially those related to user-controlled values. -Specifically, the following options are dangerous because they mark strings as trusted and safe: - -| Method | Avoid these options | -|----------------------|-------------------------------| -| HAML templates | `html_safe`, `raw`, `!=` | -| Embedded Ruby (ERB) | `html_safe`, `raw`, `<%== %>` | - -In case you want to sanitize user-controlled values against XSS vulnerabilities, you can use -[`ActionView::Helpers::SanitizeHelper`](https://api.rubyonrails.org/classes/ActionView/Helpers/SanitizeHelper.html). -Calling `link_to` and `redirect_to` with user-controlled parameters can also lead to cross-site scripting. - -Do also sanitize and validate URL schemes. - -References: - -- [XSS Defense in Rails](https://youtu.be/2VFavqfDS6w?t=2442) -- [XSS Defense with HAML](https://youtu.be/2VFavqfDS6w?t=2796) -- [Validating Untrusted URLs in Ruby](https://youtu.be/2VFavqfDS6w?t=3936) -- [RoR Model Validators](https://youtu.be/2VFavqfDS6w?t=7636) +Refer to Ruby docs #### XSS mitigation and prevention in JavaScript and Vue @@ -587,114 +567,6 @@ In order to prevent Path Traversal vulnerabilities, user-controlled filenames or - Comparing user input against an allowlist of allowed values or verifying that it only contains allowed characters. - After validating the user supplied input, it should be appended to the base directory and the path should be canonicalized using the file system API. -#### GitLab specific validations - -The methods `Gitlab::PathTraversal.check_path_traversal!()` and `Gitlab::PathTraversal.check_allowed_absolute_path!()` -can be used to validate user-supplied paths and prevent vulnerabilities. -`check_path_traversal!()` will detect their Path Traversal payloads and accepts URL-encoded paths. -`check_allowed_absolute_path!()` will check if a path is absolute and whether it is inside the allowed path list. By default, absolute -paths are not allowed, so you need to pass a list of allowed absolute paths to the `path_allowlist` -parameter when using `check_allowed_absolute_path!()`. - -To use a combination of both checks, follow the example below: - -```ruby -Gitlab::PathTraversal.check_allowed_absolute_path_and_path_traversal!(path, path_allowlist) -``` - -In the REST API, we have the [`FilePath`](https://gitlab.com/gitlab-org/security/gitlab/-/blob/master/lib/api/validations/validators/file_path.rb) -validator that can be used to perform the checking on any file path argument the endpoints have. -It can be used as follows: - -```ruby -requires :file_path, type: String, file_path: { allowlist: ['/foo/bar/', '/home/foo/', '/app/home'] } -``` - -The Path Traversal check can also be used to forbid any absolute path: - -```ruby -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 -typical 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! -``` - -#### Go - -Go 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`: - -```go -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! -``` - -#### Safe File Operations in Go - -The Go standard library provides basic file operations like `os.Open`, `os.ReadFile`, `os.WriteFile`, and `os.Readlink`. However, these functions do not prevent path traversal attacks, where user-supplied paths can escape the intended directory and access sensitive system files. - -Example of unsafe usage: - -```go -// Vulnerable: user input is directly used in the path -os.Open(filepath.Join("/app/data", userInput)) -os.ReadFile(filepath.Join("/app/data", userInput)) -os.WriteFile(filepath.Join("/app/data", userInput), []byte("data"), 0644) -os.Readlink(filepath.Join("/app/data", userInput)) -``` - -To mitigate these risks, use the [`safeopen`](https://pkg.go.dev/github.com/google/safeopen) library functions. These functions enforce a secure root directory and sanitize file paths: - -Example of safe usage: - -```go -safeopen.OpenBeneath("/app/data", userInput) -safeopen.ReadFileBeneath("/app/data", userInput) -safeopen.WriteFileBeneath("/app/data", []byte("data"), 0644) -safeopen.ReadlinkBeneath("/app/data", userInput) -``` - -Benefits: - -- Prevents path traversal attacks (`../` sequences). -- Restricts file operations to trusted root directories. -- Secures against unauthorized file reads, writes, and symlink resolutions. -- Provides simple, developer-friendly replacements. - -References: - -- [Go Standard Library os Package](https://pkg.go.dev/os) -- [Safe Go Libraries Announcement](https://bughunters.google.com/blog/4925068200771584/the-family-of-safe-golang-libraries-is-growing) -- [OWASP Path Traversal Cheat Sheet](https://owasp.org/www-community/attacks/Path_Traversal) - ## General recommendations ### TLS minimum recommended version -- GitLab From 808c16efa308d088af57f94e258ffd3bcbe2f228 Mon Sep 17 00:00:00 2001 From: Ameya Darshan Date: Thu, 18 Sep 2025 08:21:21 +0530 Subject: [PATCH 3/9] Relocate serialization and metaprogramming into Ruby --- .../secure_coding_guidelines/_index.md | 120 +----------------- .../secure_coding_guidelines/ruby.md | 115 +++++++++++++++++ 2 files changed, 120 insertions(+), 115 deletions(-) diff --git a/doc/development/secure_coding_guidelines/_index.md b/doc/development/secure_coding_guidelines/_index.md index 86084eefb3145b..2892aee1b9463b 100644 --- a/doc/development/secure_coding_guidelines/_index.md +++ b/doc/development/secure_coding_guidelines/_index.md @@ -567,6 +567,11 @@ In order to prevent Path Traversal vulnerabilities, user-controlled filenames or - Comparing user input against an allowlist of allowed values or verifying that it only contains allowed characters. - After validating the user supplied input, it should be appended to the base directory and the path should be canonicalized using the file system API. +For language-specific guidelines, refer to the following docs: + +1. Ruby +2. Go + ## General recommendations ### TLS minimum recommended version @@ -702,93 +707,6 @@ In order to prevent this from happening, it is recommended to use the method `us end ``` -## Guidelines when defining missing methods with metaprogramming - -Metaprogramming is a way to define methods **at runtime**, instead of at the time of writing and deploying the code. It is a powerful tool, but can be dangerous if we allow untrusted actors (like users) to define their own arbitrary methods. For example, imagine we accidentally let an attacker overwrite an access control method to always return true! It can lead to many classes of vulnerabilities such as access control bypass, information disclosure, arbitrary file reads, and remote code execution. - -Key methods to watch out for are `method_missing`, `define_method`, `delegate`, and similar methods. - -### Insecure metaprogramming example - -This example is adapted from an example submitted by [@jobert](https://hackerone.com/jobert?type=user) through our HackerOne bug bounty program. -Thank you for your contribution! - -Before Ruby 2.5.1, you could implement delegators using the `delegate` or `method_missing` methods. For example: - -```ruby -class User - def initialize(attributes) - @options = OpenStruct.new(attributes) - end - - def is_admin? - name.eql?("Sid") # Note - never do this! - end - - def method_missing(method, *args) - @options.send(method, *args) - end -end -``` - -When a method was called on a `User` instance that didn't exist, it passed it along to the `@options` instance variable. - -```ruby -User.new({name: "Jeeves"}).is_admin? -# => false - -User.new(name: "Sid").is_admin? -# => true - -User.new(name: "Jeeves", "is_admin?" => true).is_admin? -# => false -``` - -Because the `is_admin?` method is already defined on the class, its behavior is not overridden when passing `is_admin?` to the initializer. - -This class can be refactored to use the `Forwardable` method and `def_delegators`: - -```ruby -class User - extend Forwardable - - def initialize(attributes) - @options = OpenStruct.new(attributes) - - self.class.instance_eval do - def_delegators :@options, *attributes.keys - end - end - - def is_admin? - name.eql?("Sid") # Note - never do this! - end -end -``` - -It might seem like this example has the same behavior as the first code example. However, there's one crucial difference: **because the delegators are meta-programmed after the class is loaded, it can overwrite existing methods**: - -```ruby -User.new({name: "Jeeves"}).is_admin? -# => false - -User.new(name: "Sid").is_admin? -# => true - -User.new(name: "Jeeves", "is_admin?" => true).is_admin? -# => true -# ^------------------ The method is overwritten! Sneaky Jeeves! -``` - -In the example above, the `is_admin?` method is overwritten when passing it to the initializer. - -### Best practices - -- Never pass user-provided details into method-defining metaprogramming methods. - - If you must, be **very** confident that you've sanitized the values correctly. - Consider creating an allowlist of values, and validating the user input against that. -- When extending classes that use metaprogramming, make sure you don't inadvertently override any method definition safety checks. - ## Time of check to time of use bugs Time of check to time of use, or TOCTOU, is a class of error which occur when the state of something changes unexpectedly partway during a process. @@ -940,34 +858,6 @@ class User end ``` -## Serialization - -Serialization of active record models can leak sensitive attributes if they are not protected. - -Using the [`prevent_from_serialization`](https://gitlab.com/gitlab-org/gitlab/-/blob/d7b85128c56cc3e669f72527d9f9acc36a1da95c/app/models/concerns/sensitive_serializable_hash.rb#L11) -method protects the attributes when the object is serialized with `serializable_hash`. -When an attribute is protected with `prevent_from_serialization`, it is not included with -`serializable_hash`, `to_json`, or `as_json`. - -For more guidance on serialization: - -- [Why using a serializer is important](https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/serializers/README.md#why-using-a-serializer-is-important). -- Always use [Grape entities](api_styleguide.md#entities) for the API. - -To `serialize` an `ActiveRecord` column: - -- You can use `app/serializers`. -- You cannot use `to_json / as_json`. -- You cannot use `serialize :some_colum`. - -### Serialization example - -The following is an example used for the [`TokenAuthenticatable`](https://gitlab.com/gitlab-org/gitlab/-/blob/9b15c6621588fce7a80e0438a39eeea2500fa8cd/app/models/concerns/token_authenticatable.rb#L30) class: - -```ruby -prevent_from_serialization(*strategy.token_fields) if respond_to?(:prevent_from_serialization) -``` - ## Artificial Intelligence (AI) features The key principle is to treat AI systems as other software: apply standard software security practices. diff --git a/doc/development/secure_coding_guidelines/ruby.md b/doc/development/secure_coding_guidelines/ruby.md index 4ef75e43c4869a..3d8fa49aec0d61 100644 --- a/doc/development/secure_coding_guidelines/ruby.md +++ b/doc/development/secure_coding_guidelines/ruby.md @@ -862,3 +862,118 @@ This class of issue applies to more than just email; other examples might includ - [Watch a walkthrough video](https://www.youtube.com/watch?v=ydg95R2QKwM) for an instance of this issue causing vulnerability CVE-2023-7028. The video covers what happened, how it worked, and what you need to know for the future. - Rails documentation for [ActionController::StrongParameters](https://api.rubyonrails.org/classes/ActionController/StrongParameters.html) and [ActionController::Parameters](https://api.rubyonrails.org/classes/ActionController/Parameters.html) + +## Guidelines when defining missing methods with metaprogramming + +Metaprogramming is a way to define methods **at runtime**, instead of at the time of writing and deploying the code. It is a powerful tool, but can be dangerous if we allow untrusted actors (like users) to define their own arbitrary methods. For example, imagine we accidentally let an attacker overwrite an access control method to always return true! It can lead to many classes of vulnerabilities such as access control bypass, information disclosure, arbitrary file reads, and remote code execution. + +Key methods to watch out for are `method_missing`, `define_method`, `delegate`, and similar methods. + +### Insecure metaprogramming example + +This example is adapted from an example submitted by [@jobert](https://hackerone.com/jobert?type=user) through our HackerOne bug bounty program. +Thank you for your contribution! + +Before Ruby 2.5.1, you could implement delegators using the `delegate` or `method_missing` methods. For example: + +```ruby +class User + def initialize(attributes) + @options = OpenStruct.new(attributes) + end + + def is_admin? + name.eql?("Sid") # Note - never do this! + end + + def method_missing(method, *args) + @options.send(method, *args) + end +end +``` + +When a method was called on a `User` instance that didn't exist, it passed it along to the `@options` instance variable. + +```ruby +User.new({name: "Jeeves"}).is_admin? +# => false + +User.new(name: "Sid").is_admin? +# => true + +User.new(name: "Jeeves", "is_admin?" => true).is_admin? +# => false +``` + +Because the `is_admin?` method is already defined on the class, its behavior is not overridden when passing `is_admin?` to the initializer. + +This class can be refactored to use the `Forwardable` method and `def_delegators`: + +```ruby +class User + extend Forwardable + + def initialize(attributes) + @options = OpenStruct.new(attributes) + + self.class.instance_eval do + def_delegators :@options, *attributes.keys + end + end + + def is_admin? + name.eql?("Sid") # Note - never do this! + end +end +``` + +It might seem like this example has the same behavior as the first code example. However, there's one crucial difference: **because the delegators are meta-programmed after the class is loaded, it can overwrite existing methods**: + +```ruby +User.new({name: "Jeeves"}).is_admin? +# => false + +User.new(name: "Sid").is_admin? +# => true + +User.new(name: "Jeeves", "is_admin?" => true).is_admin? +# => true +# ^------------------ The method is overwritten! Sneaky Jeeves! +``` + +In the example above, the `is_admin?` method is overwritten when passing it to the initializer. + +### Best practices + +- Never pass user-provided details into method-defining metaprogramming methods. + - If you must, be **very** confident that you've sanitized the values correctly. + Consider creating an allowlist of values, and validating the user input against that. +- When extending classes that use metaprogramming, make sure you don't inadvertently override any method definition safety checks. + +## Serialization + +Serialization of active record models can leak sensitive attributes if they are not protected. + +Using the [`prevent_from_serialization`](https://gitlab.com/gitlab-org/gitlab/-/blob/d7b85128c56cc3e669f72527d9f9acc36a1da95c/app/models/concerns/sensitive_serializable_hash.rb#L11) +method protects the attributes when the object is serialized with `serializable_hash`. +When an attribute is protected with `prevent_from_serialization`, it is not included with +`serializable_hash`, `to_json`, or `as_json`. + +For more guidance on serialization: + +- [Why using a serializer is important](https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/serializers/README.md#why-using-a-serializer-is-important). +- Always use [Grape entities](api_styleguide.md#entities) for the API. + +To `serialize` an `ActiveRecord` column: + +- You can use `app/serializers`. +- You cannot use `to_json / as_json`. +- You cannot use `serialize :some_colum`. + +### Serialization example + +The following is an example used for the [`TokenAuthenticatable`](https://gitlab.com/gitlab-org/gitlab/-/blob/9b15c6621588fce7a80e0438a39eeea2500fa8cd/app/models/concerns/token_authenticatable.rb#L30) class: + +```ruby +prevent_from_serialization(*strategy.token_fields) if respond_to?(:prevent_from_serialization) +``` -- GitLab From 69e92a5a6eef8b3e40536147f52deadc653864d3 Mon Sep 17 00:00:00 2001 From: Ameya Darshan Date: Tue, 23 Sep 2025 19:41:49 +0530 Subject: [PATCH 4/9] Fix typos --- doc/development/secure_coding_guidelines/_index.md | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/doc/development/secure_coding_guidelines/_index.md b/doc/development/secure_coding_guidelines/_index.md index 2892aee1b9463b..071901c40b960b 100644 --- a/doc/development/secure_coding_guidelines/_index.md +++ b/doc/development/secure_coding_guidelines/_index.md @@ -341,7 +341,7 @@ have been reported to GitLab include: ### When to Consider -- When the application makes any outbound connection +When the application makes any outbound connection. ### Mitigations @@ -355,11 +355,11 @@ The preferred SSRF mitigations within GitLab are: #### GitLab HTTP Library -Refer to Ruby docs +Refer to the Ruby docs. #### URL blocker & validation libraries -Refer to Ruby docs +Refer to the Ruby docs. #### Feature-specific mitigations @@ -486,10 +486,6 @@ After you've [determined when and where](#setting-expectations) the user submitt ### Additional information -#### XSS mitigation and prevention in Rails - -Refer to Ruby docs - #### XSS mitigation and prevention in JavaScript and Vue - When updating the content of an HTML element using JavaScript, mark user-controlled values as `textContent` or `nodeValue` instead of `innerHTML`. @@ -569,8 +565,8 @@ In order to prevent Path Traversal vulnerabilities, user-controlled filenames or For language-specific guidelines, refer to the following docs: -1. Ruby -2. Go +- Ruby +- Go ## General recommendations -- GitLab From 726fc6872408ab4ca8654b9cfa42818473c8e8a9 Mon Sep 17 00:00:00 2001 From: Ameya Darshan Date: Wed, 24 Sep 2025 19:21:05 +0530 Subject: [PATCH 5/9] Fix markdown errors --- .../secure_coding_guidelines/_index.md | 18 +++++++++--------- doc/development/secure_coding_guidelines/go.md | 10 +++++++++- .../secure_coding_guidelines/ruby.md | 9 +++++++++ 3 files changed, 27 insertions(+), 10 deletions(-) diff --git a/doc/development/secure_coding_guidelines/_index.md b/doc/development/secure_coding_guidelines/_index.md index 071901c40b960b..84240cd39f5f61 100644 --- a/doc/development/secure_coding_guidelines/_index.md +++ b/doc/development/secure_coding_guidelines/_index.md @@ -16,21 +16,21 @@ For each of the vulnerabilities listed in this document, AppSec aims to have a S | Guideline | Status | Rule | |-------------------------------------------------------------------------------------|--------|------| -| [Regular Expressions](#regular-expressions-guidelines) | ✅ | [1](https://gitlab.com/gitlab-com/gl-security/product-security/appsec/sast-custom-rules/-/blob/main/secure-coding-guidelines/ruby/ruby_insecure_regex.yml) | +| Regular Expressions | ✅ | [1](https://gitlab.com/gitlab-com/gl-security/product-security/appsec/sast-custom-rules/-/blob/main/secure-coding-guidelines/ruby/ruby_insecure_regex.yml) | | [ReDOS](#denial-of-service-redos--catastrophic-backtracking) | ✅ | [1](https://gitlab.com/gitlab-com/gl-security/product-security/appsec/sast-custom-rules/-/blob/main/secure-coding-guidelines/ruby/ruby_redos_1.yml), [2](https://gitlab.com/gitlab-com/gl-security/product-security/appsec/sast-custom-rules/-/blob/main/secure-coding-guidelines/ruby/ruby_redos_2.yml), [3](https://gitlab.com/gitlab-com/gl-security/product-security/appsec/sast-custom-rules/-/merge_requests/59#note_2443657926) | | [JWT](#json-web-tokens-jwt) | ❌ | Pending | | [SSRF](#server-side-request-forgery-ssrf) | ✅ | [1](https://gitlab.com/gitlab-com/gl-security/product-security/appsec/sast-custom-rules/-/blob/main/secure-coding-guidelines/ruby/ruby_insecure_url-1.yml), [2](https://gitlab.com/gitlab-com/gl-security/product-security/appsec/sast-custom-rules/-/blob/main/secure-coding-guidelines/ruby/ruby_insecure_http.yml?ref_type=heads) | | [XSS](#xss-guidelines) | ✅ | [1](https://gitlab.com/gitlab-com/gl-security/product-security/appsec/sast-custom-rules/-/blob/main/secure-coding-guidelines/ruby/ruby_xss_redirect.yml), [2](https://gitlab.com/gitlab-com/gl-security/product-security/appsec/sast-custom-rules/-/blob/main/secure-coding-guidelines/ruby/ruby_xss_html_safe.yml) | -| [XXE](#xml-external-entities) | ✅ | [1](https://gitlab.com/gitlab-com/gl-security/product-security/appsec/sast-custom-rules/-/blob/main/secure-coding-guidelines/ruby/ruby_xml_injection_change_unsafe_nokogiri_parse_option.yml?ref_type=heads), [2](https://gitlab.com/gitlab-com/gl-security/product-security/appsec/sast-custom-rules/-/blob/main/secure-coding-guidelines/ruby/ruby_xml_injection_initialize_unsafe_nokogiri_parse_option.yml?ref_type=heads), [3](https://gitlab.com/gitlab-com/gl-security/product-security/appsec/sast-custom-rules/-/blob/main/secure-coding-guidelines/ruby/ruby_xml_injection_set_unsafe_nokogiri_parse_option.yml?ref_type=heads), [4](https://gitlab.com/gitlab-com/gl-security/product-security/appsec/sast-custom-rules/-/blob/main/secure-coding-guidelines/ruby/ruby_xml_injection_unsafe_xml_libraries.yml?ref_type=heads) | +| XXE | ✅ | [1](https://gitlab.com/gitlab-com/gl-security/product-security/appsec/sast-custom-rules/-/blob/main/secure-coding-guidelines/ruby/ruby_xml_injection_change_unsafe_nokogiri_parse_option.yml?ref_type=heads), [2](https://gitlab.com/gitlab-com/gl-security/product-security/appsec/sast-custom-rules/-/blob/main/secure-coding-guidelines/ruby/ruby_xml_injection_initialize_unsafe_nokogiri_parse_option.yml?ref_type=heads), [3](https://gitlab.com/gitlab-com/gl-security/product-security/appsec/sast-custom-rules/-/blob/main/secure-coding-guidelines/ruby/ruby_xml_injection_set_unsafe_nokogiri_parse_option.yml?ref_type=heads), [4](https://gitlab.com/gitlab-com/gl-security/product-security/appsec/sast-custom-rules/-/blob/main/secure-coding-guidelines/ruby/ruby_xml_injection_unsafe_xml_libraries.yml?ref_type=heads) | | [Path traversal](#path-traversal-guidelines) (Ruby) | ✅ | [1](https://gitlab.com/gitlab-com/gl-security/product-security/appsec/sast-custom-rules/-/blob/main/secure-coding-guidelines/ruby/ruby_path_traversal.yml?ref_type=heads) | | [Path traversal](#path-traversal-guidelines) (Go) | ✅ | [1](https://gitlab.com/gitlab-com/gl-security/product-security/appsec/sast-custom-rules/-/merge_requests/39) | -| [OS command injection](#os-command-injection-guidelines) (Ruby) | ✅ | [1](https://gitlab.com/gitlab-com/gl-security/product-security/appsec/sast-custom-rules/-/blob/main/secure-coding-guidelines/ruby/ruby_command_injection.yml?ref_type=heads) | -| [OS command injection](#os-command-injection-guidelines) (Go) | ✅ | [1](https://gitlab.com/gitlab-com/gl-security/product-security/appsec/sast-custom-rules/-/blob/main/secure-coding-guidelines/go/go_dangerous_exec_command.yml?ref_type=heads) | +| OS command injection (Ruby) | ✅ | [1](https://gitlab.com/gitlab-com/gl-security/product-security/appsec/sast-custom-rules/-/blob/main/secure-coding-guidelines/ruby/ruby_command_injection.yml?ref_type=heads) | +| OS command injection] (Go) | ✅ | [1](https://gitlab.com/gitlab-com/gl-security/product-security/appsec/sast-custom-rules/-/blob/main/secure-coding-guidelines/go/go_dangerous_exec_command.yml?ref_type=heads) | | [Insecure TLS ciphers](#tls-minimum-recommended-version) | ✅ | [1](https://gitlab.com/gitlab-com/gl-security/product-security/appsec/sast-custom-rules/-/blob/main/secure-coding-guidelines/ruby/ruby_insecure_ciphers.yml?ref_type=heads) | -| [Archive operations](#working-with-archive-files) (Ruby) | ✅ | [1](https://gitlab.com/gitlab-com/gl-security/product-security/appsec/sast-custom-rules/-/blob/main/secure-coding-guidelines/ruby/ruby_insecure_archive_operations.yml?ref_type=heads) | -| [Archive operations](#working-with-archive-files) (Go) | ✅ | [1](https://gitlab.com/gitlab-com/gl-security/product-security/appsec/sast-custom-rules/-/blob/main/secure-coding-guidelines/go/go_insecure_archive_operations.yml) | -| [URL spoofing](#url-spoofing) | ✅ | [1](https://gitlab.com/gitlab-com/gl-security/product-security/appsec/sast-custom-rules/-/blob/main/secure-coding-guidelines/ruby/ruby_url_spoofing.yml) | -| [Request Parameter Typing](#request-parameter-typing) | ✅ | `StrongParams` RuboCop | +| Archive operations (Ruby) | ✅ | [1](https://gitlab.com/gitlab-com/gl-security/product-security/appsec/sast-custom-rules/-/blob/main/secure-coding-guidelines/ruby/ruby_insecure_archive_operations.yml?ref_type=heads) | +| Archive operations (Go) | ✅ | [1](https://gitlab.com/gitlab-com/gl-security/product-security/appsec/sast-custom-rules/-/blob/main/secure-coding-guidelines/go/go_insecure_archive_operations.yml) | +| URL spoofing | ✅ | [1](https://gitlab.com/gitlab-com/gl-security/product-security/appsec/sast-custom-rules/-/blob/main/secure-coding-guidelines/ruby/ruby_url_spoofing.yml) | +| Request Parameter Typing | ✅ | `StrongParams` RuboCop | | [Paid tiers for vulnerability mitigation](#paid-tiers-for-vulnerability-mitigation) | | N/A | ## Process for creating new guidelines and accompanying rules @@ -768,7 +768,7 @@ This sensitive data must be handled carefully to avoid leaks which could lead to - Credentials must be stored as salted hashes, at rest, where the plaintext value itself does not need to be retrieved. - When the intention is to only compare secrets, store only the salted hash of the secret instead of the encrypted value. - - If the plain text value of the credentials needs to be retrieved, those credentials must be encrypted at rest (database or file) with [`encrypts`](#examples-5). + - If the plain text value of the credentials needs to be retrieved, those credentials must be encrypted at rest (database or file) with `encrypts`. - 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. diff --git a/doc/development/secure_coding_guidelines/go.md b/doc/development/secure_coding_guidelines/go.md index 4fbd9a818d8e23..2051e2accc0534 100644 --- a/doc/development/secure_coding_guidelines/go.md +++ b/doc/development/secure_coding_guidelines/go.md @@ -1,3 +1,12 @@ +--- +stage: none +group: unassigned +info: Any user with at least the Maintainer role can merge updates to this content. For details, see https://docs.gitlab.com/development/development_processes/#development-guidelines-review. +title: Secure coding development guidelines +--- + +This document contains descriptions and guidelines for secure Go programming practices commonly needed in the GitLab codebase. These guidelines are intended to help developers write secure Go code from the start, identify potential security vulnerabilities early in the development process, and follow Go-specific best practices. By adhering to these standards, we aim to reduce the number of security vulnerabilities released over time while leveraging Go's built-in security features effectively. + ## Regular Expressions guidelines ### Escape sequences in Go @@ -73,7 +82,6 @@ In order to prevent Path Traversal vulnerabilities, user-controlled filenames or - Comparing user input against an allowlist of allowed values or verifying that it only contains allowed characters. - After validating the user supplied input, it should be appended to the base directory and the path should be canonicalized using the file system API. - Go has unintuitive 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`: ```go diff --git a/doc/development/secure_coding_guidelines/ruby.md b/doc/development/secure_coding_guidelines/ruby.md index 3d8fa49aec0d61..f5ba0abd616f59 100644 --- a/doc/development/secure_coding_guidelines/ruby.md +++ b/doc/development/secure_coding_guidelines/ruby.md @@ -1,3 +1,12 @@ +--- +stage: none +group: unassigned +info: Any user with at least the Maintainer role can merge updates to this content. For details, see https://docs.gitlab.com/development/development_processes/#development-guidelines-review. +title: Secure coding development guidelines +--- + +This document contains descriptions and guidelines for secure Ruby programming practices commonly needed in the GitLab codebase. These guidelines are intended to help developers write secure Ruby code from the start, identify potential security vulnerabilities early in the development process, and follow Ruby-specific best practices. By adhering to these standards, we aim to reduce the number of security vulnerabilities released over time while leveraging Ruby's built-in security features effectively. + ## Regular Expressions guidelines ### Anchors / Multi line in Ruby -- GitLab From e6913f78f518c0a8b4b03b661af9bb4580452d15 Mon Sep 17 00:00:00 2001 From: Ameya Darshan Date: Wed, 24 Sep 2025 19:48:05 +0530 Subject: [PATCH 6/9] Fix broken links --- doc/development/ai_features/_index.md | 2 +- doc/development/application_secrets.md | 2 +- doc/development/cookies.md | 2 +- doc/development/development_processes.md | 2 +- doc/development/permissions/authorizations.md | 2 +- .../secure_coding_guidelines/_index.md | 24 +++++++++---------- .../secure_coding_guidelines/ruby.md | 16 +++---------- doc/development/token_authenticatable.md | 2 +- 8 files changed, 21 insertions(+), 31 deletions(-) diff --git a/doc/development/ai_features/_index.md b/doc/development/ai_features/_index.md index 997691b939f7e3..c34ba87bcc6f65 100644 --- a/doc/development/ai_features/_index.md +++ b/doc/development/ai_features/_index.md @@ -342,7 +342,7 @@ An [example](https://gitlab.com/gitlab-org/modelops/applied-ml/code-suggestions/ ## Security -Refer to the [secure coding guidelines for Artificial Intelligence (AI) features](../secure_coding_guidelines.md#artificial-intelligence-ai-features). +Refer to the [secure coding guidelines for Artificial Intelligence (AI) features](../secure_coding_guidelines/_index.md#artificial-intelligence-ai-features). ## Help diff --git a/doc/development/application_secrets.md b/doc/development/application_secrets.md index 4323aa708b4531..40ac4325fcbf08 100644 --- a/doc/development/application_secrets.md +++ b/doc/development/application_secrets.md @@ -18,7 +18,7 @@ Broadly speaking, there are two classes of secrets: 1. **Application secrets.** The GitLab application uses these to implement a particular feature or function. An example would be access tokens or private keys to create cryptographic signatures. We store these secrets in the database in encrypted columns. - See [Secure Coding Guidelines: At rest](secure_coding_guidelines.md#at-rest). + See [Secure Coding Guidelines: At rest](secure_coding_guidelines/_index.md#at-rest). 1. **Operational secrets.** Used to read and store other secrets or bootstrap the application. For this reason, they cannot be stored in the database. These secrets are stored as [Rails credentials](https://guides.rubyonrails.org/security.html#environmental-security) diff --git a/doc/development/cookies.md b/doc/development/cookies.md index eacebd15e25976..6ea8f985325421 100644 --- a/doc/development/cookies.md +++ b/doc/development/cookies.md @@ -7,7 +7,7 @@ title: Cookies In general, there is usually a better place to store data for users than in cookies. For backend development PostgreSQL, Redis, and [session storage](session.md) are available. For frontend development, cookies may be more secure than `localStorage`, `IndexedDB` or other options. -In general do not put sensitive information such as user IDs, potentially user-identifying information, tokens, or other secrets into cookies. See our [Secure Coding Guidelines](secure_coding_guidelines.md) for more information. +In general do not put sensitive information such as user IDs, potentially user-identifying information, tokens, or other secrets into cookies. See our [Secure Coding Guidelines](secure_coding_guidelines/_index.md) for more information. ## Cookies on Rails diff --git a/doc/development/development_processes.md b/doc/development/development_processes.md index 8ac9862b78a0e0..2161fe6052fd4b 100644 --- a/doc/development/development_processes.md +++ b/doc/development/development_processes.md @@ -16,7 +16,7 @@ Must-reads: reviewed - [Database review guidelines](database_review.md) for reviewing database-related changes and complex SQL queries, and having them reviewed -- [Secure coding guidelines](secure_coding_guidelines.md) +- [Secure coding guidelines](secure_coding_guidelines/_index.md) - [Pipelines for the GitLab project](pipelines/_index.md) - [Avoiding required stops](avoiding_required_stops.md) diff --git a/doc/development/permissions/authorizations.md b/doc/development/permissions/authorizations.md index 571311e542031e..2b84acad8db46b 100644 --- a/doc/development/permissions/authorizations.md +++ b/doc/development/permissions/authorizations.md @@ -16,7 +16,7 @@ For more information, see [guidelines for reusing abstractions](../reusing_abstr Protecting the same resources at many points means that if one layer of defense is compromised or missing, customer data is still protected by the additional layers. -For more information on permissions, see the permissions section in the [secure coding guidelines](../secure_coding_guidelines.md#permissions). +For more information on permissions, see the permissions section in the [secure coding guidelines](../secure_coding_guidelines/_index.md#permissions). ### Considerations diff --git a/doc/development/secure_coding_guidelines/_index.md b/doc/development/secure_coding_guidelines/_index.md index 84240cd39f5f61..224c16f8691425 100644 --- a/doc/development/secure_coding_guidelines/_index.md +++ b/doc/development/secure_coding_guidelines/_index.md @@ -53,7 +53,7 @@ MR. Also add the Guideline to the "SAST Coverage" table above. ### Creating new RuboCop rule -1. Follow the [RuboCop development doc](rubocop_development_guide.md#creating-new-rubocop-cops). +1. Follow the [RuboCop development doc](../rubocop_development_guide.md#creating-new-rubocop-cops). For an example, see [this merge request](https://gitlab.com/gitlab-org/gitlab-qa/-/merge_requests/1280) on adding a rule to the `gitlab-qa` project. 1. The cop itself should reside in the `gitlab-security` [gem project](https://gitlab.com/gitlab-org/ruby/gems/gitlab-styles/-/tree/master/lib/rubocop/cop/gitlab_security) @@ -62,7 +62,7 @@ MR. Also add the Guideline to the "SAST Coverage" table above. ### Description Application permissions are used to determine who can access what and what actions they can perform. -For more information about the permission model at GitLab, see [the GitLab permissions guide](permissions.md) or the [user docs on permissions](../user/permissions.md). +For more information about the permission model at GitLab, see [the GitLab permissions guide](permissions.md) or the [user docs on permissions](../../user/permissions.md). ### Impact @@ -106,7 +106,7 @@ Some example of well implemented access controls and tests: When developing features that interact with or trigger pipelines, it's essential to consider the broader implications these actions have on the system's security and operational integrity. -The [CI/CD development guidelines](cicd/_index.md) are essential reading material. No SAST or RuboCop rules enforce these guidelines. +The [CI/CD development guidelines](../cicd/_index.md) are essential reading material. No SAST or RuboCop rules enforce these guidelines. ## Denial of Service (ReDoS) / Catastrophic Backtracking @@ -490,8 +490,8 @@ After you've [determined when and where](#setting-expectations) the user submitt - When updating the content of an HTML element using JavaScript, mark user-controlled values as `textContent` or `nodeValue` instead of `innerHTML`. - Avoid using `v-html` with user-controlled data, use [`v-safe-html`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/assets/javascripts/vue_shared/directives/safe_html.js) instead. -- Render unsafe or unsanitized content using [`dompurify`](fe_guide/security.md#sanitize-html-output). -- Consider using [`gl-sprintf`](i18n/externalization.md#interpolation) to interpolate translated strings securely. +- Render unsafe or unsanitized content using [`dompurify`](../fe_guide/security.md#sanitize-html-output). +- Consider using [`gl-sprintf`](../i18n/externalization.md#interpolation) to interpolate translated strings securely. - Avoid `__()` with translations that contain user-controlled values. - When working with `postMessage`, ensure the `origin` of the message is allowlisted. - Consider using the [Safe Link Directive](https://gitlab-org.gitlab.io/gitlab-ui/?path=/story/directives-safe-link-directive--default) to generate secure hyperlinks by default. @@ -772,7 +772,7 @@ This sensitive data must be handled carefully to avoid leaks which could lead to - 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/variables_troubleshooting.md#enable-debug-logging) is enabled, all masked CI/CD variables are visible in job logs. Also consider using [protected variables](../ci/variables/_index.md#protect-a-cicd-variable) when possible so that sensitive CI/CD variables are only available to pipelines running on protected branches or protected tags. +- 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/variables_troubleshooting.md#enable-debug-logging) is enabled, all masked CI/CD variables are visible in job logs. Also consider using [protected variables](../../ci/variables/_index.md#protect-a-cicd-variable) 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://handbook.gitlab.com/handbook/security/product-security/application-security/inventory/#policies) and our [Data Classification Standards](https://handbook.gitlab.com/handbook/security/data-classification-standard/#standard). - To store and/or share credentials between teams, refer to [1Password for Teams](https://handbook.gitlab.com/handbook/security/password-guidelines/#1password-for-teams) and follow [the 1Password Guidelines](https://handbook.gitlab.com/handbook/security/password-guidelines/#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. @@ -797,7 +797,7 @@ The prefix pattern should be: Token prefixes must **not** be configurable. These are static prefixes meant for standard identification, and detection. The ability to configure the -[PAT prefix](../administration/settings/account_and_limit_settings.md#personal-access-token-prefix) +[PAT prefix](../../../administration/settings/account_and_limit_settings.md#personal-access-token-prefix) contravenes the above guidance, but is allowed as pre-existing behavior. No other tokens should have configurable token prefixes. @@ -807,7 +807,7 @@ Add the new prefix to: - The [GitLab Secret Detection rules](https://gitlab.com/gitlab-org/security-products/secret-detection/secret-detection-rules) - GitLab [secrets SAST analyzer](https://gitlab.com/gitlab-org/security-products/analyzers/secrets) - [Tokinator](https://gitlab.com/gitlab-com/gl-security/appsec/tokinator/-/blob/main/CONTRIBUTING.md?ref_type=heads) (internal tool / team members only) -- [Token Overview](../security/tokens/_index.md) documentation +- [Token Overview](../../security/tokens/_index.md) documentation Note that the token prefix is distinct to the proposed [instance token prefix](https://gitlab.com/gitlab-org/gitlab/-/issues/388379), @@ -841,7 +841,7 @@ class WebHookLog < ApplicationRecord end ``` -Using [the `TokenAuthenticatable` concern](token_authenticatable.md) to create a prefixed token **and** store the hashed value of the token, at rest: +Using [the `TokenAuthenticatable` concern](../token_authenticatable.md) to create a prefixed token **and** store the hashed value of the token, at rest: ```ruby class User @@ -1001,7 +1001,7 @@ Logging helps track events for debugging. Logging also allows the application to - Personal data, except for integer-based identifiers and UUIDs, or IP address, which can be logged when necessary. - Credentials like access tokens or passwords. If credentials must be captured for debugging purposes, log the internal ID of the credential (if available) instead. Never log credentials under any circumstances. - - When [debug logging](../ci/variables/variables_troubleshooting.md#enable-debug-logging) is enabled, all masked CI/CD variables are visible in job logs. Consider using [protected variables](../ci/variables/_index.md#protect-a-cicd-variable) when possible so that sensitive CI/CD variables are only available to pipelines running on protected branches or protected tags. + - When [debug logging](../../ci/variables/variables_troubleshooting.md#enable-debug-logging) is enabled, all masked CI/CD variables are visible in job logs. Consider using [protected variables](../../ci/variables/_index.md#protect-a-cicd-variable) when possible so that sensitive CI/CD variables are only available to pipelines running on protected branches or protected tags. - Any data supplied by the user without proper validation. - Any information that might be considered sensitive (for example, credentials, passwords, tokens, keys, or secrets). Here is an [example](https://gitlab.com/gitlab-org/gitlab/-/issues/383142) of sensitive information being leaked through logs. @@ -1014,8 +1014,8 @@ Logging helps track events for debugging. Logging also allows the application to ### Related topics -- [Log system in GitLab](../administration/logs/_index.md) -- [Audit event development guidelines](audit_event_guide/_index.md)) +- [Log system in GitLab](../../administration/logs/_index.md) +- [Audit event development guidelines](../audit_event_guide/_index.md)) - [Security logging overview](https://handbook.gitlab.com/handbook/security/security-operations/security-logging/) - [OWASP logging cheat sheet](https://cheatsheetseries.owasp.org/cheatsheets/Logging_Cheat_Sheet.html) diff --git a/doc/development/secure_coding_guidelines/ruby.md b/doc/development/secure_coding_guidelines/ruby.md index f5ba0abd616f59..659a469875afc0 100644 --- a/doc/development/secure_coding_guidelines/ruby.md +++ b/doc/development/secure_coding_guidelines/ruby.md @@ -210,7 +210,7 @@ value, we **will not** be protected against DNS rebinding. This is the case with validators such as the `AddressableUrlValidator` (called with `validates :url, addressable_url: {opts}` or `public_url: {opts}`). Validation errors are only raised when validations are called, for example when a record is created or saved. If we ignore the value returned by the validation -when persisting the record, **we need to recheck** its validity before using it. For more information, see [Time of check to time of use bugs](#time-of-check-to-time-of-use-bugs). +when persisting the record, **we need to recheck** its validity before using it. For more information, see [Time of check to time of use bugs](_index.md#time-of-check-to-time-of-use-bugs). #### Feature-specific mitigations @@ -234,7 +234,7 @@ implement. - For HTTP connections: Disable redirects or validate the redirect destination - To mitigate DNS rebinding attacks, validate and use the first IP address received. -See [`url_blocker_spec.rb`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/spec/lib/gitlab/url_blocker_spec.rb) for examples of SSRF payloads. For more information about the DNS-rebinding class of bugs, see [Time of check to time of use bugs](#time-of-check-to-time-of-use-bugs). +See [`url_blocker_spec.rb`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/spec/lib/gitlab/url_blocker_spec.rb) for examples of SSRF payloads. For more information about the DNS-rebinding class of bugs, see [Time of check to time of use bugs](_index.md#time-of-check-to-time-of-use-bugs). Don't rely on methods like `.start_with?` when validating a URL, or make assumptions about which part of a string maps to which part of a URL. Use the `URI` class to parse the string, and validate @@ -288,7 +288,7 @@ Much of the impact is contingent upon the function of the application and the ca For a demonstration of the impact on GitLab with a realistic attack scenario, see [this video on the GitLab Unfiltered channel](https://www.youtube.com/watch?v=t4PzHNycoKo) (internal, it requires being logged in with the GitLab Unfiltered account). -## XSS mitigation and prevention in Rails +### XSS mitigation and prevention in Rails By default, Rails automatically escapes strings when they are inserted into HTML templates. Avoid the methods used to keep Rails from escaping strings, especially those related to user-controlled values. @@ -312,16 +312,6 @@ References: - [Validating Untrusted URLs in Ruby](https://youtu.be/2VFavqfDS6w?t=3936) - [RoR Model Validators](https://youtu.be/2VFavqfDS6w?t=7636) -#### XSS mitigation and prevention in JavaScript and Vue - -- When updating the content of an HTML element using JavaScript, mark user-controlled values as `textContent` or `nodeValue` instead of `innerHTML`. -- Avoid using `v-html` with user-controlled data, use [`v-safe-html`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/assets/javascripts/vue_shared/directives/safe_html.js) instead. -- Render unsafe or unsanitized content using [`dompurify`](fe_guide/security.md#sanitize-html-output). -- Consider using [`gl-sprintf`](i18n/externalization.md#interpolation) to interpolate translated strings securely. -- Avoid `__()` with translations that contain user-controlled values. -- When working with `postMessage`, ensure the `origin` of the message is allowlisted. -- Consider using the [Safe Link Directive](https://gitlab-org.gitlab.io/gitlab-ui/?path=/story/directives-safe-link-directive--default) to generate secure hyperlinks by default. - ## XML external entities ### Description diff --git a/doc/development/token_authenticatable.md b/doc/development/token_authenticatable.md index d1dbd452e55bd0..67a0301af92a00 100644 --- a/doc/development/token_authenticatable.md +++ b/doc/development/token_authenticatable.md @@ -63,7 +63,7 @@ The `token_field_encrypted` column should always be indexed, because it is used - `unique: false`: Doesn't enforce token uniqueness and disables the generation of `find_by_token_field` (where `token_field` is the attribute name). Default is `true`. - `format_with_prefix: :compute_token_prefix`: Allows to define a prefix for the token. The `#compute_token_prefix` method needs to return a `String`. Default is no prefix. - See guidance on [token prefixes](secure_coding_guidelines.md#token-prefixes). + See guidance on [token prefixes](secure_coding_guidelines/_index.md#token-prefixes). - `expires_at: :compute_token_expiration_time`: Allows to define a time when the token should expire. The `#compute_token_expiration_time` method needs to return a `Time` object. Default is no expiration. - `token_generator:` A proc that returns a token. If absent, a random token is generated using `Devise.friendly_token`. -- GitLab From f8d6e4f35afe178f8069b0fae35a09cc4b33d09f Mon Sep 17 00:00:00 2001 From: Ameya Darshan Date: Wed, 24 Sep 2025 19:54:35 +0530 Subject: [PATCH 7/9] Fix more broken links --- doc/development/contributing/merge_request_workflow.md | 2 +- doc/development/secure_coding_guidelines/_index.md | 4 ++-- doc/development/secure_coding_guidelines/ruby.md | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/doc/development/contributing/merge_request_workflow.md b/doc/development/contributing/merge_request_workflow.md index e9ee41f36eab44..b8d3e117955897 100644 --- a/doc/development/contributing/merge_request_workflow.md +++ b/doc/development/contributing/merge_request_workflow.md @@ -211,7 +211,7 @@ requirements. 1. Working and clean code that is commented where needed. 1. The change is evaluated to [limit the impact of far-reaching work](https://handbook.gitlab.com/handbook/engineering/core-development/#reducing-the-impact-of-far-reaching-work). 1. [Performance guidelines](../merge_request_concepts/performance.md) have been followed. -1. [Secure coding guidelines](../secure_coding_guidelines.md) have been followed. +1. [Secure coding guidelines](../secure_coding_guidelines/_index.md) have been followed. 1. [Application and rate limit guidelines](../merge_request_concepts/rate_limits.md) have been followed. 1. [Documented](../documentation/_index.md) in the `/doc` directory. 1. If your MR touches code that executes shell commands, reads or opens files, or diff --git a/doc/development/secure_coding_guidelines/_index.md b/doc/development/secure_coding_guidelines/_index.md index 224c16f8691425..a6c59130a16f80 100644 --- a/doc/development/secure_coding_guidelines/_index.md +++ b/doc/development/secure_coding_guidelines/_index.md @@ -62,7 +62,7 @@ MR. Also add the Guideline to the "SAST Coverage" table above. ### Description Application permissions are used to determine who can access what and what actions they can perform. -For more information about the permission model at GitLab, see [the GitLab permissions guide](permissions.md) or the [user docs on permissions](../../user/permissions.md). +For more information about the permission model at GitLab, see [the GitLab permissions guide](../permissions.md) or the [user docs on permissions](../../user/permissions.md). ### Impact @@ -797,7 +797,7 @@ The prefix pattern should be: Token prefixes must **not** be configurable. These are static prefixes meant for standard identification, and detection. The ability to configure the -[PAT prefix](../../../administration/settings/account_and_limit_settings.md#personal-access-token-prefix) +[PAT prefix](../../administration/settings/account_and_limit_settings.md#personal-access-token-prefix) contravenes the above guidance, but is allowed as pre-existing behavior. No other tokens should have configurable token prefixes. diff --git a/doc/development/secure_coding_guidelines/ruby.md b/doc/development/secure_coding_guidelines/ruby.md index 659a469875afc0..1f0fc2a060c189 100644 --- a/doc/development/secure_coding_guidelines/ruby.md +++ b/doc/development/secure_coding_guidelines/ruby.md @@ -567,7 +567,7 @@ Consider using `system("command", "arg0", "arg1", ...)` whenever you can. This p from concatenating commands. For more examples on how to use shell commands securely, consult -[Guidelines for shell commands in the GitLab codebase](shell_commands.md). +[Guidelines for shell commands in the GitLab codebase](../shell_commands.md). It contains various examples on how to securely call OS commands. ## Working with archive files @@ -961,7 +961,7 @@ When an attribute is protected with `prevent_from_serialization`, it is not incl For more guidance on serialization: - [Why using a serializer is important](https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/serializers/README.md#why-using-a-serializer-is-important). -- Always use [Grape entities](api_styleguide.md#entities) for the API. +- Always use [Grape entities](../api_styleguide.md#entities) for the API. To `serialize` an `ActiveRecord` column: -- GitLab From ca29cd318e371c002cf576d545ea7f0a17e69425 Mon Sep 17 00:00:00 2001 From: Ameya Darshan Date: Wed, 24 Sep 2025 20:01:26 +0530 Subject: [PATCH 8/9] Finishing touches --- doc/development/secure_coding_guidelines/go.md | 2 -- doc/development/secure_coding_guidelines/ruby.md | 10 +++------- 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/doc/development/secure_coding_guidelines/go.md b/doc/development/secure_coding_guidelines/go.md index 2051e2accc0534..f89c2fb8c1bc96 100644 --- a/doc/development/secure_coding_guidelines/go.md +++ b/doc/development/secure_coding_guidelines/go.md @@ -162,8 +162,6 @@ where you can't avoid this: characters, for example). - Always use `--` to separate options from arguments. -#### Go - Go has built-in protections that usually prevent an attacker from successfully injecting OS commands. Consider the following example: diff --git a/doc/development/secure_coding_guidelines/ruby.md b/doc/development/secure_coding_guidelines/ruby.md index 1f0fc2a060c189..d133230ce04dc7 100644 --- a/doc/development/secure_coding_guidelines/ruby.md +++ b/doc/development/secure_coding_guidelines/ruby.md @@ -288,7 +288,7 @@ Much of the impact is contingent upon the function of the application and the ca For a demonstration of the impact on GitLab with a realistic attack scenario, see [this video on the GitLab Unfiltered channel](https://www.youtube.com/watch?v=t4PzHNycoKo) (internal, it requires being logged in with the GitLab Unfiltered account). -### XSS mitigation and prevention in Rails +### Mitigation and prevention in Rails By default, Rails automatically escapes strings when they are inserted into HTML templates. Avoid the methods used to keep Rails from escaping strings, especially those related to user-controlled values. @@ -318,7 +318,7 @@ References: XML external entity (XXE) injection is a type of attack against an application that parses XML input. This attack occurs when XML input containing a reference to an external entity is processed by a weakly configured XML parser. It can lead to disclosure of confidential data, denial of service, server-side request forgery, port scanning from the perspective of the machine where the parser is located, and other system impacts. -### XXE mitigation in Ruby +### Mitigation The two main ways we can prevent XXE vulnerabilities in our codebase are: @@ -506,8 +506,6 @@ need to provide an array of paths to the parameter `allowlist`. 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 typical use. In the examples below, we see attempts to access `/etc/passwd`, which is a sensitive file: @@ -561,8 +559,6 @@ where you can't avoid this: characters, for example). - Always use `--` to separate options from arguments. -#### Ruby - Consider using `system("command", "arg0", "arg1", ...)` whenever you can. This prevents an attacker from concatenating commands. @@ -780,7 +776,7 @@ code is secure when it merges, it's better practice to use the defense-in-depth "single recipient" check just before sending the email. This prevents a vulnerability if otherwise-vulnerable code is committed at a later date. For example: -### Example: Ruby +### Example ```ruby # Insecure if email is user-controlled -- GitLab From cf47253bca28c9ed0b1d14d66cef1d75f650c2b6 Mon Sep 17 00:00:00 2001 From: Ameya Darshan Date: Tue, 30 Sep 2025 21:16:28 +0530 Subject: [PATCH 9/9] Add missing links --- doc/development/secure_coding_guidelines/_index.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/doc/development/secure_coding_guidelines/_index.md b/doc/development/secure_coding_guidelines/_index.md index a6c59130a16f80..a2b0eb079f2f84 100644 --- a/doc/development/secure_coding_guidelines/_index.md +++ b/doc/development/secure_coding_guidelines/_index.md @@ -25,7 +25,7 @@ For each of the vulnerabilities listed in this document, AppSec aims to have a S | [Path traversal](#path-traversal-guidelines) (Ruby) | ✅ | [1](https://gitlab.com/gitlab-com/gl-security/product-security/appsec/sast-custom-rules/-/blob/main/secure-coding-guidelines/ruby/ruby_path_traversal.yml?ref_type=heads) | | [Path traversal](#path-traversal-guidelines) (Go) | ✅ | [1](https://gitlab.com/gitlab-com/gl-security/product-security/appsec/sast-custom-rules/-/merge_requests/39) | | OS command injection (Ruby) | ✅ | [1](https://gitlab.com/gitlab-com/gl-security/product-security/appsec/sast-custom-rules/-/blob/main/secure-coding-guidelines/ruby/ruby_command_injection.yml?ref_type=heads) | -| OS command injection] (Go) | ✅ | [1](https://gitlab.com/gitlab-com/gl-security/product-security/appsec/sast-custom-rules/-/blob/main/secure-coding-guidelines/go/go_dangerous_exec_command.yml?ref_type=heads) | +| OS command injection (Go) | ✅ | [1](https://gitlab.com/gitlab-com/gl-security/product-security/appsec/sast-custom-rules/-/blob/main/secure-coding-guidelines/go/go_dangerous_exec_command.yml?ref_type=heads) | | [Insecure TLS ciphers](#tls-minimum-recommended-version) | ✅ | [1](https://gitlab.com/gitlab-com/gl-security/product-security/appsec/sast-custom-rules/-/blob/main/secure-coding-guidelines/ruby/ruby_insecure_ciphers.yml?ref_type=heads) | | Archive operations (Ruby) | ✅ | [1](https://gitlab.com/gitlab-com/gl-security/product-security/appsec/sast-custom-rules/-/blob/main/secure-coding-guidelines/ruby/ruby_insecure_archive_operations.yml?ref_type=heads) | | Archive operations (Go) | ✅ | [1](https://gitlab.com/gitlab-com/gl-security/product-security/appsec/sast-custom-rules/-/blob/main/secure-coding-guidelines/go/go_insecure_archive_operations.yml) | @@ -355,11 +355,11 @@ The preferred SSRF mitigations within GitLab are: #### GitLab HTTP Library -Refer to the Ruby docs. +Refer to the [Ruby docs](ruby.md#gitlab-http-library). #### URL blocker & validation libraries -Refer to the Ruby docs. +Refer to the [Ruby docs](ruby.md#url-blocker--validation-libraries). #### Feature-specific mitigations @@ -565,8 +565,8 @@ In order to prevent Path Traversal vulnerabilities, user-controlled filenames or For language-specific guidelines, refer to the following docs: -- Ruby -- Go +- [Ruby](ruby.md#path-traversal-guidelines) +- [Go](go.md#path-traversal-guidelines) ## General recommendations -- GitLab