Security concern: SQL injection vulnerability

[skg574]

I've discovered a security vulnerability in the Fetchmail plugin related to SQL injection that could allow an attacker to execute arbitrary SQL commands against the database.

I would like to share the details with you privately following responsible disclosure practices.

Could you please provide a preferred method for secure communication so I can share the full vulnerability report and proposed fix?

I'm happy to communicate via email or any other secure channel you prefer.

/steve
security@codamail.com

[PF4Public]

#1 :D

[skg574]

My find is with your changes and the latest code. One example: if ($id != 0 || $id != '') always returns true, should be if ($id != 0 && $id != '') also multiple instances of direct string concatenation in queries.

[PF4Public]

multiple instances of direct string concatenation in queries

#23 Have you looked through any of the open bugs before filling this one? Oh, well, whatever.

If you have improvements just submit PR.

[skg574]

Well, if you are convinced it is secure, so be it. It is patched for us, anyway.

[PF4Public]

if you are convinced it is secure

I have no idea how you came to this conclusion, I never stated anything like that, but, as you say, so be it.

I cannot force you to submit your improvements if you're not willing to.

[skg574]

I got the impression (probably falsely) that you were pointing me to your 2017 input validation as that having resolved all issues. I am just trying to report in a private manner (as we did when we discovered the ajax leak vulnerabilities in the twofactor_gauthenticator plugin) so as to not give away the vulnerability publicly before evaluation and possible patch, but if you'd like, I can post the report here. If you would rather a secure manner via email, that can be done too.

[PF4Public]

I got the impression (probably falsely) that you were pointing me to your 2017 input validation as that having resolved all issues.

Why resolved? Not at all, they're both open and not fixed. If my understanding is correct, the vulnerability you're referring to could be only exploited by someone already authenticated, which in turn makes it a serious vulnerability only in a large-scale installations with untrustworthy users. I pretty much doubt this plugin is used that extensively, that's why this was neglected for so long. The description of the vulnerability itself is not that helping (as the gist of it must be obvious, right?), but the solution is something valuable. For which you can submit a PR if you wish.

as we did when we discovered the ajax leak vulnerabilities

That is very noble of you — that's for sure.

[skg574]

There are actually multiple vulnerabilities. There aren't explicit authentication checks for the actions, so in some situations these could be triggered without being authenticated. The sql injection is due direct string concatenation instead of using parameterized queries. Where would you like me to post this report for your review? Right here?

[PF4Public]

Where would you like me to post this report for your review?

The description of the vulnerability itself is not that helping (as the gist of it must be obvious, right?), but the solution is something valuable. For which you can submit a PR if you wish.

EDIT: if you want to start with the discussion first, GitHub has a feature for that: https://github.com/PF4Public/fetchmail/security/advisories

[skg574]

Vulnerability Report: SQL Injection in Fetchmail Plugin for Roundcube

Overview

A security audit of the Fetchmail plugin for Roundcube webmail has uncovered multiple SQL injection vulnerabilities. These vulnerabilities could allow an authenticated attacker to execute arbitrary SQL commands against the database, potentially leading to unauthorized data access, data modification, or in some cases, server compromise.

Vulnerability Details

Vulnerability Type

SQL Injection (CWE-89)

Affected Component

Roundcube Fetchmail Plugin (fetchmail.php)

Risk Rating

High

Description

The original Fetchmail plugin code directly embedded user input into SQL queries using string concatenation rather than using parameterized queries. This practice is vulnerable to SQL injection attacks where an attacker could manipulate input to modify the intended SQL query structure.

Specific Vulnerabilities Found

1. SQL Injection in Delete Function

In the original code:

if ($id != 0 || $id != '')
{
    $sql = "DELETE FROM fetchmail WHERE id = '$id'";
    $delete = $this->db->query($sql);
    // ...
}

The logical condition ($id != 0 || $id != '') is always true for any value of $id, making the check essentially meaningless. The user-supplied $id value is directly concatenated into the SQL query without sanitization, allowing SQL injection.

2. SQL Injection in Save Function

Multiple instances of direct string concatenation in queries throughout the save function:

For INSERT operations:

