Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Msf::Exploit::Remote::HTTP::Wordpress::SQLi #19497

Open
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

Chocapikk
Copy link
Contributor

@Chocapikk Chocapikk commented Sep 24, 2024

Hello Metasploit Team,

This pull request introduces a new mixin, Msf::Exploit::Remote::HTTP::Wordpress::SQLi, which provides reusable SQL Injection (SQLi) helper functions specifically designed for WordPress modules in the Metasploit Framework. This idea was proposed by @jvoisin to simplify the development of exploit and auxiliary modules targeting WordPress vulnerabilities by avoiding code duplication and facilitating maintenance.

Key Features:

  • Dynamic Table Prefix Detection: The mixin includes a function wordpress_sqli_identify_table_prefix that dynamically identifies the WordPress database table prefix, whether it's the default wp_ or a custom prefix. This ensures compatibility with WordPress installations using custom table prefixes.

  • Prefixed Function Names: All functions in the mixin are prefixed with wordpress_sqli_ to clearly indicate their association with the WordPress SQLi mixin and to prevent naming conflicts.

  • Helper Functions:

    • wordpress_sqli_initialize(sqli): Initializes the SQLi instance in the module, making it accessible within the mixin for executing SQL injection queries.

    • wordpress_sqli_create_user(username, password, email): Creates a new WordPress user or updates an existing one in the users table. The function handles password hashing using MD5 for compatibility with older WordPress versions.

    • wordpress_sqli_grant_admin_privileges(username): Grants administrator privileges to the specified user by creating or updating the appropriate entry in the usermeta table.

    • wordpress_sqli_get_users_credentials(count = 10): Retrieves user login names and password hashes from the users table, using the detected table prefix. All functions are dynamic thanks to the prefix detection, allowing them to handle installations with custom prefixes seamlessly.

Adapting Existing Modules:

I plan to adapt the modules (both exploit and auxiliary) I've created in the following pull requests to use this new mixin:

Integrating this mixin into these modules will prevent code duplication, facilitate maintenance, and improve consistency across modules exploiting WordPress vulnerabilities.

Thank you for your time, and please let me know if you have any questions or need further adjustments.

Comment on lines 12 to 23
# Function to initialize the SQLi instance in the mixin.
#
# This function sets up the SQLi instance that is initialized in the exploit module.
# The SQLi instance is passed as a parameter to ensure it is accessible within the mixin
# and can be used for executing SQL injection queries.
#
# @param sqli [Object] The SQLi instance initialized in the exploit module.
# @return [void]

def wordpress_sqli_initialize(sqli)
@sqli = sqli
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not super-duper convinced that it's better to have a sqli variable here, instead of passing it systematically to all functions. Why did you pick this design over a stateless one? Especially since the table_prefix is passed as argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've decided to initialise @sqli in this way so that we don't need to use it as an argument when using each function, but yes, I can also initialise the prefix directly in initialize, which will make it easier to use the functions without having to pass table_prefix in each function.

lib/msf/core/exploit/remote/http/wordpress/sqli.rb Outdated Show resolved Hide resolved
lib/msf/core/exploit/remote/http/wordpress/sqli.rb Outdated Show resolved Hide resolved
SELECT LEFT(table_name, LENGTH(table_name) - LENGTH('users')) AS prefix
FROM information_schema.tables
WHERE table_schema = database()
AND table_name LIKE '%\\_users'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe use a more WordPress specific table name than _users, as I'm sure various plugins might have something like my_plugin_users or whatnot.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll have a look, but in fact here I'm checking that the table contains the user_login and user_pass columns, so we're supposed to get the right table no matter what other tables other plugins may add.

lib/msf/core/exploit/remote/http/wordpress/sqli.rb Outdated Show resolved Hide resolved
lib/msf/core/exploit/remote/http/wordpress/sqli.rb Outdated Show resolved Hide resolved
meta_value = 'a:1:{s:13:"administrator";s:1:"1";}'
SQL

@sqli.raw_run_sql(admin_query.strip.gsub(/\s+/, ' '))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does there need to be a check here? From a quick glance at the raw_run_sql method I think we may want something here maybe.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @cgranleese-r7, I considered adding a check, but my concern is it could slow down the process significantly, especially since we're working with blind SQL injection (time-based or boolean). We're already querying to find the table prefix, and adding another check for admin privileges would likely delay the execution even more. Plus, if the SQL query fails, I think there's nothing more we can do anyway.

That said, if the check is necessary, I can implement it. What do you think?

Copy link
Contributor

@cgranleese-r7 cgranleese-r7 Oct 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thoughts where just that when I was looking at the code I was wondering if there could be a scenario were:

@sqli.raw_run_sql(admin_query.strip.gsub(/\s+/, ' ')) 

fails for whatever reason and then prompts the user with:

print_status("{WPSQLi} Admin privileges granted or updated for user '#{username}'.")

I just thought that could be confusing from a user perspective is all and was worth calling out.

@dledda-r7 dledda-r7 self-assigned this Oct 7, 2024
Copy link
Contributor

@dledda-r7 dledda-r7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @Chocapikk,
I'm leaving here couple of suggestions to make the mixin less verbose by default.

lib/msf/core/exploit/remote/http/wordpress/sqli.rb Outdated Show resolved Hide resolved
lib/msf/core/exploit/remote/http/wordpress/sqli.rb Outdated Show resolved Hide resolved
lib/msf/core/exploit/remote/http/wordpress/sqli.rb Outdated Show resolved Hide resolved
lib/msf/core/exploit/remote/http/wordpress/sqli.rb Outdated Show resolved Hide resolved
lib/msf/core/exploit/remote/http/wordpress/sqli.rb Outdated Show resolved Hide resolved
lib/msf/core/exploit/remote/http/wordpress/sqli.rb Outdated Show resolved Hide resolved
lib/msf/core/exploit/remote/http/wordpress/sqli.rb Outdated Show resolved Hide resolved
lib/msf/core/exploit/remote/http/wordpress/sqli.rb Outdated Show resolved Hide resolved
lib/msf/core/exploit/remote/http/wordpress/sqli.rb Outdated Show resolved Hide resolved
lib/msf/core/exploit/remote/http/wordpress/sqli.rb Outdated Show resolved Hide resolved
@dledda-r7
Copy link
Contributor

I think you missed this #19497 (comment)

@Chocapikk
Copy link
Contributor Author

I think you missed this #19497 (comment)

Oh yes thank you I actually forgot

@dledda-r7
Copy link
Contributor

I think you missed this #19497 (comment)

Oh yes thank you I actually forgot

Nice, thank you, I saw the CI didn't passed but doesn't look like a module issue, I'll re-run it if there are further errors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

4 participants