diff --git a/riskified-sample/src/main/java/com/riskified/samples/notificationServer/SampleAuthToken.java b/riskified-sample/src/main/java/com/riskified/samples/notificationServer/SampleAuthToken.java new file mode 100644 index 00000000..a4f59f0e --- /dev/null +++ b/riskified-sample/src/main/java/com/riskified/samples/notificationServer/SampleAuthToken.java @@ -0,0 +1,35 @@ +package com.riskified.samples.notificationServer; + +/** + * Reads the Riskified auth token for the notification samples from the environment. + * + * The samples used to carry the token as a source literal. A sample is published in a public + * repository, so a literal there is a disclosed credential - and anyone holding the token can + * forge a notification that passes HMAC verification. Supply it at run time instead: + * + * 
+ *   export RISKIFIED_AUTH_TOKEN='<the auth token from Settings in the Riskified web app>'
+ *
+ */ +public final class SampleAuthToken { + + private static final String ENV_VAR = "RISKIFIED_AUTH_TOKEN"; + + private SampleAuthToken() { + } + + /** + * @return the auth token from the RISKIFIED_AUTH_TOKEN environment variable + * @throws IllegalStateException if the variable is unset or empty, rather than starting a + * server that would reject every notification it receives + */ + public static String fromEnvironment() { + String token = System.getenv(ENV_VAR); + if (token == null || token.trim().isEmpty()) { + throw new IllegalStateException( + ENV_VAR + " is not set. Export your Riskified auth token (Settings tab in the " + + "Riskified web app) before starting this sample."); + } + return token; + } +} diff --git a/riskified-sample/src/main/java/com/riskified/samples/notificationServer/servlet/SampleServer.java b/riskified-sample/src/main/java/com/riskified/samples/notificationServer/servlet/SampleServer.java index 261ce898..b617e88e 100644 --- a/riskified-sample/src/main/java/com/riskified/samples/notificationServer/servlet/SampleServer.java +++ b/riskified-sample/src/main/java/com/riskified/samples/notificationServer/servlet/SampleServer.java @@ -1,5 +1,6 @@ package com.riskified.samples.notificationServer.servlet; +import com.riskified.samples.notificationServer.SampleAuthToken; import org.eclipse.jetty.server.Server; import org.eclipse.jetty.servlet.ServletContextHandler; import org.eclipse.jetty.servlet.ServletHolder; @@ -13,7 +14,7 @@ public static void main(String[] args) throws Exception { myContext.setContextPath("/"); server.setHandler(myContext); - myContext.addServlet(new ServletHolder(new NotificationServlet("636b3045e083eddf4ea9f0b7f2ed4a26")), "/*"); + myContext.addServlet(new ServletHolder(new NotificationServlet(SampleAuthToken.fromEnvironment())), "/*"); server.start(); server.join(); diff --git a/riskified-sample/src/main/java/com/riskified/samples/notificationServer/socket/HTTPPOSTServer.java b/riskified-sample/src/main/java/com/riskified/samples/notificationServer/socket/HTTPPOSTServer.java index 2d39b122..6d4d3208 100644 --- a/riskified-sample/src/main/java/com/riskified/samples/notificationServer/socket/HTTPPOSTServer.java +++ b/riskified-sample/src/main/java/com/riskified/samples/notificationServer/socket/HTTPPOSTServer.java @@ -16,9 +16,11 @@ public class HTTPPOSTServer extends Thread { Socket connectedClient = null; BufferedReader inFromClient = null; BufferedWriter outToClient = null; + private final String authKey; - public HTTPPOSTServer(Socket client) { + public HTTPPOSTServer(Socket client, String authKey) { connectedClient = client; + this.authKey = authKey; } public void run() { @@ -61,7 +63,7 @@ public void run() { break; } } - NotificationHandler formatter = new NotificationHandler("26faa0eb6eacf889e300944c297640b68789b11c"); + NotificationHandler formatter = new NotificationHandler(authKey); NotificationOrder notification = formatter.toObject(body, hash).getOrder(); sendResponse(200, "Merchant Received Notification For Order " + notification.getId() + " with status " + notification.getStatus() + " and description " + notification.getDescription() diff --git a/riskified-sample/src/main/java/com/riskified/samples/notificationServer/socket/Server.java b/riskified-sample/src/main/java/com/riskified/samples/notificationServer/socket/Server.java index d2907be5..80f818e5 100644 --- a/riskified-sample/src/main/java/com/riskified/samples/notificationServer/socket/Server.java +++ b/riskified-sample/src/main/java/com/riskified/samples/notificationServer/socket/Server.java @@ -1,5 +1,7 @@ package com.riskified.samples.notificationServer.socket; +import com.riskified.samples.notificationServer.SampleAuthToken; + import java.net.InetAddress; import java.net.ServerSocket; import java.net.Socket; @@ -8,12 +10,16 @@ public class Server { public static void main(String[] args) throws Exception { + // Resolved once, up front, so a misconfigured environment fails at startup rather than + // on the first notification - and so the token is not re-read per connection. + String authKey = SampleAuthToken.fromEnvironment(); + ServerSocket server = new ServerSocket(5000, 10, InetAddress.getByName("127.0.0.1")); System.out.println("HTTP Server Waiting for client on port 5000"); while (true) { Socket connected = server.accept(); - (new HTTPPOSTServer(connected)).start(); + (new HTTPPOSTServer(connected, authKey)).start(); } } } diff --git a/riskified-sdk/src/main/java/com/riskified/notifications/NotificationHandler.java b/riskified-sdk/src/main/java/com/riskified/notifications/NotificationHandler.java index d0482117..4d7d917e 100644 --- a/riskified-sdk/src/main/java/com/riskified/notifications/NotificationHandler.java +++ b/riskified-sdk/src/main/java/com/riskified/notifications/NotificationHandler.java @@ -1,6 +1,7 @@ package com.riskified.notifications; import java.io.*; +import java.nio.charset.StandardCharsets; import java.security.MessageDigest; import javax.servlet.http.HttpServletRequest; @@ -30,16 +31,25 @@ public NotificationHandler(String authKey) throws RiskifiedError { /** * Convert string to notification object * @param data the string to convert - * @param hash the sha256 of the string + * @param hash the sha256 of the string, as received on the X-Riskified-Hmac-Sha256 header * @return Notification - * @throws AuthError the hash doesn't match the was calced sha256 for the string + * @throws AuthError the hash is absent, or doesn't match the sha256 calculated for the string * @throws UnsupportedEncodingException unsupported encoding exception * @throws IllegalStateException illegal state exception * @throws JsonSyntaxException json syntax exception */ public Notification toObject(String data, String hash) throws AuthError, JsonSyntaxException, IllegalStateException, UnsupportedEncodingException { + if (hash == null) { + // An unsigned request is unauthorized, not a programming error. Without this guard + // hash.getBytes() throws NullPointerException, which is not AuthError and so escapes + // every caller that catches the documented exception. Fails closed either way; the + // type is what matters. Matches the .NET reference (Utils/HttpUtils.cs). + throw new AuthError(); + } String calcHash = sha256Handler.createSHA256(data.getBytes("UTF-8")); - if (MessageDigest.isEqual(hash.getBytes(), calcHash.getBytes())) + // Both operands are hex ASCII, so pin the charset rather than inheriting whatever the + // platform default happens to be on the deployment host. + if (MessageDigest.isEqual(hash.getBytes(StandardCharsets.US_ASCII), calcHash.getBytes(StandardCharsets.US_ASCII))) return gson.fromJson(data, Notification.class); else throw new AuthError(); diff --git a/riskified-sdk/src/test/java/com/riskified/notifications/NotificationHandlerTest.java b/riskified-sdk/src/test/java/com/riskified/notifications/NotificationHandlerTest.java new file mode 100644 index 00000000..d0a230d2 --- /dev/null +++ b/riskified-sdk/src/test/java/com/riskified/notifications/NotificationHandlerTest.java @@ -0,0 +1,100 @@ +package com.riskified.notifications; + +import com.riskified.SHA256Handler; +import org.junit.Test; + +import java.nio.charset.StandardCharsets; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.fail; + +/** + * Regression tests for {@link NotificationHandler} HMAC verification. + * + * SECURITY-10974. The sibling PHP SDK disclosed the server-computed HMAC in its authorization + * failure, which let an unauthenticated caller learn a valid signature for a payload of its + * choosing. This SDK never had that defect, and these tests are what keeps it that way. + */ +public class NotificationHandlerTest { + + private static final String AUTH_KEY = "notification-test-secret"; + private static final String BODY = + "{\"order\":{\"id\":\"ord-1\",\"status\":\"approved\",\"old_status\":\"pending\"}}"; + + private String validHmacFor(String body) throws Exception { + return new SHA256Handler(AUTH_KEY).createSHA256(body.getBytes(StandardCharsets.UTF_8)); + } + + @Test + public void correctlySignedNotificationIsParsed() throws Exception { + NotificationHandler handler = new NotificationHandler(AUTH_KEY); + + Notification notification = handler.toObject(BODY, validHmacFor(BODY)); + + assertEquals("ord-1", notification.getOrder().getId()); + assertEquals("approved", notification.getOrder().getStatus()); + } + + @Test(expected = AuthError.class) + public void wrongHmacIsRejected() throws Exception { + new NotificationHandler(AUTH_KEY).toObject(BODY, "not-the-right-hmac"); + } + + @Test(expected = AuthError.class) + public void hmacForADifferentBodyIsRejected() throws Exception { + String otherBody = "{\"order\":{\"id\":\"ord-2\",\"status\":\"declined\"}}"; + + new NotificationHandler(AUTH_KEY).toObject(BODY, validHmacFor(otherBody)); + } + + @Test(expected = AuthError.class) + public void hmacFromADifferentAuthKeyIsRejected() throws Exception { + String foreignHmac = new SHA256Handler("someone-elses-secret") + .createSHA256(BODY.getBytes(StandardCharsets.UTF_8)); + + new NotificationHandler(AUTH_KEY).toObject(BODY, foreignHmac); + } + + /** + * A request that carries no X-Riskified-Hmac-Sha256 header arrives here as a null hash. + * It must fail as AuthError - the documented exception - and not as NullPointerException, + * which would escape every caller that catches only AuthError. + */ + @Test + public void missingHmacThrowsAuthErrorNotNullPointerException() throws Exception { + try { + new NotificationHandler(AUTH_KEY).toObject(BODY, null); + fail("Expected AuthError for a notification with no HMAC header"); + } catch (AuthError expected) { + assertEquals("Request HMAC signature was either missing or incorrect", expected.getMessage()); + } catch (NullPointerException e) { + fail("Missing HMAC header must raise AuthError, not NullPointerException"); + } + } + + @Test(expected = AuthError.class) + public void emptyHmacIsRejected() throws Exception { + new NotificationHandler(AUTH_KEY).toObject(BODY, ""); + } + + /** + * The failure must not tell the caller what the correct signature was, nor echo the body + * back at it. That disclosure is the whole of SECURITY-10974. + */ + @Test + public void authErrorDisclosesNeitherHmacNorBody() throws Exception { + String computed = validHmacFor(BODY); + + try { + new NotificationHandler(AUTH_KEY).toObject(BODY, "not-the-right-hmac"); + fail("Expected AuthError"); + } catch (AuthError e) { + String message = String.valueOf(e.getMessage()); + assertFalse("computed HMAC must not appear in the error", message.contains(computed)); + assertFalse("received HMAC must not appear in the error", message.contains("not-the-right-hmac")); + assertFalse("request body must not appear in the error", message.contains(BODY)); + assertFalse("order id must not appear in the error", message.contains("ord-1")); + } + } +}