CVE-2022-24838: Improper Neutralization of Special Elements in Output Used by a Downstream Component ('Injection') (CWE-74)
value: Nextcloud Calendar is a calendar application for the nextcloud framework. SMTP Command Injection in Appointment Emails via Newlines: as newlines and special characters are not sanitized in the email value in the JSON request, a malicious attacker can inject newlines to break out of the `RCPT TO:<BOOKING USER'S EMAIL> ` SMTP command and begin injecting arbitrary SMTP commands. It is recommended that Calendar is upgraded to 3.2.2. There are no workaround available.
CWE description: The product constructs all or part of a command, data structure, or record using externally-influenced input from an upstream component, but it does not neutralize or incorrectly neutralizes special elements that could modify how it is parsed or interpreted when it is sent to a downstream component.
CVSS v3 base score: 9.8
Published: 2022-04-11T21:15Z
Repository: https://github.com/nextcloud/calendar
Language: PHP
Files changed: lib/Controller/BookingController.php, tests/php/unit/Controller/BookingControllerTest.php
Commit date: 2022-03-16 08:49:59 +0000
Commit message: Add email validation and testing for booking

Signed-off-by: Anna Larch <anna@nextcloud.com>

Diff: diff --git a/lib/Controller/BookingController.php b/lib/Controller/BookingController.php
index 92cc5aac..2da8a759 100644
--- a/lib/Controller/BookingController.php
+++ b/lib/Controller/BookingController.php
@@ -41,10 +41,11 @@ use OCP\AppFramework\Http;
 use OCP\AppFramework\Http\TemplateResponse;
 use OCP\AppFramework\Services\IInitialState;
 use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\DB\Exception;
 use OCP\IRequest;
