feat(secrets): validate device identifier format before use in paths

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
   }
 

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'
+    )
+  })
+})

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<string | null> {
+    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<DeviceCredentials | TLSCredentials | WifiCredentials | CiraConfigSecrets | null> {
+    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<any> {
+    assertSafeSecretPath(path)
     try {
       const json = {
         data
@@ -82,6 +102,7 @@ export class VaultService implements ISecretManagerService {
   }
 
   async deleteSecretAtPath(path: string): Promise<boolean> {
+    assertSafeSecretPath(path)
     try {
       // to permanently delete the key, we use metadata path
       const basePath = Environment.Config.secrets_path.replace('/data/', '/metadata/')

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', () => {