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

M west2020 oin sql injection #114

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

MWest2020
Copy link
Contributor

Title: Mitigate SQL Injection Risk in OIN Handling within SyncXxllncCasesService

Description:

This PR addresses a potential SQL injection vulnerability in the SyncXxllncCasesService class, specifically related to the handling of the Overheidsidentificatienummer (OIN). The OIN is an identification number for organizations in the Netherlands and can begin with leading zeros. Improper validation of this number could allow for SQL injection attacks.

Background:

A security assessment revealed that the oin value used in database queries might be susceptible to SQL injection.
The issue arises because the oin is not properly validated or sanitized before being used in queries or passed to other services.
Changes Made:

Added OIN Validation Method:

Introduced a new private method validateOin to ensure the oin value is safe to use.
This method checks that the oin:
Is set and is a non-empty string.
Contains only digits, allowing for leading zeros (which are common in OINs).

private function validateOin($oin): string
{
    if (!isset($oin) || !is_string($oin) || trim($oin) === '') {
        throw new \InvalidArgumentException('OIN-waarde ontbreekt of is ongeldig.');
    }
    if (!preg_match('/^\d+$/', $oin)) {
        throw new \InvalidArgumentException('OIN-waarde moet alleen uit cijfers bestaan.');
    }
    return $oin;
}

Implemented Validation in Synchronization Handler:

In the syncXxllncCasesHandler method, the oin from the configuration is now validated using the validateOin method.
If validation fails, an error is logged, and the synchronization process is halted to prevent the use of invalid data.

// Validate the OIN value
try {
    $oin = $this->validateOin($this->configuration['oin']);
} catch (\InvalidArgumentException $e) {
    $this->logger->error('Validatie van OIN mislukt: ' . $e->getMessage(), ['plugin' => 'common-gateway/woo-bundle']);
    isset($this->style) === true && $this->style->error('Validatie van OIN mislukt: ' . $e->getMessage());
    return [];
}

Ensured Safe Use of OIN in Data Processing:

Updated the data merging process to use the validated oin value.
This ensures that only sanitized and validated data is used in further processing and any database interactions.

$result = array_merge(
    $result,
    [
        'settings'    => ['allowPDFOnly' => $configuration['allowPDFOnly']],
        'autoPublish' => $this->configuration['autoPublish'] ?? true,
        'organisatie' => [
            'oin'  => $oin,
            'naam' => $this->configuration['organisatie'],
        ],
    ]
);

Impact:

Security Improvement: By validating the oin value, we mitigate the risk of SQL injection attacks that could compromise the database and the application.
Data Integrity: Ensures that only valid OINs are processed, maintaining the integrity of organizational data.
Backward Compatibility: The changes are backward compatible and do not affect other functionalities of the SyncXxllncCasesService class.

Copy link

👋 @MWest2020
Thank you for raising your pull request.
Please make sure you have followed our contributing guidelines. We will review it as soon as possible. In the meanwhile make sure your PR checks the following boxes

  • Is based on an issue
  • Has been locally tested
  • Has been tested with the admin UI
  • Has been discussed with the development team in an open channel

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

Successfully merging this pull request may close these issues.

1 participant