feat(security): restrict file attachments to attachments directory

tests/messages.test.js

@@ -1,5 +1,6 @@
 import { describe, it, expect, beforeAll, afterAll, beforeEach } from 'vitest'
 import request from 'supertest'
+import path from 'path'
 import {
   createTestServer,
   createAuthHeader,
@@ -270,6 +271,47 @@ describe('Messages API Tests', () => {
         ''
       )
     })
+
+    it('should permit file sends from paths within the attachments directory', async () => {
+      const filename = path.join(process.cwd(), 'attachments', 'document.pdf')
+      const response = await request(app)
+        .post('/WickrIO/V2/Apps/Messages')
+        .set('Authorization', authHeader)
+        .set('x-api-key', apiKey)
+        .send({
+          users: [{ name: 'user1' }],
+          attachment: {
+            filename,
+            displayname: 'document.pdf',
+          },
+        })
+
+      expect(response.status).toBe(200)
+      expect(mockWickrIOAPI.cmdSend1to1Attachment).toHaveBeenCalledWith(
+        ['user1'],
+        filename,
+        'document.pdf',
+        '',
+        ''
+      )
+    })
+
+    it('should reject file sends from paths outside of attachments directory', async () => {
+      const response = await request(app)
+        .post('/WickrIO/V2/Apps/Messages')
+        .set('Authorization', authHeader)
+        .set('x-api-key', apiKey)
+        .send({
+          users: [{ name: 'user1' }],
+          attachment: {
+            filename: '/tmp/document.pdf',
+            displayname: 'document.pdf',
+          },
+        })
+
+      expect(response.status).toBe(400)
+      expect(mockWickrIOAPI.cmdSend1to1Attachment).not.toHaveBeenCalled()
+    })
   })
 
   describe('GET /Messages - Retrieve Messages', () => {

web_interface.js

@@ -11,6 +11,8 @@
 const morgan = require('morgan')
 
 const bot = new WickrIOBotAPI.WickrIOBot()
+const userAttachmentsDir = path.join(process.cwd(), 'attachments')
+
 let WickrIOAPI = bot.apiService().WickrIOAPI
 
 if (process.env.NODE_ENV === 'test') {
@@ -81,6 +83,7 @@
       }
 
       bot.setAdminOnly(false)
+      fs.mkdirSync(userAttachmentsDir, { recursive: true })
     }
   } catch (err) {
     logger.error(err)
@@ -211,6 +214,15 @@
     const bor = req.body.bor ? req.body.bor.toString() : ''
     const messagemeta = req.body.messagemeta ? JSON.stringify(req.body.messagemeta) : ''
 
+    if (
+      req.body.attachment?.filename &&
+      path.dirname(req.body.attachment.filename) !== userAttachmentsDir
+    ) {
+      console.info('Requested upload for file not in attachments directory, rejecting with 400')
+      // Prevent sending files from outside of the attachments directory
+      return res.status(400).send('Path of file must be within the attachments directory')
+    }
+
     if (req.body.users) {
       // This is a message to be sent in 1:1s to at least 1 user
       const users = req.body.users.map(user => user.name)
@@ -330,14 +342,12 @@
           console.log('attachment is not defined!')
           return res.status(400).send('No attachment included in request')
         } else {
-          const userAttachments = path.join(process.cwd(), 'attachments')
           // multer/busboy should provide only the basename of the file, but call
           // path.basename again to be certain there's no chance of path traversal
           const filename = path.basename(req.file.originalname)
-          const userNewFile = path.join(userAttachments, filename)
-          const inFile = path.join(userAttachments, req.file.filename)
+          const userNewFile = path.join(userAttachmentsDir, filename)
+          const inFile = path.join(userAttachmentsDir, req.file.filename)
 
-          fs.mkdirSync(userAttachments, { recursive: true })
           if (fs.existsSync(userNewFile)) {
             fs.unlinkSync(userNewFile)
           }