From 9ae920951b46ff0163b16c55d744e89acb1036d4 Mon Sep 17 00:00:00 2001
From: Mark Oude Veldhuis <markoudev@users.noreply.github.com>
Date: Fri, 19 Oct 2018 10:41:23 +0200
Subject: [PATCH] Fix possible SQL injection issue (#18)

* Add a validation test case for the SQL injection

Signed-off-by: Mark Oude Veldhuis <mark.oudeveldhuis@nedap.com>

* Raise exception on invalid UUIDs

Signed-off-by: Mark Oude Veldhuis <mark.oudeveldhuis@nedap.com>

* Stay within 80 chars, add some comments.
---
 lib/mysql-binuuid/type.rb                  | 19 +++++++++++++++++--
 test/integration/mysql_integration_test.rb | 18 ++++++++++++++++++
 2 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/lib/mysql-binuuid/type.rb b/lib/mysql-binuuid/type.rb
index eea5442..816aed6 100644
--- a/lib/mysql-binuuid/type.rb
+++ b/lib/mysql-binuuid/type.rb
@@ -1,6 +1,7 @@
 module MySQLBinUUID
-  class Type < ActiveModel::Type::Binary
+  class InvalidUUID < StandardError; end
 
+  class Type < ActiveModel::Type::Binary
     def type
       :uuid
     end
@@ -27,7 +28,16 @@ def cast(value)
     # it to the database.
     def serialize(value)
       return if value.nil?
-      Data.new(strip_dashes(value))
+      undashed_uuid = strip_dashes(value)
+
+      # To avoid SQL injection, verify that it looks like a UUID. ActiveRecord
+      # does not explicity escape the Binary data type. escaping is implicit as
+      # the Binary data type always converts its value to a hex string.
+      unless valid_undashed_uuid?(undashed_uuid)
+        raise MySQLBinUUID::InvalidUUID, "#{value} is not a valid UUID"
+      end
+
+      Data.new(undashed_uuid)
     end
 
     # We're inheriting from the Binary type since ActiveRecord in that case
@@ -73,5 +83,10 @@ def strip_dashes(uuid)
       uuid.delete("-")
     end
 
+    # Verify that the undashed version of a UUID only contains characters that
+    # represent a hexadecimal value.
+    def valid_undashed_uuid?(value)
+      value =~ /\A[[:xdigit:]]{32}\z/
+    end
   end
 end
diff --git a/test/integration/mysql_integration_test.rb b/test/integration/mysql_integration_test.rb
index e45aa8a..284904e 100644
--- a/test/integration/mysql_integration_test.rb
+++ b/test/integration/mysql_integration_test.rb
@@ -112,6 +112,24 @@ class MyUuidModelWithValidations < MyUuidModel
       assert_equal results.first, @my_model
       assert_equal results.first.the_uuid, sample_uuid
     end
+
+    it "can't be used to inject SQL using .where" do
+      assert_raises MySQLBinUUID::InvalidUUID do
+        MyUuidModel.where(the_uuid: "' OR ''='").first
+      end
+    end
+
+    it "can't be used to inject SQL using .find_by" do
+      assert_raises MySQLBinUUID::InvalidUUID do
+        MyUuidModel.find_by(the_uuid: "' OR ''='")
+      end
+    end
+
+    it "can't be used to inject SQL while creating" do
+      assert_raises MySQLBinUUID::InvalidUUID do
+        MyUuidModel.create!(the_uuid: "40' + x'40")
+      end
+    end
   end
 
 end