diff --git a/src/Validator.ts b/src/Validator.ts index c37b75d16..a3c4f2a35 100644 --- a/src/Validator.ts +++ b/src/Validator.ts @@ -18,6 +18,11 @@ import { devices } from './devices.js' import { type AMTConfiguration } from './models/index.js' import { type Configurator } from './Configurator.js' import { type DeviceCredentials } from './interfaces/ISecretManagerService.js' + +// device identifiers are alphanumerics, dots, hyphens, and underscores. consecutive dots are +// rejected separately below so an identifier can never form a '..' path segment +const SAFE_IDENTIFIER = /^[A-Za-z0-9._-]+$/ + export class Validator implements IValidator { jsonParser: ClientMsgJsonParser @@ -277,6 +282,10 @@ export class Validator implements IValidator { if (msg.payload.uuid.length !== 36) { throw new RPSError(`${clientId} - uuid not valid length`) } + // kept independent of the length check so a future identifier scheme can relax length + if (!SAFE_IDENTIFIER.test(msg.payload.uuid) || msg.payload.uuid.includes('..')) { + throw new RPSError(`${clientId} - uuid contains invalid characters`) + } return msg.payload } diff --git a/src/secrets/vault/index.test.ts b/src/secrets/vault/index.test.ts index f6dbc01c6..fc8ee8261 100644 --- a/src/secrets/vault/index.test.ts +++ b/src/secrets/vault/index.test.ts @@ -3,7 +3,7 @@ * SPDX-License-Identifier: Apache-2.0 **********************************************************************/ -import { VaultService } from './index.js' +import { VaultService, assertSafeSecretPath } from './index.js' import Logger from '../../Logger.js' import { type ILogger } from '../../interfaces/ILogger.js' import { config } from '../../test/helper/Config.js' @@ -156,3 +156,38 @@ it('should get health of vault', async () => { prefixUrl: `${Environment.Config.vault_address}/v1/` }) }) + +describe('assertSafeSecretPath', () => { + it.each([ + 'devices/../profiles/default', + 'devices/../certs/acm-domain', + 'devices/..%2fprofiles%2fdefault', + 'devices\\abc', + '/v1/secret/data/profiles/default' + ])('should reject %s', (path) => { + expect(() => assertSafeSecretPath(path)).toThrow(`invalid secret path: ${path}`) + }) + + // the REST validators accept these characters in profile names, so existing secrets must resolve + it.each([ + 'devices/4bac9510-04a6-4321-bae2-d45ddf07b684', + 'profiles/corp-fleet-ccm', + 'profiles/corp#fleet', + 'profiles/corp?fleet', + 'profiles/100%', + 'profiles/corp%20fleet', + 'certs/acm-domain', + 'sys/health' + ])('should allow %s', (path) => { + expect(() => assertSafeSecretPath(path)).not.toThrow() + }) + + it('should reject a malformed path passed to the secret provider', async () => { + await expect(secretManagerService.getSecretAtPath('devices/../profiles/default')).rejects.toThrow( + 'invalid secret path' + ) + await expect(secretManagerService.deleteSecretAtPath('devices/../certs/acm-domain')).rejects.toThrow( + 'invalid secret path' + ) + }) +}) diff --git a/src/secrets/vault/index.ts b/src/secrets/vault/index.ts index b906bf034..26b3f7090 100644 --- a/src/secrets/vault/index.ts +++ b/src/secrets/vault/index.ts @@ -14,6 +14,23 @@ import { type ILogger } from '../../interfaces/ILogger.js' import { Environment } from '../../utils/Environment.js' import got, { HTTPError, type Got } from 'got' +// reject path traversal before it reaches vault. characters the REST validators already accept +// in profile names (#, ?, %, & ...) stay legal so existing profiles and secrets keep resolving. +export const assertSafeSecretPath = (path: string): void => { + const candidates = [path] + try { + candidates.push(decodeURIComponent(path)) + } catch { + // not valid percent-encoding, so the raw value is all vault will ever see + } + const isUnsafe = candidates.some( + (candidate) => candidate.startsWith('/') || candidate.includes('\\') || candidate.split('/').includes('..') + ) + if (isUnsafe) { + throw new Error(`invalid secret path: ${path}`) + } +} + export class VaultService implements ISecretManagerService { gotClient: Got logger: ILogger @@ -28,6 +45,7 @@ export class VaultService implements ISecretManagerService { } async getSecretFromKey(path: string, key: string): Promise { + assertSafeSecretPath(path) try { this.logger.verbose(`getting secret from vault: ${path}, ${key}`) const rspJson: any = await this.gotClient.get(path).json() @@ -45,6 +63,7 @@ export class VaultService implements ISecretManagerService { async getSecretAtPath( path: string ): Promise { + assertSafeSecretPath(path) try { this.logger.verbose(`getting secrets from ${path}`) const rspJson: any = await this.gotClient.get(path).json() @@ -66,6 +85,7 @@ export class VaultService implements ISecretManagerService { } async writeSecretWithObject(path: string, data: any): Promise { + assertSafeSecretPath(path) try { const json = { data @@ -82,6 +102,7 @@ export class VaultService implements ISecretManagerService { } async deleteSecretAtPath(path: string): Promise { + assertSafeSecretPath(path) try { // to permanently delete the key, we use metadata path const basePath = Environment.Config.secrets_path.replace('/data/', '/metadata/') diff --git a/src/validator.test.ts b/src/validator.test.ts index 90cd0a4f6..b3237333d 100644 --- a/src/validator.test.ts +++ b/src/validator.test.ts @@ -223,6 +223,39 @@ describe('validator', () => { expect(rpsError).toBeInstanceOf(RPSError) expect(rpsError.message).toEqual(`${clientId} - Missing uuid from payload`) }) + + test('Should accept a canonical uuid', async () => { + msg.payload.uuid = '4bac9510-04a6-4321-bae2-d45ddf07b684' + expect(validator.verifyPayload(msg, clientId)).toEqual(msg.payload) + }) + + test.each([ + ['../profiles/default#xxxxxxxxxxxxxxxx', 'slash and hash'], + ['../certs/acm-domain#xxxxxxxxxxxxxxxx', 'slash characters'], + ['..%2fprofiles%2fdefault#xxxxxxxxxxxx', 'percent character'], + ['4bac9510-04a6-4321-bae2-d45ddf07b6#4', 'hash character'], + ['4bac9510-04a6-4321-bae2-d45ddf07b6?4', 'question mark'], + ['4bac9510-04a6-4321-bae2-d45ddf07b6..', 'consecutive dots'] + ])('Should reject uuid %s (%s)', async (uuid) => { + let rpsError: any = null + try { + msg.payload.uuid = uuid + validator.verifyPayload(msg, clientId) + } catch (error) { + rpsError = error + } + expect(rpsError).toBeInstanceOf(RPSError) + expect(rpsError.message).toEqual(`${clientId} - uuid contains invalid characters`) + }) + + test('Should reject a 36-character value that contains invalid characters', async () => { + const value = '../profiles/default#xxxxxxxxxxxxxxxx' + expect(value).toHaveLength(36) + expect(() => { + msg.payload.uuid = value + validator.verifyPayload(msg, clientId) + }).toThrow(`${clientId} - uuid contains invalid characters`) + }) }) describe('validate maintenance message', () => {