if($mda != '')
{
    $sql = "INSERT INTO fetchmail (mailbox, domain, active, src_server, src_user, src_password, src_folder, poll_time, fetchall, keep, protocol, usessl, sslcertck, src_auth, mda) VALUES ('$mailbox', '$mailbox_domain', '$enabled', '$server', '$user', '$pass', '$folder', '$pollinterval', '$fetchall', '$keep', '$protocol', '$usessl', true, 'password', '$mda' )";
} else {
    $sql = "INSERT INTO fetchmail (mailbox, domain, active, src_server, src_user, src_password, src_folder, poll_time, fetchall, keep, protocol, usessl, sslcertck, src_auth) VALUES ('$mailbox', '$mailbox_domain', '$enabled', '$server', '$user', '$pass', '$folder', '$pollinterval', '$fetchall', '$keep', '$protocol', '$usessl', true, 'password')";
}
$insert = $this->db->query($sql);

For UPDATE operations:

if($mda != '')
{
    $sql = "UPDATE fetchmail SET mailbox = '$mailbox', domain = '$mailbox_domain', active = '$enabled', keep = '$keep', protocol = '$protocol', src_server = '$server', src_user = '$user', src_password = '$pass', src_folder = '$folder', poll_time = '$pollinterval', fetchall = '$fetchall', usessl = '$usessl', src_auth = 'password', mda = '$mda' WHERE id = '$id'";
} else {
    $sql = "UPDATE fetchmail SET mailbox = '$mailbox', domain = '$mailbox_domain', active = '$enabled', keep = '$keep', protocol = '$protocol', src_server = '$server', src_user = '$user', src_password = '$pass', src_folder = '$folder', poll_time = '$pollinterval', fetchall = '$fetchall', usessl = '$usessl', src_auth = 'password' WHERE id = '$id'";
}
$update = $this->db->query($sql);

For SELECT operations:

$sql = "SELECT * FROM fetchmail WHERE mailbox='" . $mailbox . "'";
$result = $this->db->query($sql);

All of these queries have user input directly embedded into SQL strings, creating critical SQL injection vulnerabilities.

3. Missing Authentication Checks

No explicit authentication verification at the beginning of action handlers, potentially allowing attackers to bypass authentication under certain conditions.

4. Inadequate Error Handling

Insufficient error handling for database operations and input validation.

Implemented Fixes

The following changes were made to address these vulnerabilities:

1. Added Authentication Verification

Added explicit authentication checks at the beginning of each action handler:

// Check if user is authenticated
$this->check_auth();

private function check_auth()
{
    if (!$this->rc->user->ID) {
        header('Location: ?_task=login');
        exit;
    }
}

2. Parameterized Queries

Replaced all direct string concatenation with parameterized queries:

Original Delete Function (vulnerable):

$sql = "DELETE FROM fetchmail WHERE id = '$id'";
$delete = $this->db->query($sql);

Fixed Delete Function:

$sql = "DELETE FROM fetchmail WHERE id = ? AND mailbox = ?";
$delete = $this->db->query($sql, $id, $mailbox);

Original Insert Function (vulnerable):

if($mda != '')
{
    $sql = "INSERT INTO fetchmail (mailbox, domain, active, src_server, src_user, src_password, src_folder, poll_time, fetchall, keep, protocol, usessl, sslcertck, src_auth, mda) VALUES ('$mailbox', '$mailbox_domain', '$enabled', '$server', '$user', '$pass', '$folder', '$pollinterval', '$fetchall', '$keep', '$protocol', '$usessl', true, 'password', '$mda' )";
} else {
    $sql = "INSERT INTO fetchmail (mailbox, domain, active, src_server, src_user, src_password, src_folder, poll_time, fetchall, keep, protocol, usessl, sslcertck, src_auth) VALUES ('$mailbox', '$mailbox_domain', '$enabled', '$server', '$user', '$pass', '$folder', '$pollinterval', '$fetchall', '$keep', '$protocol', '$usessl', true, 'password')";
}
$insert = $this->db->query($sql);

Fixed Insert Function:

if($mda != '')
{
    $sql = "INSERT INTO fetchmail (mailbox, domain, active, src_server, src_user, src_password, src_folder, poll_time, fetchall, keep, protocol, usessl, sslcertck, src_auth, mda) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)";
    $insert = $this->db->query($sql, $mailbox, $mailbox_domain, $enabled, $server, $user, $pass, $folder, $pollinterval, $fetchall, $keep, $protocol, $usessl, true, 'password', $mda);
} 
else 
{
    $sql = "INSERT INTO fetchmail (mailbox, domain, active, src_server, src_user, src_password, src_folder, poll_time, fetchall, keep, protocol, usessl, sslcertck, src_auth) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)";
    $insert = $this->db->query($sql, $mailbox, $mailbox_domain, $enabled, $server, $user, $pass, $folder, $pollinterval, $fetchall, $keep, $protocol, $usessl, true, 'password');
}

