T09 · Insecure Skill Coding Practices
Error
- Location
- sync.js:52
- Finding
- Shell Command Injection Through Unvalidated Google Drive Folder IDs<![CDATA[ ## Vulnerability Details **File Location**: `sync.js:52-54`; `sync-all.js:12-16` **Vulnerability Type**: OS command injection **Risk Level**: High ### Vulnerable Code `sync.js:52-54`: ```js const query = `'${folderId}' in parents and trashed = false`; const escapedQuery = query.replace(/'/g, "'\\''"); const cmd = `${CERT_PREFIX}npx @googleworkspace/cli drive files list --params '{"q": "${escapedQuery}", "fields": "files(id, name, mimeType, modifiedTime, shortcutDetails)"}'`; try { const res = execSync(cmd, { encoding: 'utf-8', stdio: 'pipe' }); ``` `sync-all.js:12-16`: ```js for (let folder of folders) { console.log(`Starting sync for ${folder.name} (${folder.id})...`); try { execSync(`node sync.js ${folder.id}`, { stdio: 'inherit' }); } catch(e) { ``` ### Technical Analysis Both scripts construct shell command strings by interpolating folder identifiers into arguments passed to `execSync`. The folder identifier originates either from `process.argv[2]` or from the writable `folders.json` registry. The quoting transformation in `sync.js` is intended to escape single quotes inside the Google Drive query, but it does not provide a robust security boundary across JavaScript string construction, JSON encoding, and shell parsing. Other shell metacharacters may still alter how the command is interpreted. `sync-all.js` is more directly vulnerable because `folder.id` is inserted into a shell command without validation or quoting. An attacker-controlled registry value containing shell syntax can cause additional commands to run. The legitimate operation only requires passing a Drive folder ID to another executable. Invoking a shell is unnecessary and exceeds the minimum execution capability required for the declared functionality. ### Attack Path 1. An attacker convinces a user or Agent to register or sync a crafted folder identifier, or modifies `folders.json` through another available write primitive. 2. The crafted value is passed to ...[truncated 870 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions 1. Replace shell-based execution with an API that passes arguments without shell interpretation: ```js const { execFileSync } = require('child_process'); const params = JSON.stringify({ q: `'${folderId}' in parents and trashed = false`, fields: 'files(id, name, mimeType, modifiedTime, shortcutDetails)' }); const res = execFileSync( 'npx', ['@googleworkspace/cli', 'drive', 'files', 'list', '--params', params], { encoding: 'utf8', stdio: 'pipe', env: { ...process.env, ...(fs.existsSync('/workspace/cacert.pem') ? { SSL_CERT_FILE: '/workspace/cacert.pem' } : {}) } } ); ``` 2. Invoke `sync.js` without a shell: ```js execFileSync(process.execPath, ['sync.js', folder.id], { stdio: 'inherit', cwd: __dirname }); ``` 3. Validate every folder ID before storage and before use: ```js function validateFolderId(value) { if (typeof value !== 'string' || !/^[A-Za-z0-9_-]+$/.test(value)) { throw new Error('Invalid Google Drive folder ID'); } return value; } ``` 4. Validate all entries loaded from `folders.json`; do not assume locally stored configuration is trusted. 5. Add regression tests containing spaces, quotes, semicolons, command substitutions, pipes, and newline characters, and verify that none can result in command execution. ]]>