CVE-2018-7688: Missing Authorization (CWE-862)
value: A missing permission check in the review handling of openSUSE Open Build Service before 2.9.3 allowed all authenticated users to modify sources in projects where they do not have write permissions.
CWE description: The product does not perform an authorization check when an actor attempts to access a resource or perform an action.
CVSS v3 base score: 6.5
Published: 2018-06-07T13:29Z
Repository: https://github.com/openSUSE/open-build-service
Language: Ruby
Files changed: src/api/app/models/review.rb, src/api/spec/models/review_spec.rb, src/api/test/unit/bs_request_test.rb
Commit date: 2018-05-30 11:26:15 +0200
Commit message: [api] Initialize review state with :new

to have the request review always in a sane state.

https://bugzilla.suse.com/show_bug.cgi?id=1094820

Signed-off-by: Christian Bruckmayer <cbruckmayer@suse.com>
Co-authored-by: Christian Bruckmayer <cbruckmayer@suse.com>

Diff: diff --git a/src/api/app/models/review.rb b/src/api/app/models/review.rb
index ebef34e51c..dcfcb158e5 100644
--- a/src/api/app/models/review.rb
+++ b/src/api/app/models/review.rb
@@ -131,12 +131,12 @@ class Review < ApplicationRecord
   end
 
   def self.new_from_xml_hash(hash)
     r = Review.new
 
-    r.state = hash.delete('state') { raise ArgumentError, 'no state' }
-    r.state = r.state.to_sym
+    r.state = :new
+    hash.delete('state')
 
     r.by_user = hash.delete('by_user')
     r.by_group = hash.delete('by_group')
     r.by_project = hash.delete('by_project')
     r.by_package = hash.delete('by_package')
diff --git a/src/api/spec/models/review_spec.rb b/src/api/spec/models/review_spec.rb
index aca2b19f3c..838c6f9d38 100644
--- a/src/api/spec/models/review_spec.rb
+++ b/src/api/spec/models/review_spec.rb
@@ -425,6 +425,22 @@ RSpec.describe Review do
       expect(review_by_group.reviewable_by?(by_group:     other_group.title)).to be_falsy
       expect(review_by_project.reviewable_by?(by_project: other_project.name)).to be_falsy
       expect(review_by_package.reviewable_by?(by_package: other_package.name)).to be_falsy
     end
   end
+
+  describe '.new_from_xml_hash' do
+    let(:request_xml) do
+      "<request>
+        <review state='accepted' by_user='#{user}'/>
+      </request>"
+    end
+    let(:request_hash) { Xmlhash.parse(request_xml) }
+    let(:review_hash) { request_hash['review'] }
+
+    subject { Review.new_from_xml_hash(review_hash) }
+
+    it 'initalizes the review in state :new' do
+      expect(subject.state).to eq(:new)
+    end
+  end
 end
diff --git a/src/api/test/unit/bs_request_test.rb b/src/api/test/unit/bs_request_test.rb
index 14e8521be6..f90654c335 100644
--- a/src/api/test/unit/bs_request_test.rb
+++ b/src/api/test/unit/bs_request_test.rb
@@ -123,11 +123,36 @@ class BsRequestTest < ActiveSupport::TestCase
     req.save!
     # number got increased by one
     assert_equal 1027, req.number
 
     newxml = req.render_xml
-    assert_equal xml, newxml
+    expected = <<-XML.strip_heredoc
+      <request id="1027" creator="Iggy">
+        <action type="submit">
+          <source project="home:Iggy" package="TestPack" rev="1"/>
+          <target project="kde4" package="mypackage"/>
+          <options>
+            <sourceupdate>cleanup</sourceupdate>
+          </options>
+          <acceptinfo rev="1" srcmd5="806a6e27ed7915d1bb8d8a989404fd5a" osrcmd5="d41d8cd98f00b204e9800998ecf8427e"/>
+        </action>
+        <priority>critical</priority>
+        <state name="review" who="Iggy" when="2012-11-07T21:13:12">
+          <comment>No comment</comment>
+        </state>
+        <review state="new" when="2017-09-01T09:11:11" by_user="adrian"/>
+        <review state="new" when="2017-09-01T09:11:11" by_group="test_group"/>
+        <review state="new" when="2012-11-07T21:13:12" who="tom" by_user="tom">
+          <comment>review1</comment>
+        </review>
+        <review state="new" when="2012-11-07T21:13:13" who="tom" by_user="tom">
+          <comment>please accept</comment>
+        </review>
+        <description>Left blank</description>
+      </request>
+    XML
+    assert_equal expected, newxml
 
     wi = req.webui_infos(diffs: false)
     # iggy is *not* target maintainer
     assert_equal false, wi['is_target_maintainer']
     assert_equal wi['actions'][0], type: :submit,

Vulnerable code:     r.state = hash.delete('state') { raise ArgumentError, 'no state' }
    r.state = r.state.to_sym
    assert_equal xml, newxml
Fixed code:     r.state = :new
    hash.delete('state')

  describe '.new_from_xml_hash' do
    let(:request_xml) do
      "<request>
        <review state='accepted' by_user='#{user}'/>
      </request>"
    end
    let(:request_hash) { Xmlhash.parse(request_xml) }
    let(:review_hash) { request_hash['review'] }

    subject { Review.new_from_xml_hash(review_hash) }

    it 'initalizes the review in state :new' do
      expect(subject.state).to eq(:new)
    end
  end
    expected = <<-XML.strip_heredoc
      <request id="1027" creator="Iggy">
        <action type="submit">
          <source project="home:Iggy" package="TestPack" rev="1"/>
          <target project="kde4" package="mypackage"/>
          <options>
            <sourceupdate>cleanup</sourceupdate>
          </options>
          <acceptinfo rev="1" srcmd5="806a6e27ed7915d1bb8d8a989404fd5a" osrcmd5="d41d8cd98f00b204e9800998ecf8427e"/>
        </action>
        <priority>critical</priority>
        <state name="review" who="Iggy" when="2012-11-07T21:13:12">
          <comment>No comment</comment>
        </state>
        <review state="new" when="2017-09-01T09:11:11" by_user="adrian"/>
        <review state="new" when="2017-09-01T09:11:11" by_group="test_group"/>
        <review state="new" when="2012-11-07T21:13:12" who="tom" by_user="tom">
          <comment>review1</comment>
        </review>
        <review state="new" when="2012-11-07T21:13:13" who="tom" by_user="tom">
          <comment>please accept</comment>
        </review>
        <description>Left blank</description>
      </request>
    XML
    assert_equal expected, newxml