Original Update Function (vulnerable):

if($mda != '')
{
    $sql = "UPDATE fetchmail SET mailbox = '$mailbox', domain = '$mailbox_domain', active = '$enabled', keep = '$keep', protocol = '$protocol', src_server = '$server', src_user = '$user', src_password = '$pass', src_folder = '$folder', poll_time = '$pollinterval', fetchall = '$fetchall', usessl = '$usessl', src_auth = 'password', mda = '$mda' WHERE id = '$id'";
} else {
    $sql = "UPDATE fetchmail SET mailbox = '$mailbox', domain = '$mailbox_domain', active = '$enabled', keep = '$keep', protocol = '$protocol', src_server = '$server', src_user = '$user', src_password = '$pass', src_folder = '$folder', poll_time = '$pollinterval', fetchall = '$fetchall', usessl = '$usessl', src_auth = 'password' WHERE id = '$id'";
}
$update = $this->db->query($sql);

Fixed Update Function:

if($mda != '')
{
    $sql = "UPDATE fetchmail SET mailbox = ?, domain = ?, active = ?, keep = ?, protocol = ?, src_server = ?, src_user = ?, src_password = ?, src_folder = ?, poll_time = ?, fetchall = ?, usessl = ?, src_auth = ?, mda = ? WHERE id = ? AND mailbox = ?";
    $update = $this->db->query($sql, $mailbox, $mailbox_domain, $enabled, $keep, $protocol, $server, $user, $pass, $folder, $pollinterval, $fetchall, $usessl, 'password', $mda, $id, $mailbox);
} 
else 
{
    $sql = "UPDATE fetchmail SET mailbox = ?, domain = ?, active = ?, keep = ?, protocol = ?, src_server = ?, src_user = ?, src_password = ?, src_folder = ?, poll_time = ?, fetchall = ?, usessl = ?, src_auth = ? WHERE id = ? AND mailbox = ?";
    $update = $this->db->query($sql, $mailbox, $mailbox_domain, $enabled, $keep, $protocol, $server, $user, $pass, $folder, $pollinterval, $fetchall, $usessl, 'password', $id, $mailbox);
}

Original Select Function (vulnerable):

$sql = "SELECT * FROM fetchmail WHERE mailbox='" . $mailbox . "'";
$result = $this->db->query($sql);

Fixed Select Function:

$sql = "SELECT * FROM fetchmail WHERE mailbox = ?";
$result = $this->db->query($sql, $mailbox);

3. Fixed Logic Error

Changed the conditional check from OR to AND to ensure proper validation:

Original (vulnerable):

if ($id != 0 || $id != '')

Fixed:

if ($id != 0 && $id != '')

4. Null Value Handling

Added explicit handling for potentially NULL values to maintain compatibility with the database schema:

$explode_mailbox = explode('@', $mailbox);
$mailbox_user = $explode_mailbox[0] ?? $mailbox;
$mailbox_domain = !empty($explode_mailbox[1]) ? $explode_mailbox[1] : '';
...
$folder = rcube_utils::get_input_value('_fetchmailfolder', rcube_utils::INPUT_POST);
$folder = $folder ?? '';  // Default to empty string if NULL

5. Additional Data Access Controls

Added user ownership checks to prevent unauthorized access to others' data:

// Using parameterized query with mailbox check for security
$sql = "DELETE FROM fetchmail WHERE id = ? AND mailbox = ?";

Impact

These vulnerabilities could allow an authenticated attacker to:

1. Access or modify other users' email fetching configuration
2. Execute arbitrary SQL commands with the privileges of the database user
3. Potentially extract sensitive information from the database
4. In some configurations, potentially escalate to server compromise

Disclosure Timeline

• Vulnerability discovered during code review (4/26/2025)
• Fixes implemented to address all identified issues (4/26/2025)
• This report created for documentation and disclosure purposes (4/27/2025)

References

• OWASP SQL Injection Prevention Cheat Sheet
• CWE-89: Improper Neutralization of Special Elements used in an SQL Command

[PF4Public]

These vulnerabilities could allow an authenticated attacker

As I was saying 🤷🏻

[skg574]

If you read further, the missing authenticated user checks in actions could allow unauthenticated users to trigger them

[skg574]

Anyway, there is the report with the fixes. lmk if you have questions.

[PF4Public]

could allow unauthenticated users to trigger them

May I have a minimal repro for this one?

[PF4Public]

@skg574 ?