CVE-2021-43783: Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal') (CWE-22)
value: @backstage/plugin-scaffolder-backend is the backend for the default Backstage software templates. In affected versions a malicious actor with write access to a registered scaffolder template is able to manipulate the template in a way that writes files to arbitrary paths on the scaffolder-backend host instance. This vulnerability can in some situation also be exploited through user input when executing a template, meaning you do not need write access to the templates. This method will not allow the attacker to control the contents of the injected file however, unless the template is also crafted in a specific way that gives control of the file contents. This vulnerability is fixed in version `0.15.14` of the `@backstage/plugin-scaffolder-backend`. This attack is mitigated by restricting access and requiring reviews when registering or modifying scaffolder templates.
CWE description: The product uses external input to construct a pathname that is intended to identify a file or directory that is located underneath a restricted parent directory, but the product does not properly neutralize special elements within the pathname that can cause the pathname to resolve to a location that is outside of the restricted directory.
CVSS v3 base score: 8.5
Published: 2021-11-29T20:15Z
Repository: https://github.com/backstage/backstage
Language: TypeScript
Files changed: .changeset/serious-pens-tease.md, plugins/scaffolder-backend/.eslintrc.js, plugins/scaffolder-backend/src/scaffolder/actions/builtin/catalog/write.ts, plugins/scaffolder-backend/src/scaffolder/actions/builtin/debug/log.ts, plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/helpers.ts, plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/template.ts, plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.ts
Commit date: 2021-11-24 19:56:56 +0100
Commit message: scaffolder-backend: removed all usaged and prevent new usage of path.resolve

Signed-off-by: Patrik Oldsberg <poldsberg@gmail.com>

Diff: diff --git a/.changeset/serious-pens-tease.md b/.changeset/serious-pens-tease.md
new file mode 100644
index 0000000000..7759ba3bc0
--- /dev/null
+++ b/.changeset/serious-pens-tease.md
@@ -0,0 +1,5 @@
+---
+'@backstage/plugin-scaffolder-backend': patch
+---
+
+Removed all usages of `path.resolve` in order to ensure that template paths are resolved in a safe way.
diff --git a/plugins/scaffolder-backend/.eslintrc.js b/plugins/scaffolder-backend/.eslintrc.js
index 19c9ad7395..2632b5e089 100644
--- a/plugins/scaffolder-backend/.eslintrc.js
+++ b/plugins/scaffolder-backend/.eslintrc.js
@@ -1,8 +1,41 @@
+const parent = require('@backstage/cli/config/eslint.backend');
+
 module.exports = {
   extends: [require.resolve('@backstage/cli/config/eslint.backend')],
   ignorePatterns: ['sample-templates/'],
   rules: {
     'no-console': 0, // Permitted in console programs
     'new-cap': ['error', { capIsNew: false }], // Because Express constructs things e.g. like 'const r = express.Router()'
+    // Usage of path.resolve is extra sensitive in the scaffolder, so forbid it in non-test code
+    'no-restricted-imports': [
+      'error',
+      {
+        ...parent.rules['no-restricted-imports'][1],
+        paths: [
+          {
+            name: 'path',
+            importNames: ['resolve'],
+            message:
+              'Do not use path.resolve, use `resolveSafeChildPath` from `@backstage/backend-common` instead as it prevents security issues',
+          },
+        ],
+      },
+    ],
+    'no-restricted-syntax': parent.rules['no-restricted-syntax'].concat([
+      {
+        message:
+          'Do not use path.resolve, use `resolveSafeChildPath` from `@backstage/backend-common` instead as it prevents security issues',
+        selector:
+          'MemberExpression[object.name="path"][property.name="resolve"]',
+      },
+    ]),
   },
+  overrides: [
+    {
+      files: ['*.test.*', 'src/setupTests.*', 'dev/**'],
+      rules: {
+        'no-restricted-imports': parent.rules['no-restricted-imports'],
+      },
+    },
+  ],
 };
diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/catalog/write.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/catalog/write.ts
index 45481b47e6..b813244020 100644
--- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/catalog/write.ts
+++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/catalog/write.ts
@@ -13,14 +13,14 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
 
 import fs from 'fs-extra';
-import { resolve as resolvePath } from 'path';
 import { createTemplateAction } from '../../createTemplateAction';
 import * as yaml from 'yaml';
 import { Entity } from '@backstage/catalog-model';
+import { resolveSafeChildPath } from '@backstage/backend-common';
 
 export function createCatalogWriteAction() {
   return createTemplateAction<{ name?: string; entity: Entity }>({
     id: 'catalog:write',
     description: 'Writes the catalog-info.yaml for your template',
@@ -40,11 +40,11 @@ export function createCatalogWriteAction() {
     async handler(ctx) {
       ctx.logStream.write(`Writing catalog-info.yaml`);
       const { entity } = ctx.input;
 
       await fs.writeFile(
-        resolvePath(ctx.workspacePath, 'catalog-info.yaml'),
+        resolveSafeChildPath(ctx.workspacePath, 'catalog-info.yaml'),
         yaml.stringify(entity),
       );
     },
   });
 }
diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/debug/log.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/debug/log.ts
index 9bf9b7fb90..36ea842a66 100644
--- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/debug/log.ts
+++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/debug/log.ts
@@ -13,11 +13,11 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
 
 import { readdir, stat } from 'fs-extra';
-import { relative, resolve } from 'path';
+import { relative, join } from 'path';
 import { createTemplateAction } from '../../createTemplateAction';
 
 /**
  * This task is useful for local development and testing of both the scaffolder
  * and scaffolder templates.
@@ -66,11 +66,11 @@ export function createDebugLogAction() {
 
 export async function recursiveReadDir(dir: string): Promise<string[]> {
   const subdirs = await readdir(dir);
   const files = await Promise.all(
     subdirs.map(async subdir => {
-      const res = resolve(dir, subdir);
+      const res = join(dir, subdir);
       return (await stat(res)).isDirectory() ? recursiveReadDir(res) : [res];
     }),
   );
   return files.reduce((a, f) => a.concat(f), []);
 }
diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/helpers.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/helpers.ts
index 3946110d42..b56bab0d53 100644
--- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/helpers.ts
+++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/helpers.ts
@@ -17,11 +17,11 @@
 import { resolveSafeChildPath, UrlReader } from '@backstage/backend-common';
 import { JsonValue } from '@backstage/types';
 import { InputError } from '@backstage/errors';
 import { ScmIntegrations } from '@backstage/integration';
 import fs from 'fs-extra';
-import * as path from 'path';
+import path from 'path';
 
 export async function fetchContents({
   reader,
   integrations,
   baseUrl,
diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/template.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/template.ts
index 47bfcccfdf..54a2c25cf3 100644
--- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/template.ts
+++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/fetch/template.ts
@@ -12,11 +12,11 @@
  * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
 
-import { resolve as resolvePath, extname } from 'path';
+import { extname } from 'path';
 import { resolveSafeChildPath, UrlReader } from '@backstage/backend-common';
 import { InputError } from '@backstage/errors';
 import { ScmIntegrations } from '@backstage/integration';
 import { fetchContents } from './helpers';
 import { createTemplateAction } from '../../createTemplateAction';
@@ -112,11 +112,11 @@ export function createFetchTemplateAction(options: {
     },
     async handler(ctx) {
       ctx.logger.info('Fetching template content from remote URL');
 
       const workDir = await ctx.createTemporaryDirectory();
-      const templateDir = resolvePath(workDir, 'template');
+      const templateDir = resolveSafeChildPath(workDir, 'template');
 
       const targetPath = ctx.input.targetPath ?? './';
       const outputDir = resolveSafeChildPath(ctx.workspacePath, targetPath);
 
       if (
@@ -238,11 +238,11 @@ export function createFetchTemplateAction(options: {
           renderFilename = renderContents = !nonTemplatedEntries.has(location);
         }
         if (renderFilename) {
           localOutputPath = templater.renderString(localOutputPath, context);
         }
-        const outputPath = resolvePath(outputDir, localOutputPath);
+        const outputPath = resolveSafeChildPath(outputDir, localOutputPath);
         // variables have been expanded to make an empty file name
         // this is due to a conditional like if values.my_condition then file-name.txt else empty string so skip
         if (outputDir === outputPath) {
           continue;
         }
@@ -257,11 +257,11 @@ export function createFetchTemplateAction(options: {
           ctx.logger.info(
             `Writing directory ${location} to template output path.`,
           );
           await fs.ensureDir(outputPath);
         } else {
-          const inputFilePath = resolvePath(templateDir, location);
+          const inputFilePath = resolveSafeChildPath(templateDir, location);
 
           if (await isBinaryFile(inputFilePath)) {
             ctx.logger.info(
               `Copying binary file ${location} to template output path.`,
             );
diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.ts
index 0cceb47d71..3a9dd01686 100644
--- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.ts
+++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/githubPullRequest.ts
@@ -13,11 +13,10 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
 
 import fs from 'fs-extra';
-import path from 'path';
 import { parseRepoUrl, isExecutable } from './util';
 
 import {
   GithubCredentialsProvider,
   ScmIntegrationRegistry,
@@ -195,11 +194,11 @@ export const createPublishGithubPullRequestAction = ({
         dot: true,
       });
 
       const fileContents = await Promise.all(
         localFilePaths.map(filePath => {
-          const absPath = path.resolve(fileRoot, filePath);
+          const absPath = resolveSafeChildPath(fileRoot, filePath);
           const base64EncodedContent = fs
             .readFileSync(absPath)
             .toString('base64');
           const fileStat = fs.statSync(absPath);
           // See the properties of tree items

Vulnerable code: import { resolve as resolvePath } from 'path';
        resolvePath(ctx.workspacePath, 'catalog-info.yaml'),
import { relative, resolve } from 'path';
      const res = resolve(dir, subdir);
import * as path from 'path';
import { resolve as resolvePath, extname } from 'path';
      const templateDir = resolvePath(workDir, 'template');
        const outputPath = resolvePath(outputDir, localOutputPath);
          const inputFilePath = resolvePath(templateDir, location);
import path from 'path';
          const absPath = path.resolve(fileRoot, filePath);
Fixed code: ---
'@backstage/plugin-scaffolder-backend': patch
---

Removed all usages of `path.resolve` in order to ensure that template paths are resolved in a safe way.
const parent = require('@backstage/cli/config/eslint.backend');

    // Usage of path.resolve is extra sensitive in the scaffolder, so forbid it in non-test code
    'no-restricted-imports': [
      'error',
      {
        ...parent.rules['no-restricted-imports'][1],
        paths: [
          {
            name: 'path',
            importNames: ['resolve'],
            message:
              'Do not use path.resolve, use `resolveSafeChildPath` from `@backstage/backend-common` instead as it prevents security issues',
          },
        ],
      },
    ],
    'no-restricted-syntax': parent.rules['no-restricted-syntax'].concat([
      {
        message:
          'Do not use path.resolve, use `resolveSafeChildPath` from `@backstage/backend-common` instead as it prevents security issues',
        selector:
          'MemberExpression[object.name="path"][property.name="resolve"]',
      },
    ]),
  overrides: [
    {
      files: ['*.test.*', 'src/setupTests.*', 'dev/**'],
      rules: {
        'no-restricted-imports': parent.rules['no-restricted-imports'],
      },
    },
  ],
import { resolveSafeChildPath } from '@backstage/backend-common';
        resolveSafeChildPath(ctx.workspacePath, 'catalog-info.yaml'),
import { relative, join } from 'path';
      const res = join(dir, subdir);
import path from 'path';
import { extname } from 'path';
      const templateDir = resolveSafeChildPath(workDir, 'template');
        const outputPath = resolveSafeChildPath(outputDir, localOutputPath);
          const inputFilePath = resolveSafeChildPath(templateDir, location);
          const absPath = resolveSafeChildPath(fileRoot, filePath);