+use OCP\Mail\IMailer;
 use Psr\Log\LoggerInterface;
 
 class BookingController extends Controller {
 
 	/** @var BookingService */
@@ -63,26 +64,31 @@ class BookingController extends Controller {
 	private $urlGenerator;
 
 	/** @var LoggerInterface */
 	private $logger;
 
-	public function __construct(string                   $appName,
-								IRequest                 $request,
-								ITimeFactory             $timeFactory,
-								IInitialState            $initialState,
-								BookingService           $bookingService,
+	/** @var IMailer */
+	private $mailer;
+
+	public function __construct(string $appName,
+								IRequest $request,
+								ITimeFactory $timeFactory,
+								IInitialState $initialState,
+								BookingService $bookingService,
 								AppointmentConfigService $appointmentConfigService,
-								URLGenerator             $urlGenerator,
-								LoggerInterface $logger) {
+								URLGenerator $urlGenerator,
+								LoggerInterface $logger,
+								IMailer $mailer) {
 		parent::__construct($appName, $request);
 
 		$this->bookingService = $bookingService;
 		$this->timeFactory = $timeFactory;
 		$this->appointmentConfigService = $appointmentConfigService;
 		$this->initialState = $initialState;
 		$this->urlGenerator = $urlGenerator;
 		$this->logger = $logger;
+		$this->mailer = $mailer;
 	}
 
 	/**
 	 * @NoAdminRequired
 	 * @PublicPage
@@ -152,17 +158,21 @@ class BookingController extends Controller {
 	 * @param string $email
 	 * @param string $description
 	 * @param string $timeZone
 	 * @return JsonResponse
 	 */
-	public function bookSlot(int    $appointmentConfigId,
-							 int    $start,
-							 int    $end,
+	public function bookSlot(int $appointmentConfigId,
+							 int $start,
+							 int $end,
 							 string $displayName,
 							 string $email,
 							 string $description,
 							 string $timeZone): JsonResponse {
+		if (!$this->mailer->validateMailAddress($email)) {
+			return JsonResponse::fail('Invalid email address', Http::STATUS_UNPROCESSABLE_ENTITY);
+		}
+
 		if ($start > $end) {
 			return JsonResponse::fail('Invalid time range', Http::STATUS_UNPROCESSABLE_ENTITY);
 		}
 
 		try {
diff --git a/tests/php/unit/Controller/BookingControllerTest.php b/tests/php/unit/Controller/BookingControllerTest.php
index 5044944a..0c6bd02a 100644
--- a/tests/php/unit/Controller/BookingControllerTest.php
+++ b/tests/php/unit/Controller/BookingControllerTest.php
@@ -25,21 +25,26 @@ declare(strict_types=1);
 namespace OCA\Calendar\Controller;
 
 use ChristophWurst\Nextcloud\Testing\TestCase;
 use DateTimeZone;
 use Exception;
+use InvalidArgumentException;
 use OC\URLGenerator;
 use OCA\Calendar\Db\AppointmentConfig;
+use OCA\Calendar\Db\Booking;
+use OCA\Calendar\Exception\NoSlotFoundException;
+use OCA\Calendar\Exception\ServiceException;
 use OCA\Calendar\Service\Appointments\AppointmentConfigService;
 use OCA\Calendar\Service\Appointments\BookingService;
 use OCP\AppFramework\Services\IInitialState;
 use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\Calendar\ICalendarQuery;
 use OCP\Contacts\IManager;
 use OCP\IInitialStateService;
 use OCP\IRequest;
 use OCP\IUser;
+use OCP\Mail\IMailer;
 use PHPUnit\Framework\MockObject\MockObject;
 use Psr\Log\LoggerInterface;
 use Safe\DateTimeImmutable;
 
 class BookingControllerTest extends TestCase {
@@ -78,10 +83,13 @@ class BookingControllerTest extends TestCase {
 	private $urlGenerator;
 
 	/** @var mixed|MockObject|LoggerInterface */
 	private $logger;
 
+	/** @var IMailer|MockObject */
+	private $mailer;
+
 	protected function setUp():void {
 		parent::setUp();
 
 		if (!interface_exists(ICalendarQuery::class)) {
 			self::markTestIncomplete();
@@ -93,19 +101,21 @@ class BookingControllerTest extends TestCase {
 		$this->initialState = $this->createMock(IInitialState::class);
 		$this->bookingService = $this->createMock(BookingService::class);
 		$this->apptService = $this->createMock(AppointmentConfigService::class);
 		$this->urlGenerator = $this->createMock(URLGenerator::class);
 		$this->logger = $this->createMock(LoggerInterface::class);
+		$this->mailer = $this->createMock(IMailer::class);
 		$this->controller = new BookingController(
 			$this->appName,
 			$this->request,
 			$this->time,
 			$this->initialState,
 			$this->bookingService,
 			$this->apptService,
 			$this->urlGenerator,
-			$this->logger
+			$this->logger,
+			$this->mailer
 		);
 	}
 
 	public function testGetBookableSlots(): void {
 		$start = time();
@@ -169,6 +179,114 @@ class BookingControllerTest extends TestCase {
 		$this->logger->expects(self::once())
 			->method('warning');
 
 		$this->controller->getBookableSlots($apptConfg->getId(), $start,'Europe/Berlin');
 	}
+
+	public function testBook(): void {
+		$email = 'penny@stardewvalley.edu';
+		$config = new AppointmentConfig();
+
+		$this->mailer->expects(self::once())
+			->method('validateMailAddress')
+			->with($email)
+			->willReturn(true);
+		$this->apptService->expects(self::once())
+			->method('findById')
+			->willReturn($config);
+		$this->bookingService->expects(self::once())
+			->method('book')
+			->with($config, 1, 1, 'Hook/Neverland', 'Test', $email, 'Test')
+			->willReturn(new Booking());
+
+		$this->controller->bookSlot(1, 1, 1, 'Test', $email, 'Test', 'Hook/Neverland');
+	}
+
+
+	public function testBookInvalidTimeZone(): void {
+		$email = 'penny@stardewvalley.edu';
+		$config = new AppointmentConfig();
+
+		$this->mailer->expects(self::once())
+			->method('validateMailAddress')
+			->with($email)
+			->willReturn(true);
+		$this->apptService->expects(self::once())
+			->method('findById')
+			->willReturn($config);
+		$this->bookingService->expects(self::once())
+			->method('book')
+			->with($config, 1, 1, 'Hook/Neverland', 'Test', $email, 'Test')
+			->willThrowException(new InvalidArgumentException());
+
+		$this->controller->bookSlot(1, 1, 1, 'Test', $email, 'Test', 'Hook/Neverland');
+	}
+
+	public function testBookInvalidSlot(): void {
+		$email = 'penny@stardewvalley.edu';
+		$config = new AppointmentConfig();
+
+		$this->mailer->expects(self::once())
+			->method('validateMailAddress')
+			->with($email)
+			->willReturn(true);
+		$this->apptService->expects(self::once())
+			->method('findById')
+			->willReturn($config);
+		$this->bookingService->expects(self::once())
+			->method('book')
+			->with($config, 1, 1, 'Europe/Berlin', 'Test', $email, 'Test')
+			->willThrowException(new NoSlotFoundException());
+
+		$this->controller->bookSlot(1, 1, 1, 'Test', $email, 'Test', 'Europe/Berlin');
+	}
+
+	public function testBookInvalidBooking(): void {
+		$email = 'penny@stardewvalley.edu';
+		$config = new AppointmentConfig();
+
+		$this->mailer->expects(self::once())
+			->method('validateMailAddress')
+			->with($email)
+			->willReturn(true);
+		$this->apptService->expects(self::once())
+			->method('findById')
+			->willReturn($config);
+		$this->bookingService->expects(self::once())
+			->method('book')
+			->with($config, 1, 1, 'Europe/Berlin', 'Test', $email, 'Test')
+			->willThrowException(new ServiceException());
+
+		$this->controller->bookSlot(1, 1, 1, 'Test', $email, 'Test', 'Europe/Berlin');
+	}
+
+	public function testBookInvalidId(): void {
+		$email = 'penny@stardewvalley.edu';
+		$this->mailer->expects(self::once())
+			->method('validateMailAddress')
+			->with($email)
+			->willReturn(true);
+		$this->apptService->expects(self::once())
+			->method('findById')
+			->willThrowException(new ServiceException());
+		$this->bookingService->expects(self::never())
+			->method('book');
+
+		$this->controller->bookSlot(1, 1, 1, 'Test', $email, 'Test', 'Europe/Berlin');
+	}
+
+
+	public function testBookInvalidEmail(): void {
+		$email = 'testing-abcdef';
+
+		$this->mailer->expects(self::once())
+			->method('validateMailAddress')
+			->with($email)
+			->willReturn(false);
+		$this->apptService->expects(self::never())
+			->method('findById');
+		$this->bookingService->expects(self::never())
+			->method('book');
+
+		$this->controller->bookSlot(1, 1, 1, 'Test', $email, 'Test', 'Europe/Berlin');
+	}
 }

Vulnerable code: 	public function __construct(string                   $appName,
								IRequest                 $request,
								ITimeFactory             $timeFactory,
								IInitialState            $initialState,
								BookingService           $bookingService,
								URLGenerator             $urlGenerator,
								LoggerInterface $logger) {
	public function bookSlot(int    $appointmentConfigId,
							 int    $start,
							 int    $end,
			$this->logger
Fixed code: use OCP\Mail\IMailer;
	/** @var IMailer */
	private $mailer;

	public function __construct(string $appName,
								IRequest $request,
								ITimeFactory $timeFactory,
								IInitialState $initialState,
								BookingService $bookingService,
								URLGenerator $urlGenerator,
								LoggerInterface $logger,
								IMailer $mailer) {
		$this->mailer = $mailer;
	public function bookSlot(int $appointmentConfigId,
							 int $start,
							 int $end,
		if (!$this->mailer->validateMailAddress($email)) {
			return JsonResponse::fail('Invalid email address', Http::STATUS_UNPROCESSABLE_ENTITY);
		}

use InvalidArgumentException;
use OCA\Calendar\Db\Booking;
use OCA\Calendar\Exception\NoSlotFoundException;
use OCA\Calendar\Exception\ServiceException;
use OCP\Mail\IMailer;
	/** @var IMailer|MockObject */
	private $mailer;

		$this->mailer = $this->createMock(IMailer::class);
			$this->logger,
			$this->mailer

	public function testBook(): void {
		$email = 'penny@stardewvalley.edu';
		$config = new AppointmentConfig();

		$this->mailer->expects(self::once())
			->method('validateMailAddress')
			->with($email)
			->willReturn(true);
		$this->apptService->expects(self::once())
			->method('findById')
			->willReturn($config);
		$this->bookingService->expects(self::once())
			->method('book')
			->with($config, 1, 1, 'Hook/Neverland', 'Test', $email, 'Test')
			->willReturn(new Booking());

		$this->controller->bookSlot(1, 1, 1, 'Test', $email, 'Test', 'Hook/Neverland');
	}


	public function testBookInvalidTimeZone(): void {
		$email = 'penny@stardewvalley.edu';
		$config = new AppointmentConfig();

		$this->mailer->expects(self::once())
			->method('validateMailAddress')
			->with($email)
			->willReturn(true);
		$this->apptService->expects(self::once())
			->method('findById')
			->willReturn($config);
		$this->bookingService->expects(self::once())
			->method('book')
			->with($config, 1, 1, 'Hook/Neverland', 'Test', $email, 'Test')
			->willThrowException(new InvalidArgumentException());

		$this->controller->bookSlot(1, 1, 1, 'Test', $email, 'Test', 'Hook/Neverland');
	}

	public function testBookInvalidSlot(): void {
		$email = 'penny@stardewvalley.edu';
		$config = new AppointmentConfig();

		$this->mailer->expects(self::once())
			->method('validateMailAddress')
			->with($email)
			->willReturn(true);
		$this->apptService->expects(self::once())
			->method('findById')
			->willReturn($config);
		$this->bookingService->expects(self::once())
			->method('book')
			->with($config, 1, 1, 'Europe/Berlin', 'Test', $email, 'Test')
			->willThrowException(new NoSlotFoundException());

		$this->controller->bookSlot(1, 1, 1, 'Test', $email, 'Test', 'Europe/Berlin');
	}

	public function testBookInvalidBooking(): void {
		$email = 'penny@stardewvalley.edu';
		$config = new AppointmentConfig();

		$this->mailer->expects(self::once())
			->method('validateMailAddress')
			->with($email)
			->willReturn(true);
		$this->apptService->expects(self::once())
			->method('findById')
			->willReturn($config);
		$this->bookingService->expects(self::once())
			->method('book')
			->with($config, 1, 1, 'Europe/Berlin', 'Test', $email, 'Test')
			->willThrowException(new ServiceException());

		$this->controller->bookSlot(1, 1, 1, 'Test', $email, 'Test', 'Europe/Berlin');
	}

	public function testBookInvalidId(): void {
		$email = 'penny@stardewvalley.edu';
		$this->mailer->expects(self::once())
			->method('validateMailAddress')
			->with($email)
			->willReturn(true);
		$this->apptService->expects(self::once())
			->method('findById')
			->willThrowException(new ServiceException());
		$this->bookingService->expects(self::never())
			->method('book');

		$this->controller->bookSlot(1, 1, 1, 'Test', $email, 'Test', 'Europe/Berlin');
	}


	public function testBookInvalidEmail(): void {
		$email = 'testing-abcdef';

		$this->mailer->expects(self::once())
			->method('validateMailAddress')
			->with($email)
			->willReturn(false);
		$this->apptService->expects(self::never())
			->method('findById');
		$this->bookingService->expects(self::never())
			->method('book');

		$this->controller->bookSlot(1, 1, 1, 'Test', $email, 'Test', 'Europe/Berlin');
	}