From 1bcf139fca64ea281a41e9f11cb649f75897dc6b Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Fri, 4 Mar 2022 14:58:51 +0100 Subject: [PATCH 1/3] SECURITY.md: add section with coding practices Signed-off-by: Patrik Oldsberg --- SECURITY.md | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/SECURITY.md b/SECURITY.md index ba96c694ee..caf90669c1 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -36,3 +36,34 @@ There are many situations where a vulnerability does not affect a particular dep To work around this and other similar issues, Snyk provides a method to ignore vulnerabilities. In order to provide the best visibility and most utility to adopters of Backstage, we store these ignore rules in `.snyk` policy files. This allows adopters to rely on our ignore policies if they wish to do so. Adding a new ignore policy is done by creating or modifying an existing `.snyk` file within a package root. See the [Snyk Documentation](https://support.snyk.io/hc/en-us/articles/360007487097-The-snyk-file) for details on the syntax. Always include a description, full path, and time limit of the ignore policy. + +## Coding Practices + +In this section we highlight patterns where particular care needs to be taken in order to avoid security vulnerabilities, as well as the best practices to follow. + +### Local file path resolution + +A common pitfall in backend packages is to resolve local file paths based on user input, without sufficiently protecting against input that traverses outside the intended directory. + +For example, consider the following code: + +```ts +import { join } from 'path'; +import fs from 'fs-extra'; + +function writeTemporaryFile(tmpDir: string, name: string, content: string) { + // WARNING: DO NOT DO THIS + const filePath = join(tmpDir, name); + await fs.writeFile(filePath, content); +} +``` + +If the `name` of the file is controlled by the user, they can for example enter `../../../../../../../../etc/hosts` as the name of the file. This can lead to a file being written outside the intended directory, which in turn can be used to inject malicious code or other form of attacks. + +The recommended solution to this is to use `resolveSafeChildPath` from `@backstage/backend-common` to resolve the file path instead. It makes sure that the resolved path does not fall outside the provided directory. If you simply what to validate whether a file path is safe, you can use `isChildPath` instead. + +### Express responses + +When returning a response from an Express route, always use `.json(...)` or `.end()` without any arguments. This ensures that the response can not be interpreted as an HTML document with embedded JavaScript by the browser. Never use `.send(...)` unless you want to send other forms of content, and be sure to always set an explicit content type. If you need to return HTML or other content that may be executed by the browser, be very careful how you handle user input. + +If you want to return an error response, simply throw the error or pass it on to the `next(...)` callback. There is a middleware installed that will transform the error into a JSON response. Many of the common HTTP errors are available from `@backstage/errors`, for example `NotFoundError`, which will also set the correct status code. From 81e0166c5de152b77d660338e986271f700e3817 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Fri, 4 Mar 2022 16:43:33 +0100 Subject: [PATCH 2/3] SECURITY.md: add more examples to the coding practices Signed-off-by: Patrik Oldsberg --- SECURITY.md | 49 +++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 47 insertions(+), 2 deletions(-) diff --git a/SECURITY.md b/SECURITY.md index caf90669c1..1ff2460d19 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -48,22 +48,67 @@ A common pitfall in backend packages is to resolve local file paths based on use For example, consider the following code: ```ts +// WARNING: DO NOT DO THIS + import { join } from 'path'; import fs from 'fs-extra'; function writeTemporaryFile(tmpDir: string, name: string, content: string) { - // WARNING: DO NOT DO THIS const filePath = join(tmpDir, name); await fs.writeFile(filePath, content); } ``` -If the `name` of the file is controlled by the user, they can for example enter `../../../../../../../../etc/hosts` as the name of the file. This can lead to a file being written outside the intended directory, which in turn can be used to inject malicious code or other form of attacks. +If the `name` of the file is controlled by the user, they can for example enter `../../../../etc/hosts` as the name of the file. This can lead to a file being written outside the intended directory, which in turn can be used to inject malicious code or other form of attacks. The recommended solution to this is to use `resolveSafeChildPath` from `@backstage/backend-common` to resolve the file path instead. It makes sure that the resolved path does not fall outside the provided directory. If you simply what to validate whether a file path is safe, you can use `isChildPath` instead. +The insecure example above should instead be written like this: + +```ts +// THIS IS GOOD, DO THIS + +import { resolveSafeChildPath } from '@backstaghe/backend-common'; +import fs from 'fs-extra'; + +function writeTemporaryFile(tmpDir: string, name: string, content: string) { + const filePath = resolveSafeChildPath(tmpDir, name); + await fs.writeFile(filePath, content); +} +``` + ### Express responses When returning a response from an Express route, always use `.json(...)` or `.end()` without any arguments. This ensures that the response can not be interpreted as an HTML document with embedded JavaScript by the browser. Never use `.send(...)` unless you want to send other forms of content, and be sure to always set an explicit content type. If you need to return HTML or other content that may be executed by the browser, be very careful how you handle user input. If you want to return an error response, simply throw the error or pass it on to the `next(...)` callback. There is a middleware installed that will transform the error into a JSON response. Many of the common HTTP errors are available from `@backstage/errors`, for example `NotFoundError`, which will also set the correct status code. + +The following example show how to return an error that contains user input: + +```ts +res.send(`Invalid id: '${req.params.id}'`); // BAD + +// import { InputError } from '@backstage/errors'; +throw new InputError(`Invalid id: '${req.params.id}'`); // GOOD + +// OR, in case a custom response is needed +res.json({ message: `Invalid id: '${req.params.id}'` }); // NOT BAD +``` + +No matter how trivial it may seem, always use `.json(...)`. It reduces the risk that a future refactoring introduces vulnerabilities: + +```ts +res.send({ ok: true }); // BAD + +res.json({ ok: true }); // GOOD +``` + +If you absolute must return a string with `.send(...)`, use an explicit and secure `Content-Type`: + +```ts +res.send(`message=${message}`); // BAD + +res.contentType('text/plain').send(`message=${message}`); // GOOD +``` + +An example of how to return dynamic HTML is not provided here. If you need to do so, proceed with extreme caution and be very sure that you know what you are doing. From 30d7d3975c68894da666de838183d17af6338087 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Sat, 5 Mar 2022 09:00:04 +0100 Subject: [PATCH 3/3] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Patrik Oldsberg Co-authored-by: Fredrik Adelöw --- SECURITY.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/SECURITY.md b/SECURITY.md index 1ff2460d19..49e26349b0 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -61,7 +61,7 @@ function writeTemporaryFile(tmpDir: string, name: string, content: string) { If the `name` of the file is controlled by the user, they can for example enter `../../../../etc/hosts` as the name of the file. This can lead to a file being written outside the intended directory, which in turn can be used to inject malicious code or other form of attacks. -The recommended solution to this is to use `resolveSafeChildPath` from `@backstage/backend-common` to resolve the file path instead. It makes sure that the resolved path does not fall outside the provided directory. If you simply what to validate whether a file path is safe, you can use `isChildPath` instead. +The recommended solution to this is to use `resolveSafeChildPath` from `@backstage/backend-common` to resolve the file path instead. It makes sure that the resolved path does not fall outside the provided directory. If you simply want to validate whether a file path is safe, you can use `isChildPath` instead. The insecure example above should instead be written like this: @@ -103,7 +103,7 @@ res.send({ ok: true }); // BAD res.json({ ok: true }); // GOOD ``` -If you absolute must return a string with `.send(...)`, use an explicit and secure `Content-Type`: +If you absolutely must return a string with `.send(...)`, use an explicit and secure `Content-Type`: ```ts res.send(`message=${message}`); // BAD