Merge pull request rsim#2558 from yahonda/harden-db-tasks-create-sql

Harden CREATE USER / GRANT SQL in DatabaseTasks#create

Author: yahonda

lib/active_record/connection_adapters/oracle_enhanced/database_tasks.rb

@@ -7,24 +7,44 @@ module ActiveRecord
   module ConnectionAdapters
     class OracleEnhancedAdapter
       class DatabaseTasks < ActiveRecord::Tasks::AbstractTasks
+        ORACLE_IDENTIFIER = /\A[[:alpha:]][\w$#]*\z/
+        # GRANT operand: one or more space-separated tokens consisting of
+        # word chars only. Covers system privileges (e.g. "create session",
+        # "CREATE ANY TABLE") and simple role names. Rejects separators such
+        # as `;`, `--`, `'`, `"`, as well as newlines/tabs — anything that
+        # could turn a single GRANT into multiple statements or otherwise
+        # make the operand hard to reason about.
+        ORACLE_PRIVILEGE = /\A\w+( \w+)*\z/
+
         def create
+          username = configuration_hash[:username].to_s
+          unless username.match?(ORACLE_IDENTIFIER)
+            raise ArgumentError, "Invalid Oracle identifier for :username: #{username.inspect}"
+          end
+          OracleEnhancedAdapter.permissions.each do |permission|
+            unless permission.to_s.match?(ORACLE_PRIVILEGE)
+              raise ArgumentError, "Invalid Oracle privilege in OracleEnhancedAdapter.permissions: #{permission.inspect}"
+            end
+          end
+          quoted_password = %("#{configuration_hash[:password].to_s.gsub('"', '""')}")
+
           system_password = ENV.fetch("ORACLE_SYSTEM_PASSWORD") {
             print "Please provide the SYSTEM password for your Oracle installation (set ORACLE_SYSTEM_PASSWORD to avoid this prompt)\n>"
             $stdin.gets.strip
           }
           establish_connection(configuration_hash.merge(username: "SYSTEM", password: system_password))
           begin
-            connection.execute "CREATE USER #{configuration_hash[:username]} IDENTIFIED BY #{configuration_hash[:password]}"
+            connection.execute "CREATE USER #{username} IDENTIFIED BY #{quoted_password}"
           rescue => e
             if /ORA-01920/.match?(e.message) # user name conflicts with another user or role name
-              connection.execute "ALTER USER #{configuration_hash[:username]} IDENTIFIED BY #{configuration_hash[:password]}"
+              connection.execute "ALTER USER #{username} IDENTIFIED BY #{quoted_password}"
             else
               raise e
             end
           end
 
           OracleEnhancedAdapter.permissions.each do |permission|
-            connection.execute "GRANT #{permission} TO #{configuration_hash[:username]}"
+            connection.execute "GRANT #{permission} TO #{username}"
           end
         end
 

spec/active_record/connection_adapters/oracle_enhanced/database_tasks_spec.rb

@@ -1,6 +1,7 @@
 # frozen_string_literal: true
 
 require "active_record/connection_adapters/oracle_enhanced/database_tasks"
+require "active_support/testing/stream"
 require "stringio"
 require "tempfile"
 
@@ -65,6 +66,75 @@ def fake_terminal(input)
     end
   end
 
+  describe "create input validation" do
+    around do |example|
+      original_stderr, $stderr = $stderr, StringIO.new
+      example.run
+    ensure
+      $stderr = original_stderr
+    end
+
+    it "raises ArgumentError before touching the database when :username is not an Oracle identifier" do
+      invalid_config = config.merge(username: "oracle;DROP USER system;--")
+      expect(ActiveRecord::Base).not_to receive(:establish_connection)
+      expect {
+        ActiveRecord::Tasks::DatabaseTasks.create(invalid_config)
+      }.to raise_error(ArgumentError, /Invalid Oracle identifier for :username/)
+    end
+
+    it "raises ArgumentError before touching the database when OracleEnhancedAdapter.permissions contains a statement separator" do
+      original_permissions = ActiveRecord::ConnectionAdapters::OracleEnhancedAdapter.permissions
+      ActiveRecord::ConnectionAdapters::OracleEnhancedAdapter.permissions =
+        ["create session; DROP USER system;--"]
+      expect(ActiveRecord::Base).not_to receive(:establish_connection)
+      expect {
+        ActiveRecord::Tasks::DatabaseTasks.create(config.merge(username: "oracle_enhanced_test_user"))
+      }.to raise_error(ArgumentError, /Invalid Oracle privilege in OracleEnhancedAdapter.permissions/)
+    ensure
+      ActiveRecord::ConnectionAdapters::OracleEnhancedAdapter.permissions = original_permissions
+    end
+
+    it "raises ArgumentError before touching the database when OracleEnhancedAdapter.permissions contains a newline" do
+      original_permissions = ActiveRecord::ConnectionAdapters::OracleEnhancedAdapter.permissions
+      ActiveRecord::ConnectionAdapters::OracleEnhancedAdapter.permissions = ["create\nsession"]
+      expect(ActiveRecord::Base).not_to receive(:establish_connection)
+      expect {
+        ActiveRecord::Tasks::DatabaseTasks.create(config.merge(username: "oracle_enhanced_test_user"))
+      }.to raise_error(ArgumentError, /Invalid Oracle privilege in OracleEnhancedAdapter.permissions/)
+    ensure
+      ActiveRecord::ConnectionAdapters::OracleEnhancedAdapter.permissions = original_permissions
+    end
+  end
+
+  describe "create password quoting" do
+    include ActiveSupport::Testing::Stream
+
+    let(:captured_sqls) { [] }
+    let(:fake_connection) do
+      double("connection").tap do |c|
+        allow(c).to receive(:execute) { |sql| captured_sqls << sql }
+      end
+    end
+
+    before do
+      allow(ActiveRecord::Base).to receive(:establish_connection)
+      allow(ActiveRecord::Base).to receive(:lease_connection).and_return(fake_connection)
+      ENV["ORACLE_SYSTEM_PASSWORD"] = "dummy"
+    end
+
+    after { ENV.delete("ORACLE_SYSTEM_PASSWORD") }
+
+    it "wraps :password as a double-quoted literal with embedded double quotes doubled" do
+      quietly do
+        ActiveRecord::Tasks::DatabaseTasks.create(
+          config.merge(username: "oracle_enhanced_test_user", password: %{p"w})
+        )
+      end
+      create_sql = captured_sqls.first
+      expect(create_sql).to eq(%{CREATE USER oracle_enhanced_test_user IDENTIFIED BY "p""w"})
+    end
+  end
+
   context "with test table" do
     before(:all) do
       $stdout, @original_stdout = StringIO.new, $stdout