[GSoC] Specs for the SQLi library

[red0xff]

This adds specs for testing the SQL Injection library.

• Shared examples for common, timebased injections
• individual tests for mysqli_common, mysqli_timebased

Didn't include tests for methods that are subject to change (time-based blind run_sql for example, as the current way to retrieve data is specific to my implementation, retrieves the length then the data).
Didn't include tests for methods that are not supported on every DBMS, enumerating databases for example (as these methods should just end up calling call_function, dump_table_fields, or another common method), these could be included in the DBMS-specific tests.

Testing

• Setup the postgresql database
• bundle exec rspec spec/lib/msf/core/exploit/sqli/mysqli/mysqli_common_spec.rb
• It should display 14 examples, 0 failures (possibly more, as I add tests to this branch)
• bundle exec rspec spec/lib/msf/core/exploit/sqli/mysqli/mysqli_time_based_spec.rb
• It should display 1 example, 0 failures (possibly more, as I add tests to this branch)

Feedback

Feedback is much appreciated, I'm pretty new to writing specs.

[adfoster-r7]

I've added some high level discussion points

[adfoster-r7]

If I understand correctly the arguments here are datastore, framework, and userinput - what are your thoughts on introducing let statements to help increase the readability of these tests?

let(:datastore) { instance_double(::Msf::DataStore) }
let(:framework) { instance_double(::Msf::Framework) }
let(:user_output) { File.open(File::NULL, 'w') }
...

common.new(datastore, framework, user_input, user_output) do |_payload|

[red0xff]

framework and user_output are only expected to be used by vprint_XXX functions, which I allowed to return nil, they are never accessed or used in any other code from the library, that's why I considered them as outside the scope of my testing, that's why I didn't want to make them more visible or changeable, please let me know if I should put them in let statements. (for datastore, I will rewrite it in a let statement, as some advanced options are already used in the library).
(the end users will not initialize the class as I am doing, or see these arguments, there is the create_sqli method for that purpose)

[adfoster-r7]

If the next developer changes the implementation details of the sqli class, i.e. renaming @query_proc to something else, or writes a new injection class that doesn't use the same internal datastructures, these shared tests will break. That's generally a sign of brittle tests. It's therefore considered best practice to avoid tricks like instance_variable_get in tests.

I wonder it it would help to introduce an additional let for the proc instead? That way the tests don't need to reach into implementation details

-    let(:sqli_obj) do
-      common_class.new({}, {}, {}) do |payload|
-        payload[/'(.+?)'/, 1] || ''
-      end
-    end

+    let(:query_proc) do
+      proc do |payload|
+        payload[/'(.+?)'/, 1] || ''
+      end
+    end
+   let(:sqli_obj) do
+      common_class.new(datastore, framework, user_output, &query_proc)
+    end

Going a step further, is there a better name for query_proc in the current test's context? Is there perhaps a better name that describes the behaviour to improve the readability of the code? 🤔

[red0xff]

I agree with this, I will add an additional let for the proc.

query_proc is the name of the method argument, it's the proc that queries the server with a given payload, and returns the result, I did add documentation on my implementation of these methods (as comments).

If you have a suggestion for a better method name, would really appreciate it.

[adfoster-r7]

I'm not familiar with the internals of sqli_obj.dump_table_fields- is there any output that could be verified as part of this test?

[red0xff]

dump_table_fields should return an Array of rows (each row being an array of strings), the dump of a table from the database.

It just generates some SQL queries (depending on the user options) and calls run_sql (or truncated_query), which yield the SQL to the user-supplied block (query_proc), which is expected to query the server and return the response, for this reason, it's hard to check its results without having a database set-up (and validating the DBMS-specific SQL also is hard, for this, I'm using some regular expressions that just check for basic things that should be there), but the return value really depends on what the server returned (for select group_concat(table_name) from information_schema.tables where table_schema=database() for example, how would I check what the expected return value would be?)