# Vulnerability Report: SQL Injection and Encryption Weaknesses in Fetchmail Plugin for Roundcube

## Overview

A security audit of the Fetchmail plugin for Roundcube webmail has uncovered multiple SQL injection vulnerabilities and encryption weaknesses. 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. Additionally, the password storage mechanism used base64 encoding rather than strong encryption, exposing sensitive credentials to potential compromise.

## Vulnerability Details

### Vulnerability Types
- SQL Injection (CWE-89)
- Insecure Password Storage (CWE-256)
- Logic Error in Conditional Statements (CWE-670)
- **Reported**: Yes.  Package maintainer did not consider it a vulnerability because it required an active account to compromise.   We patched.

### Affected Component
Roundcube Fetchmail Plugin (`fetchmail.php` and `fetchmail.pl`)

### 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. Additionally, password storage relied on base64 encoding (which is easily reversible) rather than strong encryption, and there were logical errors in conditional statements that could lead to unexpected behavior.

## Specific Vulnerabilities Found

### 1. SQL Injection in Delete Function
In the original code:
```php
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:**
```php
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:**
```php
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:**
```php
$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.

### 5. Insecure Password Storage
The plugin used base64 encoding for password storage, which is easily reversible and provides no cryptographic protection:

```php
$pass = base64_encode(rcube_utils::get_input_value('_fetchmailpass', rcube_utils::INPUT_POST));
```

### 6. Logical Errors in Conditional Statements
Several conditions used incorrect operators or logical combinations that could lead to unexpected behavior:

```php
// This condition is always true for any $id value
if ($id != '' || $id != 0)
```

## 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:

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

```php
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):**
```php
$sql = "DELETE FROM fetchmail WHERE id = '$id'";
$delete = $this->db->query($sql);
```

**Fixed Delete Function:**
```php
$sql = "DELETE FROM fetchmail WHERE id = ? AND mailbox = ?";
$delete = $this->db->query($sql, $id, $mailbox);
```

**Original Insert Function (vulnerable):**
```php
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:**
```php
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');
}
```

### 3. Fixed Logic Errors
Changed the conditional checks from OR to AND and implemented strict type comparison to ensure proper validation:

**Original (vulnerable):**
```php
if ($id != 0 || $id != '')
```

**Fixed:**
```php
if ($id !== 0 && $id !== '')
```

### 4. Null Value Handling
Added explicit handling for potentially NULL values to maintain compatibility with the database schema:
```php
$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:
```php
// Using parameterized query with mailbox check for security
$sql = "DELETE FROM fetchmail WHERE id = ? AND mailbox = ?";
```

### 6. Improved Password Storage with AES-GCM Encryption
Implemented AES-GCM authenticated encryption for password storage instead of base64 encoding:

**Original (vulnerable):**
```php
$pass = base64_encode(rcube_utils::get_input_value('_fetchmailpass', rcube_utils::INPUT_POST));
```

**Fixed with AES-GCM encryption:**
```php
private function encrypt_password($password) {
    $encryption_key = $this->rc->config->get('fetchmail_encryption_key', 'ChangeThisTestEncryptionKeyForFetchmail123');
    
    // For AES-GCM, we need a 12-byte (96-bit) nonce/IV
    $nonce = openssl_random_pseudo_bytes(12);
    
    // Tag for authentication
    $tag = '';
    
    // Encrypt with AES-256-GCM
    $encrypted = openssl_encrypt(
        $password,
        'aes-256-gcm',
        $encryption_key,
        OPENSSL_RAW_DATA,
        $nonce,
        $tag,
        '', // No additional authenticated data
        16  // Tag length: 16 bytes (128 bits)
    );
    
    // Format: base64(nonce + tag + ciphertext)
    return base64_encode($nonce . $tag . $encrypted);
}
```

**Modified Save Method:**
```php
$password = rcube_utils::get_input_value('_fetchmailpass', rcube_utils::INPUT_POST);
$pass = $this->encrypt_password($password);
```

**Corresponding Decryption Method for fetchmail.pl:**
```perl
sub decrypt_password {
    my ($encrypted_data) = @_;
    
    # Decode from base64
    my $decoded = decode_base64($encrypted_data);
    
    # Extract components
    my $nonce = substr($decoded, 0, 12);
    my $tag = substr($decoded, 12, 16);
    my $ciphertext = substr($decoded, 28);
    
    # Create GCM decryption object
    my $gcm = Crypt::AuthEnc::GCM->new("AES", $encryption_key);
    
    # Set the nonce (IV)
    $gcm->iv_add($nonce);
    
    # Decrypt
    my $plaintext = $gcm->decrypt_add($ciphertext);
    
    # Verify tag
    my $result = $gcm->decrypt_done($tag);
    
    if (!$result) {
        syslog("err", "Authentication tag verification failed - password data may be corrupted");
        return ""; # Return empty string on error
    }
    
    return $plaintext;
}
```

## 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
5. Decrypt passwords stored with base64 encoding (prior to the encryption fix)


## References

- [OWASP SQL Injection Prevention Cheat Sheet](https://cheatsheetseries.owasp.org/cheatsheets/SQL_Injection_Prevention_Cheat_Sheet.html)
- [CWE-89: Improper Neutralization of Special Elements used in an SQL Command](https://cwe.mitre.org/data/definitions/89.html)
- [CWE-256: Unprotected Storage of Credentials](https://cwe.mitre.org/data/definitions/256.html)
- [CWE-670: Always-Incorrect Control Flow Implementation](https://cwe.mitre.org/data/definitions/670.html)

## Contact

security@codamail.com