Merge pull request #20 from secroots/security/attachments-in-other-pr…

…ojects

Fix attachments showing for unrelated projects

CHANGELOG

@@ -22,9 +22,7 @@
     - [API entity]:
       - [future tense verb] [API enhancement]
   - Security Fixes:
-    - High: (Authenticated|Unauthenticated) (admin|author|contributor) [vulnerability description]
-    - Medium: (Authenticated|Unauthenticated) (admin|author|contributor) [vulnerability description]
-    - Low: (Authenticated|Unauthenticated) (admin|author|contributor) [vulnerability description]
+    - Medium: Authenticated (author) horizontal privilege escalation affecting attachments
 
 v4.12.0 (May 2024)
   - Attachments: Add size, created_at, and download link to the API

Gemfile

@@ -86,7 +86,7 @@ gem 'bcrypt', '3.1.12'
 gem 'json', '2.3.0'
 
 # XML manipulation
-gem 'nokogiri', '>= 1.16.2'
+gem 'nokogiri', '>= 1.16.5'
 
 # MySQL backend
 # gem 'mysql2', '~> 0.5.1'

Gemfile.lock

@@ -279,7 +279,7 @@ GEM
     matrix (0.4.2)
     method_source (0.9.2)
     mini_mime (1.1.5)
-    mini_portile2 (2.8.5)
+    mini_portile2 (2.8.6)
     mini_racer (0.6.2)
       libv8-node (~> 16.10.0.0)
     minitest (5.22.2)
@@ -299,14 +299,14 @@ GEM
     net-smtp (0.4.0.1)
       net-protocol
     nio4r (2.7.0)
-    nokogiri (1.16.2)
+    nokogiri (1.16.5)
       mini_portile2 (~> 2.8.2)
       racc (~> 1.4)
-    nokogiri (1.16.2-arm64-darwin)
+    nokogiri (1.16.5-arm64-darwin)
       racc (~> 1.4)
-    nokogiri (1.16.2-x86_64-darwin)
+    nokogiri (1.16.5-x86_64-darwin)
       racc (~> 1.4)
-    nokogiri (1.16.2-x86_64-linux)
+    nokogiri (1.16.5-x86_64-linux)
       racc (~> 1.4)
     notiffany (0.1.3)
       nenv (~> 0.1)
@@ -327,7 +327,7 @@ GEM
     public_suffix (5.0.3)
     puma (6.4.2)
       nio4r (~> 2.0)
-    racc (1.7.3)
+    racc (1.8.0)
     rack (2.2.8.1)
     rack-mini-profiler (2.3.0)
       rack (>= 1.2.0)
@@ -389,7 +389,8 @@ GEM
       vegas (~> 0.1.2)
     resque-status (0.5.0)
       resque (~> 1.19)
-    rexml (3.2.5)
+    rexml (3.2.8)
+      strscan (>= 3.0.9)
     rinku (2.0.6)
     rprogram (0.3.2)
     rspec (3.10.0)
@@ -471,6 +472,7 @@ GEM
       activesupport (>= 5.2)
       sprockets (>= 3.0.0)
     sqlite3 (1.4.2)
+    strscan (3.1.0)
     terser (1.1.15)
       execjs (>= 0.3.0, < 3)
     thor (1.2.2)
@@ -573,7 +575,7 @@ DEPENDENCIES
   net-imap
   net-pop
   net-smtp
-  nokogiri (>= 1.16.2)
+  nokogiri (>= 1.16.5)
   paper_trail (~> 12.2.0)
   parslet (~> 1.6.0)
   pg

app/controllers/attachments_controller.rb

@@ -93,7 +93,7 @@ def destroy
   # give :node_id is valid.
   def find_or_initialize_node
     begin
-      @node = Node.find(params[:node_id])
+      @node = current_project.nodes.find(params[:node_id])
     rescue
       redirect_to root_path, alert: 'Node not found'
     end

spec/features/attachments_spec.rb

@@ -1,15 +1,15 @@
 require 'rails_helper'
 
-describe "Describe attachments" do
-  it "should require authenticated users" do
+describe 'Describe attachments' do
+  it 'should require authenticated users' do
     node = create(:node)
     visit project_node_attachments_path(node.project, node)
 
     expect(current_path).to eq(login_path)
     expect(page).to have_content('Access denied.')
   end
 
-  describe "as authenticated user" do
+  describe 'as authenticated user' do
     before do
       login_to_project_as_user
       @node = create(:node, project: current_project)
@@ -19,7 +19,7 @@
       FileUtils.rm_rf(Attachment.pwd.join(@node.id.to_s))
     end
 
-    it "stores the file on disk" do
+    it 'stores the file on disk' do
       visit project_node_path(current_project, @node)
 
       file_path = Rails.root.join('spec/fixtures/files/rails.png')
@@ -30,12 +30,12 @@
       expect(File.exist?(Attachment.pwd.join(@node.id.to_s, 'rails.png'))).to be true
     end
 
-    it "auto-renames the upload if an attachment with the same name already exists" do
+    it 'auto-renames the upload if an attachment with the same name already exists' do
       node_attachments = Attachment.pwd.join(@node.id.to_s)
       FileUtils.rm_rf(node_attachments)
       FileUtils.mkdir_p(node_attachments)
 
-      FileUtils.cp( Rails.root.join('spec/fixtures/files/rails.png'), node_attachments.join('rails.png') )
+      FileUtils.cp(Rails.root.join('spec/fixtures/files/rails.png'), node_attachments.join('rails.png'))
       expect(Dir["#{node_attachments}/*"].count).to eq(1)
 
       visit project_node_path(current_project, @node)
@@ -48,7 +48,7 @@
     end
 
     it 'builds a URL encoded link for attachments' do
-      FileUtils.mkdir_p( Attachment.pwd.join(@node.id.to_s) )
+      FileUtils.mkdir_p(Attachment.pwd.join(@node.id.to_s))
 
       filenames = ['attachment with space.png', 'attachmentwith&.png', 'attachmentwith+.png']
 
@@ -64,6 +64,26 @@
         expect(page).to have_css("button[data-clipboard-text='!/projects/#{current_project.id}/nodes/#{@node.id}/attachments/#{url_encoded_filename}!']")
       end
     end
+
+    describe 'viewing the attachment' do
+      before do
+        visit project_node_path(current_project, @node)
+
+        file_path = Rails.root.join('spec/fixtures/files/rails.png')
+        attach_file('files[]', file_path)
+        click_button 'Start'
+
+        expect(page).to have_content('rails.png')
+      end
+
+      it 'does not render the attachment in the wrong project', skip: !defined?(Dradis::Pro) do
+        new_project = create(:project)
+        new_project.permissions << Permission.new(component: Dradis::Pro.permission_component_name, user: @logged_in_as, name: 'read-update')
+        new_project.save!
+
+        visit project_node_attachment_path(new_project, @node, 'rails.png')
+        expect(page).to have_text('Node not found')
+      end
+    end
   end
 end
-#