Skip to content

Commit

Permalink
fix: Harden strict CORS checking
Browse files Browse the repository at this point in the history
  • Loading branch information
rhuss committed Jul 18, 2018
1 parent 14f1158 commit ec1cebd
Show file tree
Hide file tree
Showing 17 changed files with 70 additions and 51 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -224,12 +224,13 @@ public boolean isRemoteAccessAllowed(String pRemoteHost, String pRemoteAddr) {
/**
* Check whether CORS access is allowed for the given origin.
*
* @param pOrigin origin URL which needs to be checked
* @param pStrictChecking whether to a strict check (i.e. server side check)
* @param pOrigin origin URL which needs to be checked. Can be null.
* @param pOnlyWhenStrictCheckingIsEnabled whether do the check only when strict-checking is enabled.
* If true an no strict-checking is configured then always allow.
* @return true if if cors access is allowed
*/
public boolean isOriginAllowed(String pOrigin,boolean pStrictChecking) {
return restrictor.isOriginAllowed(pOrigin, pStrictChecking);
public boolean isOriginAllowed(String pOrigin,boolean pOnlyWhenStrictCheckingIsEnabled) {
return restrictor.isOriginAllowed(pOrigin, pOnlyWhenStrictCheckingIsEnabled);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,7 @@ protected void doPost(HttpServletRequest req, HttpServletResponse resp)
protected void doOptions(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException {
Map<String,String> responseHeaders =
requestHandler.handleCorsPreflightRequest(
req.getHeader("Origin"),
getOriginOrReferer(req),
req.getHeader("Access-Control-Request-Headers"));
for (Map.Entry<String,String> entry : responseHeaders.entrySet()) {
resp.setHeader(entry.getKey(),entry.getValue());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -153,15 +153,15 @@ public JSONAware handlePostRequest(String pUri, InputStream pInputStream, String
*/
public Map<String, String> handleCorsPreflightRequest(String pOrigin, String pRequestHeaders) {
Map<String,String> ret = new HashMap<String, String>();
if (pOrigin != null && backendManager.isOriginAllowed(pOrigin,false)) {
if (backendManager.isOriginAllowed(pOrigin,false)) {
// CORS is allowed, we set exactly the origin in the header, so there are no problems with authentication
ret.put("Access-Control-Allow-Origin","null".equals(pOrigin) ? "*" : pOrigin);
ret.put("Access-Control-Allow-Origin",pOrigin == null || "null".equals(pOrigin) ? "*" : pOrigin);
if (pRequestHeaders != null) {
ret.put("Access-Control-Allow-Headers",pRequestHeaders);
}
// Fix for CORS with authentication (#104)
ret.put("Access-Control-Allow-Credentials","true");
// Allow for one year. Changes in access.xml are reflected directly in the cors request itself
// Allow for one year. Changes in access.xml are reflected directly in the CORS request itself
ret.put("Access-Control-Max-Age","" + 3600 * 24 * 365);
}
return ret;
Expand Down Expand Up @@ -283,7 +283,7 @@ public void checkAccess(String pHost, String pAddress, String pOrigin) {
if (!backendManager.isRemoteAccessAllowed(pHost, pAddress)) {
throw new SecurityException("No access from client " + pAddress + " allowed");
}
if (pOrigin != null && !backendManager.isOriginAllowed(pOrigin,true)) {
if (!backendManager.isOriginAllowed(pOrigin,true)) {
throw new SecurityException("Origin " + pOrigin + " is not allowed to call this agent");
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ public boolean isRemoteAccessAllowed(String... pHostOrAddress) {
}

/** {@inheritDoc} */
public boolean isOriginAllowed(String pOrigin, boolean pIsStrictCheck) {
public boolean isOriginAllowed(String pOrigin, boolean pOnlyWhenStrictCheckingIsEnabled) {
return isAllowed;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -120,13 +120,13 @@ public boolean isRemoteAccessAllowed(String ... pHostOrAddress) {
}

/** {@inheritDoc} */
public boolean isOriginAllowed(String pOrigin, boolean pIsStrictCheck) {
return corsChecker.check(pOrigin,pIsStrictCheck);
public boolean isOriginAllowed(String pOrigin, boolean pOnlyWhenStrictCheckingIsEnabled) {
return corsChecker.check(pOrigin, pOnlyWhenStrictCheckingIsEnabled);
}

/** {@inheritDoc} */
public boolean isAttributeReadAllowed(ObjectName pName, String pAttribute) {
return check(RequestType.READ,pName,pAttribute);
return check(RequestType.READ, pName, pAttribute);
}

/** {@inheritDoc} */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,8 +91,8 @@ public interface Restrictor {
* for details
*
* @param pOrigin the "Origin:" header provided within the request
* @param pIsStrictCheck whether doing a strict check
* @param pOnlyWhenStrictCheckingIsEnabled whether by-pass check when strict checking is disabled
* @return true if this cross browser request allowed, false otherwise
*/
boolean isOriginAllowed(String pOrigin, boolean pIsStrictCheck);
boolean isOriginAllowed(String pOrigin, boolean pOnlyWhenStrictCheckingIsEnabled);
}
Original file line number Diff line number Diff line change
Expand Up @@ -79,16 +79,23 @@ public boolean check(String pArg) {
return check(pArg,false);
}

public boolean check(String pOrigin, boolean pIsStrictCheck) {
public boolean check(String pOrigin, boolean pOnlyWhenStrictCheckingIsEnabled) {
// Method called during strict checking but we have not configured that
// So the check passes always.
if (pIsStrictCheck && !strictChecking) {
if (pOnlyWhenStrictCheckingIsEnabled && !strictChecking) {
return true;
}

if (patterns == null || patterns.size() == 0) {
// If strict checking is enabled but Origin is null, the don't allow. This can
// be the case when both Referer: and Origin: are set to null
if (pOrigin == null && strictChecking) {
return false;
}

if (patterns == null || patterns.size() == 0 || pOrigin == null) {
return true;
}

for (Pattern pattern : patterns) {
if (pattern.matcher(pOrigin).matches()) {
return true;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ public void tearDown() {
@Test
public void accessAllowed() {
expect(backend.isRemoteAccessAllowed("localhost","127.0.0.1")).andReturn(true);
expect(backend.isOriginAllowed((String) isNull(), eq(true))).andReturn(true);
replay(backend);

handler.checkAccess("localhost", "127.0.0.1",null);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,7 @@ public void cors() {
PolicyRestrictor restrictor = new PolicyRestrictor(is);

for (boolean strict : new boolean[] {true, false}) {
assertFalse(restrictor.isOriginAllowed(null, strict));
assertTrue(restrictor.isOriginAllowed("http://bla.com", strict));
assertFalse(restrictor.isOriginAllowed("http://www.jolokia.org", strict));
assertTrue(restrictor.isOriginAllowed("https://www.consol.de", strict));
Expand All @@ -238,6 +239,7 @@ public void corsStrictCheckingOff() {
PolicyRestrictor restrictor = new PolicyRestrictor(is);

// Allways true since we want a strict check but strict checking is off.
assertTrue(restrictor.isOriginAllowed(null, true));
assertTrue(restrictor.isOriginAllowed("http://bla.com", true));
assertTrue(restrictor.isOriginAllowed("http://www.jolokia.org", true));
assertTrue(restrictor.isOriginAllowed("https://www.consol.de", true));
Expand All @@ -248,6 +250,7 @@ public void corsWildCard() {
InputStream is = getClass().getResourceAsStream("/allow-origin2.xml");
PolicyRestrictor restrictor = new PolicyRestrictor(is);

assertTrue(restrictor.isOriginAllowed(null, false));
assertTrue(restrictor.isOriginAllowed("http://bla.com", false));
assertTrue(restrictor.isOriginAllowed("http://www.jolokia.org", false));
assertTrue(restrictor.isOriginAllowed("http://www.consol.de", false));
Expand All @@ -258,6 +261,7 @@ public void corsEmpty() {
InputStream is = getClass().getResourceAsStream("/allow-origin3.xml");
PolicyRestrictor restrictor = new PolicyRestrictor(is);

assertTrue(restrictor.isOriginAllowed(null, false));
assertTrue(restrictor.isOriginAllowed("http://bla.com", false));
assertTrue(restrictor.isOriginAllowed("http://www.jolokia.org", false));
assertTrue(restrictor.isOriginAllowed("http://www.consol.de", false));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ public boolean isRemoteAccessAllowed(String... pHostOrAddress) {
return false;
}

public boolean isOriginAllowed(String pOrigin, boolean pIsStrictCheck) {
public boolean isOriginAllowed(String pOrigin, boolean pOnlyWhenStrictCheckingIsEnabled) {
return false;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -293,7 +293,7 @@ private JSONAware executePostRequest(HttpExchange pExchange, ParsedUri pUri) thr
private void performCorsPreflightCheck(HttpExchange pExchange) {
Headers requestHeaders = pExchange.getRequestHeaders();
Map<String,String> respHeaders =
requestHandler.handleCorsPreflightRequest(requestHeaders.getFirst("Origin"),
requestHandler.handleCorsPreflightRequest(extractOriginOrReferer(pExchange),
requestHeaders.getFirst("Access-Control-Request-Headers"));
Headers responseHeaders = pExchange.getResponseHeaders();
for (Map.Entry<String,String> entry : respHeaders.entrySet()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ public boolean isRemoteAccessAllowed(String... pHostOrAddress) {
}

@Override
public boolean isOriginAllowed(String pOrigin, boolean pIsStrictCheck) {
public boolean isOriginAllowed(String pOrigin, boolean pOnlyWhenStrictCheckingIsEnabled) {
return res;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -171,8 +171,8 @@ public boolean check(Restrictor restrictor, Object... args) {
};

/** {@inheritDoc} */
public boolean isOriginAllowed(String pOrigin, boolean pIsStrictCheck) {
return checkRestrictorService(CORS_CHECK,pOrigin,pIsStrictCheck);
public boolean isOriginAllowed(String pOrigin, boolean pOnlyWhenStrictCheckingIsEnabled) {
return checkRestrictorService(CORS_CHECK, pOrigin, pOnlyWhenStrictCheckingIsEnabled);
}

// =======================================================================================================
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ public boolean isRemoteAccessAllowed(String... pHostOrAddress) {
return remote;
}

public boolean isOriginAllowed(String pOrigin, boolean pIsStrictCheck) {
public boolean isOriginAllowed(String pOrigin, boolean pOnlyWhenStrictCheckingIsEnabled) {
return cors;
}
}
Expand Down
2 changes: 1 addition & 1 deletion client/javascript/test-app/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@

<groupId>org.jolokia</groupId>
<artifactId>jolokia-client-javascript-test-app</artifactId>
<version>1.4.0-SNAPSHOT</version>
<version>1.6.1-SNAPSHOT</version>
<name>jolokia-client-javascript-test-app</name>
<packaging>war</packaging>
<description>Test application for Jolokia's JavaScript client library</description>
Expand Down
3 changes: 3 additions & 0 deletions src/changes/changes.xml
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,9 @@
Add configuration option threadNamePrefix which will be used by jolokia http server executor
Default: "jolokia-"
</action>
<action dev="rhuss" type="fix">
When strict-checking is enabled for a CORS policy, then reject a request also when no Origin or Referer header is given.
</action>
</release>
<release version="1.6.0" description="Release 1.6.0" date="2018-06-25">
<action dev="rhuss" type="update">
Expand Down
Loading

0 comments on commit ec1cebd

Please sign in to comment.