Merge pull request #2131 from nordic-institute/XRDDEV-2620

fix: Don't reveal DB data in error response
nordic-institute · May 9, 2024 · f1a0766 · f1a0766
2 parents b1c705a + 4f2aa48
commit f1a0766
Show file tree

Hide file tree

Showing 12 changed files with 122 additions and 36 deletions.
diff --git a/src/common/common-core/src/main/java/ee/ria/xroad/common/ErrorCodes.java b/src/common/common-core/src/main/java/ee/ria/xroad/common/ErrorCodes.java
@@ -202,8 +202,8 @@ public final class ErrorCodes {
      */
     @SuppressWarnings("squid:S1872")
     public static CodedException translateException(Throwable ex) {
-        if (ex instanceof CodedException) {
-            return (CodedException) ex;
+        if (ex instanceof CodedException cex) {
+            return cex;
         } else if (ex instanceof UnknownHostException
                 || ex instanceof MalformedURLException
                 || ex instanceof SocketException

diff --git a/src/common/common-db/src/main/java/ee/ria/xroad/common/db/HibernateUtil.java b/src/common/common-db/src/main/java/ee/ria/xroad/common/db/HibernateUtil.java
@@ -92,7 +92,7 @@ public static synchronized SessionFactory getSessionFactory(String name, Interce
             } catch (Exception e) {
                 log.error("Failed to create session factory", e);
 
-                throw new CodedException(X_DATABASE_ERROR, e);
+                throw new CodedException(X_DATABASE_ERROR, e, "Error accessing database (%s)", name);
             }
         }
     }
@@ -144,7 +144,7 @@ private static void closeSessionFactory(SessionFactoryCtx ctx) {
     private static SessionFactoryCtx createSessionFactoryCtx(String name, Interceptor interceptor) throws Exception {
         log.trace("Creating session factory for '{}'...", name);
 
-        Configuration configuration = new Configuration();
+        Configuration configuration = createEmptyConfiguration();
         if (interceptor != null) {
             configuration.setInterceptor(interceptor);
         }
@@ -160,6 +160,10 @@ private static SessionFactoryCtx createSessionFactoryCtx(String name, Intercepto
         return new SessionFactoryCtx(sessionFactory);
     }
 
+    static Configuration createEmptyConfiguration() {
+        return new Configuration();
+    }
+
     private static void applySystemProperties(Configuration configuration, String name) {
         final String prefix = name + ".hibernate.";
         for (String key : System.getProperties().stringPropertyNames()) {

diff --git a/src/common/common-db/src/test/java/ee/ria/xroad/common/db/HibernateUtilTest.java b/src/common/common-db/src/test/java/ee/ria/xroad/common/db/HibernateUtilTest.java
@@ -0,0 +1,83 @@
+/*
+ * The MIT License
+ * Copyright (c) 2019- Nordic Institute for Interoperability Solutions (NIIS)
+ * Copyright (c) 2018 Estonian Information System Authority (RIA),
+ * Nordic Institute for Interoperability Solutions (NIIS), Population Register Centre (VRK)
+ * Copyright (c) 2015-2017 Estonian Information System Authority (RIA), Population Register Centre (VRK)
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+package ee.ria.xroad.common.db;
+
+import ee.ria.xroad.common.CodedException;
+import ee.ria.xroad.common.SystemProperties;
+
+import org.hibernate.HibernateException;
+import org.hibernate.cfg.Configuration;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.Mock;
+import org.mockito.MockedStatic;
+import org.mockito.Mockito;
+import org.mockito.junit.jupiter.MockitoExtension;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.jupiter.api.Assertions.fail;
+import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.Mockito.CALLS_REAL_METHODS;
+import static org.mockito.Mockito.when;
+
+@ExtendWith(MockitoExtension.class)
+class HibernateUtilTest {
+    private static final String TEST_SESSION_FACTORY_NAME = "testSessionFactory";
+
+    @Mock
+    private Configuration configuration;
+
+    @BeforeEach
+    public void setUp() {
+        when(configuration.configure()).thenReturn(configuration);
+        when(configuration.configure(anyString())).thenReturn(configuration);
+    }
+
+    @Test
+    void getSessionFactoryHideErrorDetails() {
+
+        try (
+                MockedStatic<HibernateUtil> db = Mockito.mockStatic(HibernateUtil.class, CALLS_REAL_METHODS);
+                MockedStatic<SystemProperties> system = Mockito.mockStatic(SystemProperties.class)
+        ) {
+            db.when(HibernateUtil::createEmptyConfiguration).thenReturn(configuration);
+            system.when(SystemProperties::getDatabasePropertiesFile)
+                    .thenReturn(HibernateUtilTest.class.getResource("/empty_db.properties").getFile());
+            when(configuration.buildSessionFactory())
+                    .thenThrow(new HibernateException("username and ip address"));
+            HibernateUtil.getSessionFactory(TEST_SESSION_FACTORY_NAME);
+
+            fail("Should have thrown an exception");
+        } catch (Exception e) {
+            assertThat(e)
+                    .isInstanceOf(CodedException.class)
+                    .hasMessageContaining("DatabaseError: Error accessing database (testSessionFactory)");
+        }
+
+
+    }
+}
diff --git a/src/common/common-db/src/test/resources/empty_db.properties b/src/common/common-db/src/test/resources/empty_db.properties
diff --git a/src/common/common-jetty/src/main/java/ee/ria/xroad/common/util/HandlerBase.java b/src/common/common-jetty/src/main/java/ee/ria/xroad/common/util/HandlerBase.java
@@ -36,6 +36,7 @@
 
 import java.io.IOException;
 import java.nio.ByteBuffer;
+import java.nio.charset.StandardCharsets;
 
 import static ee.ria.xroad.common.util.JettyUtils.setContentLength;
 import static ee.ria.xroad.common.util.JettyUtils.setContentType;
@@ -58,8 +59,7 @@ public void sendErrorResponse(Request request,
                                   Response response,
                                   Callback callback,
                                   CodedException ex) throws IOException {
-        String faultXml = ex instanceof CodedException.Fault
-                ? ((CodedException.Fault) ex).getFaultXml() : SoapFault.createFaultXml(ex);
+        String faultXml = SoapFault.createFaultXml(ex);
         String encoding = MimeUtils.UTF8;
         byte[] messageBytes = faultXml.getBytes(encoding);
 
@@ -76,11 +76,9 @@ public void sendErrorResponse(Request request,
      * @param response HTTP servlet response for sending the plain fault
      * @param status   HTTP status code
      * @param message  fault message
-     * @throws IOException if an I/O error occurred
      */
-    public void sendPlainTextErrorResponse(Response response, Callback callback, int status, String message)
-            throws IOException {
-        byte[] messageBytes = message.getBytes("UTF-8");
+    public void sendPlainTextErrorResponse(Response response, Callback callback, int status, String message) {
+        byte[] messageBytes = message.getBytes(StandardCharsets.UTF_8);
         response.setStatus(status);
         setContentType(response, MimeTypes.TEXT_PLAIN_UTF8);
         setContentLength(response, messageBytes.length);

diff --git a/src/common/common-jetty/src/main/java/ee/ria/xroad/common/util/RequestWrapper.java b/src/common/common-jetty/src/main/java/ee/ria/xroad/common/util/RequestWrapper.java
@@ -28,11 +28,14 @@
 import org.apache.commons.io.input.ProxyInputStream;
 import org.eclipse.jetty.http.HttpFields;
 import org.eclipse.jetty.http.HttpURI;
+import org.eclipse.jetty.io.EndPoint;
 import org.eclipse.jetty.server.Request;
 
 import java.io.IOException;
 import java.io.InputStream;
+import java.security.cert.X509Certificate;
 import java.util.Map;
+import java.util.Optional;
 
 public interface RequestWrapper {
     InputStream getInputStream();
@@ -51,6 +54,8 @@ public interface RequestWrapper {
 
     Map<String, String[]> getParametersMap() throws Exception;
 
+    Optional<X509Certificate[]> getPeerCertificates();
+
     static RequestWrapper of(Request request) {
         var in = new ProxyInputStream(Request.asInputStream(request)) {
             @Override
@@ -101,6 +106,13 @@ public String getParameter(String name) throws Exception {
             public Map<String, String[]> getParametersMap() throws Exception {
                 return Request.getParameters(request).toStringArrayMap();
             }
+
+            @Override
+            public Optional<X509Certificate[]> getPeerCertificates() {
+                var ssd = (EndPoint.SslSessionData) request.getAttribute(EndPoint.SslSessionData.ATTRIBUTE);
+                return Optional.ofNullable(ssd)
+                        .map(EndPoint.SslSessionData::peerCertificates);
+            }
         };
     }
 }
diff --git a/src/common/common-message/src/main/java/ee/ria/xroad/common/message/SaxSoapParserImpl.java b/src/common/common-message/src/main/java/ee/ria/xroad/common/message/SaxSoapParserImpl.java
@@ -56,6 +56,7 @@
 
 import java.io.BufferedWriter;
 import java.io.ByteArrayOutputStream;
+import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStreamWriter;
 import java.io.Writer;
@@ -263,8 +264,8 @@ protected boolean isProcessedXmlRequired() {
         return false;
     }
 
-    private InputStream excludeUtf8Bom(String contentType, InputStream soapStream) {
-        return hasUtf8Charset(contentType) ? new BOMInputStream(soapStream) : soapStream;
+    private InputStream excludeUtf8Bom(String contentType, InputStream soapStream) throws IOException {
+        return hasUtf8Charset(contentType) ? BOMInputStream.builder().setInputStream(soapStream).get() : soapStream;
     }
 
     @SneakyThrows

diff --git a/src/common/common-message/src/main/java/ee/ria/xroad/common/message/SoapFault.java b/src/common/common-message/src/main/java/ee/ria/xroad/common/message/SoapFault.java
@@ -29,7 +29,7 @@
 
 import jakarta.xml.soap.SOAPFault;
 import lombok.SneakyThrows;
-import org.apache.commons.lang3.StringEscapeUtils;
+import org.apache.commons.text.StringEscapeUtils;
 
 /**
  * Soap interface implementation representing an error message.

diff --git a/src/common/common-message/src/main/java/ee/ria/xroad/common/message/SoapParserImpl.java b/src/common/common-message/src/main/java/ee/ria/xroad/common/message/SoapParserImpl.java
@@ -46,6 +46,7 @@
 import javax.xml.namespace.QName;
 
 import java.io.ByteArrayInputStream;
+import java.io.IOException;
 import java.io.InputStream;
 import java.util.HashSet;
 import java.util.Iterator;
@@ -105,10 +106,8 @@ protected Soap parseMessage(String contentType, InputStream is)
         return parseMessage(rawXml, soap, charset, contentType);
     }
 
-    private InputStream excludeUtf8Bom(String contentType,
-                                       InputStream soapStream) {
-        return hasUtf8Charset(contentType)
-                ? new BOMInputStream(soapStream) : soapStream;
+    private InputStream excludeUtf8Bom(String contentType, InputStream soapStream) throws IOException {
+        return hasUtf8Charset(contentType) ? BOMInputStream.builder().setInputStream(soapStream).get() : soapStream;
     }
 
     protected Soap parseMessage(byte[] rawXml, SOAPMessage soap,

diff --git a/src/proxy/src/main/java/ee/ria/xroad/proxy/clientproxy/AbstractClientProxyHandler.java b/src/proxy/src/main/java/ee/ria/xroad/proxy/clientproxy/AbstractClientProxyHandler.java
@@ -40,7 +40,6 @@
 import lombok.extern.slf4j.Slf4j;
 import org.apache.commons.lang3.ArrayUtils;
 import org.apache.http.client.HttpClient;
-import org.eclipse.jetty.io.EndPoint;
 import org.eclipse.jetty.server.Request;
 import org.eclipse.jetty.server.Response;
 import org.eclipse.jetty.util.Callback;
@@ -158,7 +157,7 @@ protected void failure(MessageProcessorBase processor, Request request, Response
     }
 
     protected void failure(Response response, Callback callback, CodedExceptionWithHttpStatus e,
-                           OpMonitoringData opMonitoringData) throws IOException {
+                           OpMonitoringData opMonitoringData) {
 
         updateOpMonitoringResponseOutTs(opMonitoringData);
 
@@ -174,10 +173,11 @@ static boolean isPostRequest(RequestWrapper request) {
     }
 
     static IsAuthenticationData getIsAuthenticationData(RequestWrapper request) {
-        var ssd = (EndPoint.SslSessionData) request.getAttribute(EndPoint.SslSessionData.ATTRIBUTE);
-
         var isPlaintextConnection = !"https".equals(request.getHttpURI().getScheme()); // if not HTTPS, it's plaintext
-        var cert = (ssd == null || ArrayUtils.isEmpty(ssd.peerCertificates())) ? null : ssd.peerCertificates()[0];
+        var cert = request.getPeerCertificates()
+                .filter(ArrayUtils::isNotEmpty)
+                .map(arr -> arr[0])
+                .orElse(null);
         return new IsAuthenticationData(cert, isPlaintextConnection);
     }
 

diff --git a/src/proxy/src/main/java/ee/ria/xroad/proxy/serverproxy/ServerProxyHandler.java b/src/proxy/src/main/java/ee/ria/xroad/proxy/serverproxy/ServerProxyHandler.java
@@ -38,13 +38,11 @@
 
 import lombok.extern.slf4j.Slf4j;
 import org.apache.http.client.HttpClient;
-import org.eclipse.jetty.io.EndPoint;
 import org.eclipse.jetty.server.Request;
 import org.eclipse.jetty.server.Response;
 import org.eclipse.jetty.util.Callback;
 
 import java.io.IOException;
-import java.security.cert.X509Certificate;
 
 import static ee.ria.xroad.common.ErrorCodes.SERVER_SERVERPROXY_X;
 import static ee.ria.xroad.common.ErrorCodes.X_INVALID_HTTP_METHOD;
@@ -114,10 +112,10 @@ private MessageProcessorBase createRequestProcessor(RequestWrapper request, Resp
                                                         OpMonitoringData opMonitoringData) {
 
         if (VALUE_MESSAGE_TYPE_REST.equals(request.getHeaders().get(HEADER_MESSAGE_TYPE))) {
-            return new ServerRestMessageProcessor(request, response, client, getClientSslCertChain(request),
+            return new ServerRestMessageProcessor(request, response, client, request.getPeerCertificates().orElse(null),
                     opMonitoringData);
         } else {
-            return new ServerMessageProcessor(request, response, client, getClientSslCertChain(request),
+            return new ServerMessageProcessor(request, response, client, request.getPeerCertificates().orElse(null),
                     opMonitorClient, opMonitoringData);
         }
     }
@@ -127,13 +125,4 @@ protected void failure(Request request, Response response, Callback callback, Co
             throws IOException {
         sendErrorResponse(request, response, callback, e);
     }
-
-    private static X509Certificate[] getClientSslCertChain(RequestWrapper request) {
-        Object attribute = request.getAttribute(EndPoint.SslSessionData.ATTRIBUTE);
-        if (attribute != null) {
-            return ((EndPoint.SslSessionData) attribute).peerCertificates();
-        } else {
-            return null;
-        }
-    }
 }
diff --git a/src/proxy/src/main/java/ee/ria/xroad/proxy/util/CertHashBasedOcspResponder.java b/src/proxy/src/main/java/ee/ria/xroad/proxy/util/CertHashBasedOcspResponder.java
@@ -170,7 +170,7 @@ private void doHandleRequest(Request request, Response response) throws Exceptio
 
     private final class RequestHandler extends Handler.Abstract {
         @Override
-        public boolean handle(Request request, Response response, Callback callback) throws Exception {
+        public boolean handle(Request request, Response response, Callback callback) {
             log.trace("Received {} request from {}", request.getMethod(), getRemoteAddr(request));
 
             try {