diff --git a/services/download/src/main/java/app/coronawarn/server/services/download/Application.java b/services/download/src/main/java/app/coronawarn/server/services/download/Application.java index 197d896442..ddc6c0897d 100644 --- a/services/download/src/main/java/app/coronawarn/server/services/download/Application.java +++ b/services/download/src/main/java/app/coronawarn/server/services/download/Application.java @@ -4,6 +4,7 @@ import app.coronawarn.server.services.download.config.DownloadServiceConfigValidator; import org.apache.logging.log4j.LogManager; +import org.hibernate.validator.messageinterpolation.ParameterMessageInterpolator; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.beans.factory.DisposableBean; @@ -15,6 +16,7 @@ import org.springframework.context.annotation.ComponentScan; import org.springframework.data.jdbc.repository.config.EnableJdbcRepositories; import org.springframework.validation.Validator; +import org.springframework.validation.beanvalidation.LocalValidatorFactoryBean; @SpringBootApplication @@ -45,4 +47,14 @@ public static Validator configurationPropertiesValidator() { return new DownloadServiceConfigValidator(); } + /** + * Validation factory bean is configured here because its message interpolation mechanism + * is considered a potential threat if enabled. + */ + @Bean + public static LocalValidatorFactoryBean defaultValidator() { + LocalValidatorFactoryBean factoryBean = new LocalValidatorFactoryBean(); + factoryBean.setMessageInterpolator(new ParameterMessageInterpolator()); + return factoryBean; + } } diff --git a/services/submission/src/main/java/app/coronawarn/server/services/submission/config/SecurityConfig.java b/services/submission/src/main/java/app/coronawarn/server/services/submission/config/SecurityConfig.java index d2f306f169..d344720a4a 100644 --- a/services/submission/src/main/java/app/coronawarn/server/services/submission/config/SecurityConfig.java +++ b/services/submission/src/main/java/app/coronawarn/server/services/submission/config/SecurityConfig.java @@ -4,14 +4,21 @@ import app.coronawarn.server.services.submission.controller.SubmissionController; import java.util.Arrays; +import javax.validation.Validator; +import org.hibernate.validator.messageinterpolation.ParameterMessageInterpolator; +import org.springframework.beans.factory.config.BeanDefinition; +import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean; +import org.springframework.boot.validation.MessageInterpolatorFactory; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; +import org.springframework.context.annotation.Role; import org.springframework.http.HttpMethod; import org.springframework.security.config.annotation.web.builders.HttpSecurity; import org.springframework.security.config.annotation.web.configuration.EnableWebSecurity; import org.springframework.security.config.annotation.web.configuration.WebSecurityConfigurerAdapter; import org.springframework.security.web.firewall.HttpFirewall; import org.springframework.security.web.firewall.StrictHttpFirewall; +import org.springframework.validation.beanvalidation.LocalValidatorFactoryBean; @Configuration @EnableWebSecurity @@ -44,4 +51,14 @@ protected void configure(HttpSecurity http) throws Exception { http.headers().contentSecurityPolicy("default-src 'self'"); } + /** + * Validation factory bean is configured here because its message interpolation mechanism + * is considered a potential threat if enabled. + */ + @Bean + public static LocalValidatorFactoryBean defaultValidator() { + LocalValidatorFactoryBean factoryBean = new LocalValidatorFactoryBean(); + factoryBean.setMessageInterpolator(new ParameterMessageInterpolator()); + return factoryBean; + } } diff --git a/services/submission/src/main/java/app/coronawarn/server/services/submission/validation/ValidSubmissionPayload.java b/services/submission/src/main/java/app/coronawarn/server/services/submission/validation/ValidSubmissionPayload.java index 09c4a8c316..8f42812af3 100644 --- a/services/submission/src/main/java/app/coronawarn/server/services/submission/validation/ValidSubmissionPayload.java +++ b/services/submission/src/main/java/app/coronawarn/server/services/submission/validation/ValidSubmissionPayload.java @@ -131,8 +131,8 @@ private boolean checkOriginCountryIsValid(SubmissionPayload submissionPayload, String originCountry = submissionPayload.getOrigin(); if (submissionPayload.hasOrigin() && !StringUtils.isEmpty(originCountry) && !originCountry.equals(defaultOriginCountry)) { - addViolation(validatorContext, - "Submission payload contains an origin country which does not match the backend configuration."); + addViolation(validatorContext, String.format( + "Origin country %s is not part of the supported countries list", originCountry)); return false; } return true; @@ -148,7 +148,7 @@ private boolean checkVisitedCountriesAreValid(SubmissionPayload submissionPayloa if (!invalidVisitedCountries.isEmpty()) { invalidVisitedCountries.forEach(country -> addViolation(validatorContext, - "Key contains visited country which is not part of the supported countries list")); + "[" + country + "]: Visited country is not part of the supported countries list")); } return invalidVisitedCountries.isEmpty(); } diff --git a/services/submission/src/test/java/app/coronawarn/server/services/submission/controller/SubmissionPayloadMockData.java b/services/submission/src/test/java/app/coronawarn/server/services/submission/controller/SubmissionPayloadMockData.java index 99e6363a21..ff8c5f3c69 100644 --- a/services/submission/src/test/java/app/coronawarn/server/services/submission/controller/SubmissionPayloadMockData.java +++ b/services/submission/src/test/java/app/coronawarn/server/services/submission/controller/SubmissionPayloadMockData.java @@ -136,6 +136,18 @@ public static Collection buildMultipleKeysWithoutDSOSAndTR .collect(Collectors.toCollection(ArrayList::new)); } + public static SubmissionPayload buildPayloadWithOriginCountry(String originCountry) { + TemporaryExposureKey key = + buildTemporaryExposureKey(VALID_KEY_DATA_1, createRollingStartIntervalNumber(2), 3, + ReportType.CONFIRMED_TEST, 1); + return SubmissionPayload.newBuilder() + .addKeys(key) + .addAllVisitedCountries(List.of("DE", "FR")) + .setOrigin(originCountry) + .setRequestPadding(ByteString.copyFrom("PaddingString".getBytes())) + .build(); + } + public static SubmissionPayload buildPayloadWithVisitedCountries(List visitedCountries) { TemporaryExposureKey key = buildTemporaryExposureKey(VALID_KEY_DATA_1, createRollingStartIntervalNumber(2), 3, diff --git a/services/submission/src/test/java/app/coronawarn/server/services/submission/validation/ValidSubmissionPayloadTest.java b/services/submission/src/test/java/app/coronawarn/server/services/submission/validation/ValidSubmissionPayloadTest.java new file mode 100644 index 0000000000..7b1a14390f --- /dev/null +++ b/services/submission/src/test/java/app/coronawarn/server/services/submission/validation/ValidSubmissionPayloadTest.java @@ -0,0 +1,89 @@ +package app.coronawarn.server.services.submission.validation; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.api.Assertions.*; +import static org.springframework.http.HttpStatus.BAD_REQUEST; + +import java.util.List; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.mockito.ArgumentCaptor; +import org.mockito.Captor; +import org.mockito.Mockito; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.boot.test.mock.mockito.MockBean; +import org.springframework.http.ResponseEntity; +import org.springframework.test.annotation.DirtiesContext; +import org.springframework.test.context.ActiveProfiles; +import app.coronawarn.server.common.protocols.internal.SubmissionPayload; +import app.coronawarn.server.services.submission.controller.ApiExceptionHandler; +import app.coronawarn.server.services.submission.controller.RequestExecutor; +import app.coronawarn.server.services.submission.controller.SubmissionPayloadMockData; + +@SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT) +@DirtiesContext +@ActiveProfiles({"disable-ssl-client-verification", + "disable-ssl-client-verification-verify-hostname"}) +/* This test must have the public modifier to be callable by the interpolation logic */ +public class ValidSubmissionPayloadTest { + + private static String interpolationSideEffect; + + @MockBean + private ApiExceptionHandler apiExceptionHandler; + + @Captor + private ArgumentCaptor apiExceptionCaptor; + + + @Autowired + private RequestExecutor requestExecutor; + + @BeforeEach + public void setup() { + interpolationSideEffect = ""; + Mockito.doNothing().when(apiExceptionHandler).diagnosisKeyExceptions(apiExceptionCaptor.capture(), Mockito.any()); + } + + /** + * Provide an EL expression in the protobuf message which modifies a member of this test class. + * This string member can then be used to test whether interpolation was performed by the Java + * Validation Framework during submission. + */ + @Test + void testOriginCountryConstraintViolationInterpolationIsTurnedOff() { + SubmissionPayload payload = SubmissionPayloadMockData + .buildPayloadWithOriginCountry("java.lang.Runtime.getRuntime().exec(\'%s\');//${" + + "\'\'.getClass().forName(\'app.coronawarn.server.services.submission.validation.ValidSubmissionPayloadTest\')" + + ".newInstance().showInterpolationEffect()}"); + + ResponseEntity response = requestExecutor.executePost(payload); + assertThat(response.getStatusCode()).isEqualTo(BAD_REQUEST); + assertEquals(apiExceptionCaptor.getValue().getMessage(), + "submitDiagnosisKey.exposureKeys: Origin country java.lang.Runtime.getRuntime().exec('%s');//${''.getClass()." + + "forName('app.coronawarn.server.services.submission.validation.ValidSubmissionPayloadTest').newInstance().showInterpolationEffect()}" + + " is not part of the supported countries list"); + assertNotEquals("INTERPOLATION_OCCURRED", interpolationSideEffect); + } + + @Test + void testVisitedCountryConstraintViolationInterpolationIsTurnedOff() { + SubmissionPayload payload = SubmissionPayloadMockData + .buildPayloadWithVisitedCountries(List.of("java.lang.Runtime.getRuntime().exec(\'%s\');//${" + + "\'\'.getClass().forName(\'app.coronawarn.server.services.submission.validation.ValidSubmissionPayloadTest\')" + + ".newInstance().showInterpolationEffect()}")); + + ResponseEntity response = requestExecutor.executePost(payload); + assertThat(response.getStatusCode()).isEqualTo(BAD_REQUEST); + assertEquals(apiExceptionCaptor.getValue().getMessage(), + "submitDiagnosisKey.exposureKeys: [java.lang.Runtime.getRuntime().exec('%s');//${''.getClass()." + + "forName('app.coronawarn.server.services.submission.validation.ValidSubmissionPayloadTest').newInstance().showInterpolationEffect()}]:" + + " Visited country is not part of the supported countries list"); + assertNotEquals("INTERPOLATION_OCCURRED", interpolationSideEffect); + } + + public static void showInterpolationEffect() { + interpolationSideEffect = "INTERPOLATION_OCCURRED"; + } +}