diff --git a/google-http-client-xml/src/main/java/com/google/api/client/xml/Xml.java b/google-http-client-xml/src/main/java/com/google/api/client/xml/Xml.java index a5132839c..ef8e050fe 100644 --- a/google-http-client-xml/src/main/java/com/google/api/client/xml/Xml.java +++ b/google-http-client-xml/src/main/java/com/google/api/client/xml/Xml.java @@ -87,7 +87,13 @@ public static XmlSerializer createSerializer() { /** Returns a new XML pull parser. */ public static XmlPullParser createParser() throws XmlPullParserException { - return getParserFactory().newPullParser(); + XmlPullParser parser = getParserFactory().newPullParser(); + try { + parser.setFeature(XmlPullParser.FEATURE_PROCESS_DOCDECL, false); + } catch (XmlPullParserException e) { + // Ignore if the feature is not supported + } + return parser; } /** diff --git a/google-http-client-xml/src/test/java/com/google/api/client/xml/XmlTest.java b/google-http-client-xml/src/test/java/com/google/api/client/xml/XmlTest.java index d9a381424..940e73a1e 100644 --- a/google-http-client-xml/src/test/java/com/google/api/client/xml/XmlTest.java +++ b/google-http-client-xml/src/test/java/com/google/api/client/xml/XmlTest.java @@ -23,7 +23,9 @@ import com.google.api.client.util.ArrayMap; import com.google.api.client.util.Key; import java.io.ByteArrayOutputStream; +import java.io.IOException; import java.io.StringReader; +import org.xmlpull.v1.XmlPullParserException; import java.util.ArrayList; import java.util.Collection; import java.util.List; @@ -621,4 +623,26 @@ private static class AllType { @Key public String[] stringArray; @Key public List integerCollection; } + + @Test + public void testCreateParser_disablesDocDecl() throws Exception { + XmlPullParser parser = Xml.createParser(); + String xmlWithDtd = "\n" + + "\n" + + "]>\n" + + "&xxe;"; + parser.setInput(new StringReader(xmlWithDtd)); + try { + SimpleTypeString xml = new SimpleTypeString(); + XmlNamespaceDictionary namespaceDictionary = new XmlNamespaceDictionary().set("", ""); + Xml.parseElement(parser, xml, namespaceDictionary, null); + if (xml.value != null) { + assertTrue(xml.value.isEmpty() || xml.value.equals("&xxe;")); + } + } catch (XmlPullParserException | IOException e) { + // Expected exception if the parser fails on DOCDECL + } + } } + diff --git a/google-http-client/src/main/java/com/google/api/client/http/HttpHeaders.java b/google-http-client/src/main/java/com/google/api/client/http/HttpHeaders.java index e43f09b0f..2da90980d 100644 --- a/google-http-client/src/main/java/com/google/api/client/http/HttpHeaders.java +++ b/google-http-client/src/main/java/com/google/api/client/http/HttpHeaders.java @@ -838,6 +838,9 @@ private static void addHeader( } // compute value String stringValue = toStringValue(value); + if (name.contains("\r") || name.contains("\n") || stringValue.contains("\r") || stringValue.contains("\n")) { + throw new IllegalArgumentException("Header name or value contains CRLF characters."); + } // log header String loggedStringValue = stringValue; if (("Authorization".equalsIgnoreCase(name) || "Cookie".equalsIgnoreCase(name)) diff --git a/google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java b/google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java index 78f15d868..790137b07 100644 --- a/google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java +++ b/google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java @@ -29,6 +29,7 @@ import io.opencensus.trace.Tracer; import java.io.IOException; import java.io.InputStream; +import java.net.URL; import java.util.Properties; import java.util.concurrent.Callable; import java.util.concurrent.Executor; @@ -860,6 +861,10 @@ public HttpResponse execute() throws IOException { Preconditions.checkNotNull(requestMethod); Preconditions.checkNotNull(url); + final String originalScheme = url.getScheme(); + final String originalHost = url.getHost(); + final int originalPort = url.getPort(); + Span span = tracer .spanBuilder(OpenCensusUtils.SPAN_NAME_HTTP_REQUEST_EXECUTE) @@ -879,6 +884,11 @@ public HttpResponse execute() throws IOException { if (executeInterceptor != null) { executeInterceptor.intercept(this); } + // Prevent credential leak on cross-origin redirects + if (!isSameOrigin(originalScheme, originalHost, originalPort, url.getScheme(), url.getHost(), url.getPort())) { + headers.setAuthorization((String) null); + headers.setCookie((String) null); + } // build low-level HTTP request String urlString = url.build(); addSpanAttribute(span, HttpTraceAttributeConstants.HTTP_METHOD, requestMethod); @@ -1022,10 +1032,17 @@ public HttpResponse execute() throws IOException { response = new HttpResponse(this, lowLevelHttpResponse); responseConstructed = true; } finally { - if (!responseConstructed) { - InputStream lowLevelContent = lowLevelHttpResponse.getContent(); - if (lowLevelContent != null) { - lowLevelContent.close(); + if (!responseConstructed && lowLevelHttpResponse != null) { + try { + InputStream lowLevelContent = lowLevelHttpResponse.getContent(); + if (lowLevelContent != null) { + lowLevelContent.close(); + } + } catch (IOException ignored) { + } + try { + lowLevelHttpResponse.disconnect(); + } catch (IOException ignored) { } } } @@ -1180,7 +1197,25 @@ public boolean handleRedirect(int statusCode, HttpHeaders responseHeaders) { && HttpStatusCodes.isRedirect(statusCode) && redirectLocation != null) { // resolve the redirect location relative to the current location - setUrl(new GenericUrl(url.toURL(redirectLocation), useRawRedirectUrls)); + URL newURL = url.toURL(redirectLocation); + + // Check if redirecting to a different origin + String oldScheme = url.getScheme(); + String oldHost = url.getHost(); + int oldPort = url.getPort(); + + String newScheme = newURL.getProtocol(); + String newHost = newURL.getHost(); + int newPort = newURL.getPort(); + + int oldEffectivePort = getEffectivePort(oldScheme, oldPort); + int newEffectivePort = getEffectivePort(newScheme, newPort); + + boolean sameOrigin = (oldScheme == null ? newScheme == null : oldScheme.equalsIgnoreCase(newScheme)) + && (oldHost == null ? newHost == null : oldHost.equalsIgnoreCase(newHost)) + && (oldEffectivePort == newEffectivePort); + + setUrl(new GenericUrl(newURL, useRawRedirectUrls)); // on 303 change method to GET if (statusCode == HttpStatusCodes.STATUS_CODE_SEE_OTHER) { setRequestMethod(HttpMethods.GET); @@ -1194,11 +1229,39 @@ public boolean handleRedirect(int statusCode, HttpHeaders responseHeaders) { headers.setIfModifiedSince((String) null); headers.setIfUnmodifiedSince((String) null); headers.setIfRange((String) null); + + // remove Cookie header if redirect is cross-origin + if (!sameOrigin) { + headers.setCookie((String) null); + } return true; } return false; } + private static int getEffectivePort(String scheme, int port) { + if (port != -1) { + return port; + } + if ("http".equalsIgnoreCase(scheme)) { + return 80; + } + if ("https".equalsIgnoreCase(scheme)) { + return 443; + } + return -1; + } + + private static boolean isSameOrigin( + String scheme1, String host1, int port1, + String scheme2, String host2, int port2) { + int effectivePort1 = getEffectivePort(scheme1, port1); + int effectivePort2 = getEffectivePort(scheme2, port2); + return (scheme1 == null ? scheme2 == null : scheme1.equalsIgnoreCase(scheme2)) + && (host1 == null ? host2 == null : host1.equalsIgnoreCase(host2)) + && (effectivePort1 == effectivePort2); + } + /** * Returns the sleeper. * diff --git a/google-http-client/src/test/java/com/google/api/client/http/HttpHeadersTest.java b/google-http-client/src/test/java/com/google/api/client/http/HttpHeadersTest.java index 3b9baf113..6264e8ef6 100644 --- a/google-http-client/src/test/java/com/google/api/client/http/HttpHeadersTest.java +++ b/google-http-client/src/test/java/com/google/api/client/http/HttpHeadersTest.java @@ -17,6 +17,7 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; import com.google.api.client.http.HttpRequestTest.E; import com.google.api.client.testing.http.MockLowLevelHttpRequest; @@ -322,4 +323,29 @@ public void testFromHttpResponse_void() throws Exception { assertNull(v.v); assertEquals("svalue", v.s); } + + @Test + public void testSerializeHeaders_crlfInjectionInName() throws Exception { + HttpHeaders headers = new HttpHeaders(); + headers.set("foo\r\nbar", "value"); + try { + HttpHeaders.serializeHeaders(headers, null, null, null, new MockLowLevelHttpRequest(), null); + fail("Expected IllegalArgumentException"); + } catch (IllegalArgumentException e) { + // Expected + } + } + + @Test + public void testSerializeHeaders_crlfInjectionInValue() throws Exception { + HttpHeaders headers = new HttpHeaders(); + headers.set("foo", "value\r\nbar"); + try { + HttpHeaders.serializeHeaders(headers, null, null, null, new MockLowLevelHttpRequest(), null); + fail("Expected IllegalArgumentException"); + } catch (IllegalArgumentException e) { + // Expected + } + } } + diff --git a/google-http-client/src/test/java/com/google/api/client/http/HttpRequestTest.java b/google-http-client/src/test/java/com/google/api/client/http/HttpRequestTest.java index 085b9f563..8840c26bd 100644 --- a/google-http-client/src/test/java/com/google/api/client/http/HttpRequestTest.java +++ b/google-http-client/src/test/java/com/google/api/client/http/HttpRequestTest.java @@ -543,6 +543,92 @@ public void subtestHandleRedirect_relativeLocation( assertEquals(newLocation, req.getUrl().toString()); } + @Test + public void testHandleRedirect_crossOriginCookieRemoval() throws IOException { + // 1. Same-origin redirect (http://some.org/a/b -> http://some.org/a/z) + { + HttpTransport transport = new MockHttpTransport(); + HttpRequest req = transport.createRequestFactory().buildGetRequest(new GenericUrl("http://some.org/a/b")); + req.getHeaders().setCookie("foo=bar"); + HttpHeaders responseHeaders = new HttpHeaders().setLocation("http://some.org/a/z"); + req.handleRedirect(HttpStatusCodes.STATUS_CODE_SEE_OTHER, responseHeaders); + assertEquals("foo=bar", req.getHeaders().getCookie()); + assertEquals("http://some.org/a/z", req.getUrl().toString()); + } + + // 2. Cross-origin redirect due to host change (http://some.org/a/b -> http://other.org/c) + { + HttpTransport transport = new MockHttpTransport(); + HttpRequest req = transport.createRequestFactory().buildGetRequest(new GenericUrl("http://some.org/a/b")); + req.getHeaders().setCookie("foo=bar"); + HttpHeaders responseHeaders = new HttpHeaders().setLocation("http://other.org/c"); + req.handleRedirect(HttpStatusCodes.STATUS_CODE_SEE_OTHER, responseHeaders); + assertNull(req.getHeaders().getCookie()); + assertEquals("http://other.org/c", req.getUrl().toString()); + } + + // 3. Cross-origin redirect due to scheme change (https://some.org/a/b -> http://some.org/a/z) + { + HttpTransport transport = new MockHttpTransport(); + HttpRequest req = transport.createRequestFactory().buildGetRequest(new GenericUrl("https://some.org/a/b")); + req.getHeaders().setCookie("foo=bar"); + HttpHeaders responseHeaders = new HttpHeaders().setLocation("http://some.org/a/z"); + req.handleRedirect(HttpStatusCodes.STATUS_CODE_SEE_OTHER, responseHeaders); + assertNull(req.getHeaders().getCookie()); + assertEquals("http://some.org/a/z", req.getUrl().toString()); + } + + // 4. Cross-origin redirect due to port change (http://some.org/a/b -> http://some.org:8080/a/z) + { + HttpTransport transport = new MockHttpTransport(); + HttpRequest req = transport.createRequestFactory().buildGetRequest(new GenericUrl("http://some.org/a/b")); + req.getHeaders().setCookie("foo=bar"); + HttpHeaders responseHeaders = new HttpHeaders().setLocation("http://some.org:8080/a/z"); + req.handleRedirect(HttpStatusCodes.STATUS_CODE_SEE_OTHER, responseHeaders); + assertNull(req.getHeaders().getCookie()); + assertEquals("http://some.org:8080/a/z", req.getUrl().toString()); + } + + // 5. Same-origin redirect with implicit/explicit default ports matching (http://some.org/a/b -> http://some.org:80/a/z) + { + HttpTransport transport = new MockHttpTransport(); + HttpRequest req = transport.createRequestFactory().buildGetRequest(new GenericUrl("http://some.org/a/b")); + req.getHeaders().setCookie("foo=bar"); + HttpHeaders responseHeaders = new HttpHeaders().setLocation("http://some.org:80/a/z"); + req.handleRedirect(HttpStatusCodes.STATUS_CODE_SEE_OTHER, responseHeaders); + assertEquals("foo=bar", req.getHeaders().getCookie()); + assertEquals("http://some.org:80/a/z", req.getUrl().toString()); + } + } + + @Test + public void testExecute_disconnectOnResponseConstructionFailure() throws Exception { + class FailingHttpResponse extends MockLowLevelHttpResponse { + @Override + public String getContentEncoding() { + throw new RuntimeException("Simulated response construction failure"); + } + } + + final FailingHttpResponse failingResponse = new FailingHttpResponse(); + HttpTransport transport = new MockHttpTransport() { + @Override + public LowLevelHttpRequest buildRequest(String method, String url) throws IOException { + return new MockLowLevelHttpRequest().setResponse(failingResponse); + } + }; + + HttpRequest req = transport.createRequestFactory().buildGetRequest(new GenericUrl("http://example.com")); + try { + req.execute(); + fail("Expected RuntimeException"); + } catch (RuntimeException e) { + assertEquals("Simulated response construction failure", e.getMessage()); + } + + assertTrue(failingResponse.isDisconnected()); + } + @Deprecated @Test public void testExecuteErrorWithRetryEnabled() throws Exception { @@ -1327,4 +1413,97 @@ public void testVersion_matchesAcceptablePatterns() throws Exception { String.format("the loaded version '%s' did not match the acceptable pattern", version), version.matches(acceptableVersionPattern)); } + + @Test + public void testExecute_crossOriginRedirectCredentialLeakPrevention() throws Exception { + final List recordedRequests = Lists.newArrayList(); + + HttpTransport transport = new MockHttpTransport() { + @Override + public LowLevelHttpRequest buildRequest(String method, final String url) { + MockLowLevelHttpRequest req = new MockLowLevelHttpRequest(url) { + @Override + public LowLevelHttpResponse execute() throws IOException { + recordedRequests.add(this); + MockLowLevelHttpResponse resp = new MockLowLevelHttpResponse(); + if (recordedRequests.size() == 1) { + resp.setStatusCode(302); + resp.addHeader("Location", "https://untrusted-target.com/path"); + } else { + resp.setStatusCode(200); + } + return resp; + } + }; + return req; + } + }; + + HttpRequest req = transport.createRequestFactory().buildGetRequest(new GenericUrl("https://example.com/start")); + req.setInterceptor(new BasicAuthentication("myuser", "mypass")); + req.getHeaders().setCookie("mycookie=val"); + + HttpResponse response = req.execute(); + assertEquals(200, response.getStatusCode()); + assertEquals(2, recordedRequests.size()); + + // First request (https://example.com/start) + MockLowLevelHttpRequest firstReq = recordedRequests.get(0); + assertTrue(firstReq.getUrl().contains("example.com")); + assertNotNull(firstReq.getFirstHeaderValue("Authorization")); + assertEquals("mycookie=val", firstReq.getFirstHeaderValue("Cookie")); + + // Second request (https://untrusted-target.com/path) - redirect to cross-origin + MockLowLevelHttpRequest secondReq = recordedRequests.get(1); + assertTrue(secondReq.getUrl().contains("untrusted-target.com")); + assertNull(secondReq.getFirstHeaderValue("Authorization")); + assertNull(secondReq.getFirstHeaderValue("Cookie")); + } + + @Test + public void testExecute_sameOriginRedirectCredentialLeakPrevention() throws Exception { + final List recordedRequests = Lists.newArrayList(); + + HttpTransport transport = new MockHttpTransport() { + @Override + public LowLevelHttpRequest buildRequest(String method, final String url) { + MockLowLevelHttpRequest req = new MockLowLevelHttpRequest(url) { + @Override + public LowLevelHttpResponse execute() throws IOException { + recordedRequests.add(this); + MockLowLevelHttpResponse resp = new MockLowLevelHttpResponse(); + if (recordedRequests.size() == 1) { + resp.setStatusCode(302); + resp.addHeader("Location", "https://example.com/redirect-path"); + } else { + resp.setStatusCode(200); + } + return resp; + } + }; + return req; + } + }; + + HttpRequest req = transport.createRequestFactory().buildGetRequest(new GenericUrl("https://example.com/start")); + req.setInterceptor(new BasicAuthentication("myuser", "mypass")); + req.getHeaders().setCookie("mycookie=val"); + + HttpResponse response = req.execute(); + assertEquals(200, response.getStatusCode()); + assertEquals(2, recordedRequests.size()); + + // First request + MockLowLevelHttpRequest firstReq = recordedRequests.get(0); + assertTrue(firstReq.getUrl().contains("example.com/start")); + assertNotNull(firstReq.getFirstHeaderValue("Authorization")); + assertEquals("mycookie=val", firstReq.getFirstHeaderValue("Cookie")); + + // Second request - redirect same-origin + MockLowLevelHttpRequest secondReq = recordedRequests.get(1); + assertTrue(secondReq.getUrl().contains("example.com/redirect-path")); + assertNotNull(secondReq.getFirstHeaderValue("Authorization")); + assertEquals("mycookie=val", secondReq.getFirstHeaderValue("Cookie")); + } } +