SignedToken: Use URL-safe encoding
If Gerrit's email verification token contains a '+' character, the
verification will fail.
To avoid this problem, modify the token generation and check methods
to use URL-safe encoding.
Bug: Issue 12424
Change-Id: Ib81f1440b8ee9a3b2249d34d7c979bdc13da9597
diff --git a/java/com/google/gerrit/server/mail/CheckTokenException.java b/java/com/google/gerrit/server/mail/CheckTokenException.java
new file mode 100644
index 0000000..bc6105a
--- /dev/null
+++ b/java/com/google/gerrit/server/mail/CheckTokenException.java
@@ -0,0 +1,28 @@
+// Copyright 2020 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.mail;
+
+/** Indicates the SignedToken is invalid */
+public class CheckTokenException extends Exception {
+ private static final long serialVersionUID = 1L;
+
+ CheckTokenException(final String message) {
+ super(message);
+ }
+
+ CheckTokenException(final String message, final Throwable why) {
+ super(message, why);
+ }
+}
diff --git a/java/com/google/gerrit/server/mail/SignedToken.java b/java/com/google/gerrit/server/mail/SignedToken.java
index 436b854..a010e30 100644
--- a/java/com/google/gerrit/server/mail/SignedToken.java
+++ b/java/com/google/gerrit/server/mail/SignedToken.java
@@ -14,6 +14,7 @@
package com.google.gerrit.server.mail;
+import com.google.common.io.BaseEncoding;
import java.security.InvalidKeyException;
import java.security.NoSuchAlgorithmException;
import java.security.SecureRandom;
@@ -21,7 +22,6 @@
import javax.crypto.Mac;
import javax.crypto.ShortBufferException;
import javax.crypto.spec.SecretKeySpec;
-import org.apache.commons.codec.binary.Base64;
/**
* Utility function to compute and verify XSRF tokens.
@@ -88,24 +88,29 @@
}
/**
- * Validate a returned token.
+ * Validate a returned token. If the token is valid then return a {@link ValidToken}, else will
+ * throw {@link XsrfException} when it's an unexpected token overflow or {@link
+ * CheckTokenException} when it's an illegal token string format.
*
* @param tokenString a token string previously created by this class.
* @param text text that must have been used during {@link #newToken(String)} in order for the
* token to be valid. If null the text will be taken from the token string itself.
- * @return true if the token is valid; false if the token is null, the empty string, has expired,
- * does not match the text supplied, or is a forged token.
+ * @return the token which is valid.
* @throws XsrfException the JVM doesn't support the necessary algorithms to generate a token.
* XSRF services are simply not available.
+ * @throws CheckTokenException throws when token is null, the empty string, has expired, does not
+ * match the text supplied, or is a forged token.
*/
- ValidToken checkToken(final String tokenString, final String text) throws XsrfException {
+ public ValidToken checkToken(final String tokenString, final String text)
+ throws XsrfException, CheckTokenException {
+
if (tokenString == null || tokenString.length() == 0) {
- return null;
+ throw new CheckTokenException("Empty token");
}
final int s = tokenString.indexOf('$');
if (s <= 0) {
- return null;
+ throw new CheckTokenException("Token does not contain character '$'");
}
final String recvText = tokenString.substring(s + 1);
@@ -113,24 +118,25 @@
try {
in = decodeBase64(tokenString.substring(0, s));
} catch (RuntimeException e) {
- return null;
+ throw new CheckTokenException("Base64 decoding failed", e);
}
+
if (in.length != tokenLength) {
- return null;
+ throw new CheckTokenException("Token length mismatch");
}
final int q = decodeInt(in, 0);
final int c = decodeInt(in, INT_SZ) ^ q;
final int n = now();
if (maxAge > 0 && Math.abs(c - n) > maxAge) {
- return null;
+ throw new CheckTokenException("Token is expired");
}
final byte[] gen = new byte[tokenLength];
System.arraycopy(in, 0, gen, 0, 2 * INT_SZ);
computeToken(gen, text != null ? text : recvText);
if (!Arrays.equals(gen, in)) {
- return null;
+ throw new CheckTokenException("Token text mismatch");
}
return new ValidToken(maxAge > 0 && c + (maxAge >> 1) <= n, recvText);
@@ -164,11 +170,11 @@
}
private static byte[] decodeBase64(final String s) {
- return Base64.decodeBase64(toBytes(s));
+ return BaseEncoding.base64Url().decode(s);
}
private static String encodeBase64(final byte[] buf) {
- return toString(Base64.encodeBase64(buf));
+ return BaseEncoding.base64Url().encode(buf);
}
private static void encodeInt(final byte[] buf, final int o, final int v) {
@@ -202,12 +208,4 @@
}
return r;
}
-
- private static String toString(final byte[] b) {
- final StringBuilder r = new StringBuilder(b.length);
- for (int i = 0; i < b.length; i++) {
- r.append((char) b[i]);
- }
- return r.toString();
- }
}
diff --git a/java/com/google/gerrit/server/mail/SignedTokenEmailTokenVerifier.java b/java/com/google/gerrit/server/mail/SignedTokenEmailTokenVerifier.java
index af492f1..897a5f2 100644
--- a/java/com/google/gerrit/server/mail/SignedTokenEmailTokenVerifier.java
+++ b/java/com/google/gerrit/server/mail/SignedTokenEmailTokenVerifier.java
@@ -17,6 +17,7 @@
import static com.google.common.base.Preconditions.checkState;
import static java.nio.charset.StandardCharsets.UTF_8;
+import com.google.common.io.BaseEncoding;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.server.config.AuthConfig;
import com.google.gerrit.server.mail.send.RegisterNewEmailSender;
@@ -25,7 +26,6 @@
import com.google.inject.Singleton;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
-import org.eclipse.jgit.util.Base64;
/** Verifies the token sent by {@link RegisterNewEmailSender}. */
@Singleton
@@ -50,7 +50,7 @@
try {
String payload = String.format("%s:%s", accountId, emailAddress);
byte[] utf8 = payload.getBytes(UTF_8);
- String base64 = Base64.encodeBytes(utf8);
+ String base64 = BaseEncoding.base64Url().encode(utf8);
return emailRegistrationToken.newToken(base64);
} catch (XsrfException e) {
throw new IllegalArgumentException(e);
@@ -63,14 +63,14 @@
ValidToken token;
try {
token = emailRegistrationToken.checkToken(tokenString, null);
- } catch (XsrfException err) {
+ } catch (XsrfException | CheckTokenException err) {
throw new InvalidTokenException(err);
}
if (token == null || token.getData() == null || token.getData().isEmpty()) {
throw new InvalidTokenException();
}
- String payload = new String(Base64.decode(token.getData()), UTF_8);
+ String payload = new String(BaseEncoding.base64Url().decode(token.getData()), UTF_8);
Matcher matcher = Pattern.compile("^([0-9]+):(.+@.+)$").matcher(payload);
if (!matcher.matches()) {
throw new InvalidTokenException();
diff --git a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java
index 9d64770..7bb7e37 100644
--- a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java
@@ -1120,7 +1120,7 @@
@Test
@GerritConfig(
name = "auth.registerEmailPrivateKey",
- value = "HsOc6l+2lhS9G7sE/RsnS7Z6GJjdRDX14co=")
+ value = "HsOc6l_2lhS9G7sE_RsnS7Z6GJjdRDX14co=")
public void addEmailSendsConfirmationEmail() throws Exception {
String email = "new.email@example.com";
EmailInput input = newEmailInput(email, false);
@@ -1134,7 +1134,7 @@
@Test
@GerritConfig(
name = "auth.registerEmailPrivateKey",
- value = "HsOc6l+2lhS9G7sE/RsnS7Z6GJjdRDX14co=")
+ value = "HsOc6l_2lhS9G7sE-RsnS7Z6GJjdRDX14co=")
public void addEmailToBeConfirmedToOwnAccount() throws Exception {
TestAccount user = accountCreator.create();
requestScopeOperations.setApiUser(user.id());
@@ -1158,7 +1158,7 @@
@Test
@GerritConfig(
name = "auth.registerEmailPrivateKey",
- value = "HsOc6l+2lhS9G7sE/RsnS7Z6GJjdRDX14co=")
+ value = "HsOc6l_2lhS9G7sE-RsnS7Z6GJjdRDX14co=")
public void addEmailToBeConfirmedToOtherAccount() throws Exception {
TestAccount user = accountCreator.create();
String email = "me@example.com";
diff --git a/javatests/com/google/gerrit/acceptance/server/mail/SignedTokenEmailTokenVerifierIT.java b/javatests/com/google/gerrit/acceptance/server/mail/SignedTokenEmailTokenVerifierIT.java
new file mode 100644
index 0000000..c24b1a0
--- /dev/null
+++ b/javatests/com/google/gerrit/acceptance/server/mail/SignedTokenEmailTokenVerifierIT.java
@@ -0,0 +1,125 @@
+// Copyright (C) 2020 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.acceptance.server.mail;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.testing.GerritJUnit.assertThrows;
+
+import com.google.common.io.BaseEncoding;
+import com.google.gerrit.acceptance.AbstractDaemonTest;
+import com.google.gerrit.server.mail.EmailTokenVerifier;
+import com.google.gerrit.server.mail.EmailTokenVerifier.InvalidTokenException;
+import com.google.gerrit.server.mail.SignedToken;
+import com.google.gerrit.server.mail.SignedTokenEmailTokenVerifier;
+import com.google.gerrit.testing.ConfigSuite;
+import java.nio.charset.StandardCharsets;
+import org.eclipse.jgit.lib.Config;
+import org.junit.Before;
+import org.junit.Test;
+
+public class SignedTokenEmailTokenVerifierIT extends AbstractDaemonTest {
+ @ConfigSuite.Default
+ public static Config defaultConfig() {
+ Config cfg = new Config();
+ cfg.setString("auth", null, "registerEmailPrivateKey", SignedToken.generateRandomKey());
+ return cfg;
+ }
+
+ private SignedTokenEmailTokenVerifier signedTokenEmailTokenVerifier;
+
+ @Before
+ public void setUp() throws Exception {
+ signedTokenEmailTokenVerifier =
+ server
+ .getTestInjector()
+ .getBinding(SignedTokenEmailTokenVerifier.class)
+ .getProvider()
+ .get();
+ }
+
+ /** Test encode */
+ @Test
+ public void encode() throws Exception {
+ String tokenString = signedTokenEmailTokenVerifier.encode(user.id(), user.email());
+ int index = tokenString.indexOf("$");
+ String text = tokenString.substring(index + 1);
+ String textDecoded = new String(BaseEncoding.base64Url().decode(text), StandardCharsets.UTF_8);
+ int pos = textDecoded.indexOf(":");
+ assertThat(textDecoded.substring(0, pos)).isEqualTo(user.id().toString());
+ assertThat(textDecoded.substring(pos + 1)).isEqualTo(user.email());
+ }
+
+ /** Test decode */
+ @Test
+ public void decode() throws Exception {
+ String tokenString = signedTokenEmailTokenVerifier.encode(user.id(), user.email());
+ String tokenKey = tokenString.substring(0, tokenString.indexOf("$"));
+ String text = user.id() + ":" + user.email();
+ String invalidTokenString =
+ tokenKey + "$" + BaseEncoding.base64Url().encode(text.getBytes(StandardCharsets.UTF_8));
+ EmailTokenVerifier.ParsedToken parsedToken =
+ signedTokenEmailTokenVerifier.decode(invalidTokenString);
+ assertThat(parsedToken.getAccountId()).isEqualTo(user.id());
+ assertThat(parsedToken.getEmailAddress()).isEqualTo(user.email());
+ }
+
+ /** Test token format is wrong(without '$' to split key and text) */
+ @Test
+ public void invalidFormat() throws Exception {
+ InvalidTokenException thrown =
+ assertThrows(
+ InvalidTokenException.class,
+ () -> signedTokenEmailTokenVerifier.decode("Invalid token"));
+ assertThat(thrown)
+ .hasCauseThat()
+ .hasMessageThat()
+ .isEqualTo("Token does not contain character '$'");
+ }
+
+ /** Test input token string is empty or null */
+ @Test
+ public void emptyInput() throws Exception {
+ InvalidTokenException thrownWithNull =
+ assertThrows(InvalidTokenException.class, () -> signedTokenEmailTokenVerifier.decode(null));
+ InvalidTokenException thrownWithEmpty =
+ assertThrows(InvalidTokenException.class, () -> signedTokenEmailTokenVerifier.decode(""));
+ assertThat(thrownWithNull).hasCauseThat().hasMessageThat().isEqualTo("Empty token");
+ assertThat(thrownWithEmpty).hasCauseThat().hasMessageThat().isEqualTo("Empty token");
+ }
+
+ /** Test token format is right but key is an illegal BASE64 string */
+ @Test
+ public void illegalTokenKey() throws Exception {
+ InvalidTokenException thrown =
+ assertThrows(
+ InvalidTokenException.class,
+ () -> signedTokenEmailTokenVerifier.decode("Illegal token key$...."));
+ assertThat(thrown).hasCauseThat().hasMessageThat().isEqualTo("Base64 decoding failed");
+ }
+
+ /** Test token text not match the required pattern */
+ @Test
+ public void tokenTextPatternMismatch() throws Exception {
+ String tokenString = signedTokenEmailTokenVerifier.encode(user.id(), user.email());
+ String tokenKey = tokenString.substring(0, tokenString.indexOf("$"));
+ String pattern = user.id() + ":" + user.email();
+ String invalidTokenTextPattern = tokenKey + "$" + pattern.replace(":", "");
+ InvalidTokenException thrown =
+ assertThrows(
+ InvalidTokenException.class,
+ () -> signedTokenEmailTokenVerifier.decode(invalidTokenTextPattern));
+ assertThat(thrown).hasCauseThat().hasMessageThat().isEqualTo("Token text mismatch");
+ }
+}
diff --git a/javatests/com/google/gerrit/server/mail/SignedTokenTest.java b/javatests/com/google/gerrit/server/mail/SignedTokenTest.java
new file mode 100644
index 0000000..cfb551f
--- /dev/null
+++ b/javatests/com/google/gerrit/server/mail/SignedTokenTest.java
@@ -0,0 +1,172 @@
+// Copyright (C) 2020 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.mail;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.testing.GerritJUnit.assertThrows;
+
+import java.util.Random;
+import org.junit.Before;
+import org.junit.Test;
+
+public class SignedTokenTest {
+
+ private static final String REGISTER_EMAIL_PRIVATE_KEY =
+ "R2Vycml0JTIwcmVnaXN0ZXJFbWFpbFByaXZhdGVLZXk=";
+ private static final String URL_SAFE_REGISTER_EMAIL_PRIVATE_KEY =
+ REGISTER_EMAIL_PRIVATE_KEY.replaceFirst("R2", "_-");
+ private static final String URL_UNSAFE_REGISTER_EMAIL_PRIVATE_KEY_WITH_PLUS =
+ REGISTER_EMAIL_PRIVATE_KEY.replaceFirst("R", "+");
+ private static final String URL_UNSAFE_REGISTER_EMAIL_PRIVATE_KEY_WITH_SLASH =
+ REGISTER_EMAIL_PRIVATE_KEY.replaceFirst("R", "/");
+
+ private static final int maxAge = 5;
+ private static final String TEXT = "This is a text";
+ private static final String FORGED_TEXT = "This is a forged text";
+ private static final String FORGED_TOKEN = String.format("Zm9yZ2VkJTIwa2V5$%s", TEXT);
+
+ private SignedToken signedToken;
+
+ @Before
+ public void setUp() throws Exception {
+ signedToken = new SignedToken(maxAge, REGISTER_EMAIL_PRIVATE_KEY);
+ }
+
+ /**
+ * Test new token: the key is a normal BASE64 string without index of '62'(+ or _) or '63'(/ or -)
+ */
+ @Test
+ public void newTokenKeyDoesNotContainUnsafeChar() throws Exception {
+ new SignedToken(maxAge, REGISTER_EMAIL_PRIVATE_KEY);
+ }
+
+ /** Test new token: the key is an URL safe BASE64 string with indexes of '62'(_) and '63'(-) */
+ @Test
+ public void newTokenWithUrlSafeBase64() throws Exception {
+ new SignedToken(maxAge, URL_SAFE_REGISTER_EMAIL_PRIVATE_KEY);
+ }
+
+ /** Test new token: the key is an URL unsafe BASE64 string with index of '62'(+) */
+ @Test
+ public void newTokenWithUrlUnsafeBase64Plus() throws Exception {
+ IllegalArgumentException thrown =
+ assertThrows(
+ IllegalArgumentException.class,
+ () -> new SignedToken(maxAge, URL_UNSAFE_REGISTER_EMAIL_PRIVATE_KEY_WITH_PLUS));
+
+ assertThat(thrown)
+ .hasMessageThat()
+ .isEqualTo(
+ "com.google.common.io.BaseEncoding$DecodingException: Unrecognized character: +");
+ }
+
+ /** Test new token: the key is an URL unsafe BASE64 string with '63'(/) */
+ @Test
+ public void newTokenWithUrlUnsafeBase64Slash() throws Exception {
+ IllegalArgumentException thrown =
+ assertThrows(
+ IllegalArgumentException.class,
+ () -> new SignedToken(maxAge, URL_UNSAFE_REGISTER_EMAIL_PRIVATE_KEY_WITH_SLASH));
+
+ assertThat(thrown)
+ .hasMessageThat()
+ .isEqualTo(
+ "com.google.common.io.BaseEncoding$DecodingException: Unrecognized character: /");
+ }
+
+ /** Test check token: BASE64 encoding and decoding in a safe URL way */
+ @Test
+ public void checkToken() throws Exception {
+ String token = signedToken.newToken(TEXT);
+ ValidToken validToken = signedToken.checkToken(token, TEXT);
+ assertThat(validToken).isNotNull();
+ assertThat(validToken.getData()).isEqualTo(TEXT);
+ }
+
+ /** Test check token: input token string is null */
+ @Test
+ public void checkTokenInputTokenNull() throws Exception {
+ CheckTokenException thrown =
+ assertThrows(CheckTokenException.class, () -> signedToken.checkToken(null, TEXT));
+
+ assertThat(thrown).hasMessageThat().isEqualTo("Empty token");
+ }
+
+ /** Test check token: input token string is empty */
+ @Test
+ public void checkTokenInputTokenEmpty() throws Exception {
+ CheckTokenException thrown =
+ assertThrows(CheckTokenException.class, () -> signedToken.checkToken("", TEXT));
+
+ assertThat(thrown).hasMessageThat().isEqualTo("Empty token");
+ }
+
+ /** Test check token: token string is not illegal with no '$' character */
+ @Test
+ public void checkTokenInputTokenNoDollarSplitChar() throws Exception {
+ String token = signedToken.newToken(TEXT).replace("$", "¥");
+ CheckTokenException thrown =
+ assertThrows(CheckTokenException.class, () -> signedToken.checkToken(token, TEXT));
+
+ assertThat(thrown).hasMessageThat().isEqualTo("Token does not contain character '$'");
+ }
+
+ /** Test check token: token string length is match but is not a legal BASE64 string */
+ @Test
+ public void checkTokenInputTokenKeyBase64DecodeFail() throws Exception {
+ String token = signedToken.newToken(TEXT);
+ String key = randomString(token.indexOf("$") + 1);
+ String illegalBase64Token = key + "$" + TEXT;
+ CheckTokenException thrown =
+ assertThrows(
+ CheckTokenException.class, () -> signedToken.checkToken(illegalBase64Token, TEXT));
+
+ assertThat(thrown).hasMessageThat().isEqualTo("Base64 decoding failed");
+ }
+
+ /** Test check token: token is illegal with a forged key */
+ @Test
+ public void checkTokenForgedKey() throws Exception {
+ CheckTokenException thrown =
+ assertThrows(CheckTokenException.class, () -> signedToken.checkToken(FORGED_TOKEN, TEXT));
+
+ assertThat(thrown).hasMessageThat().isEqualTo("Token length mismatch");
+ }
+
+ /** Test check token: token is illegal with a forged text */
+ @Test
+ public void checkTokenForgedText() throws Exception {
+ CheckTokenException thrown =
+ assertThrows(
+ CheckTokenException.class,
+ () -> {
+ String token = signedToken.newToken(TEXT);
+ signedToken.checkToken(token, FORGED_TEXT);
+ });
+
+ assertThat(thrown).hasMessageThat().isEqualTo("Token text mismatch");
+ }
+
+ private static String randomString(int length) {
+ String str = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789";
+ Random random = new Random();
+ StringBuffer sb = new StringBuffer();
+ for (int i = 0; i < length; i++) {
+ int number = random.nextInt(62);
+ sb.append(str.charAt(number));
+ }
+ return sb.toString();
+ }
+}
