Remove unnecessary SecureRandom from UrlFetchUtils

We're only using it for generating multiparty boundaries, and there's no real need for the random boundary values to be cryptographically secure.  The point of the randomness is just to make collisions with content in the payload sufficiently unlikely.  The app itself controls the payload contents, and while it might be derived from user-submitted content, in practice it would be nearly infeasible to get the payload to contain arbitrary boundary values even if the RNG-produced boundaries could be determined in advance.

To further insulate against this, I've increased the boundary size (from 40 bits to 192) and added an actual check that the boundary isn't present in the input data, so that in the extremely unlikely event of a collision, we fail rather than producing an invalid multipart request.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=142784289

java/google/registry/util/UrlFetchUtils.java

@@ -14,7 +14,7 @@
 
 package google.registry.util;
 
-import static com.google.common.io.BaseEncoding.base32;
+import static com.google.common.base.Preconditions.checkState;
 import static com.google.common.io.BaseEncoding.base64;
 import static com.google.common.net.HttpHeaders.AUTHORIZATION;
 import static com.google.common.net.HttpHeaders.CONTENT_DISPOSITION;
@@ -28,16 +28,15 @@ import com.google.appengine.api.urlfetch.HTTPRequest;
 import com.google.appengine.api.urlfetch.HTTPResponse;
 import com.google.common.base.Ascii;
 import com.google.common.base.Optional;
+import com.google.common.base.Strings;
 import com.google.common.net.MediaType;
-import java.security.NoSuchAlgorithmException;
+import java.util.Random;
-import java.security.ProviderException;
-import java.security.SecureRandom;
 
 /** Helper methods for the App Engine URL fetch service. */
 public final class UrlFetchUtils {
 
   @NonFinalForTesting
-  private static SecureRandom secureRandom = initSecureRandom();
+  private static Random random = new Random();
 
   /** Returns value of first header matching {@code name}. */
   public static Optional<String> getHeaderFirst(HTTPResponse rsp, String name) {
@@ -66,9 +65,12 @@ public final class UrlFetchUtils {
    *
    * @see <a href="http://www.ietf.org/rfc/rfc2388.txt"> RFC2388 - Returning Values from Forms</a>
    */
-  public static <T> void setPayloadMultipart(
+  public static void setPayloadMultipart(
-      HTTPRequest request, String name, String filename, MediaType contentType, T data) {
+      HTTPRequest request, String name, String filename, MediaType contentType, String data) {
     String boundary = createMultipartBoundary();
+    checkState(
+        !data.contains(boundary),
+        "Multipart data contains autogenerated boundary: %s", boundary);
     StringBuilder multipart = new StringBuilder();
     multipart.append(format("--%s\r\n", boundary));
     multipart.append(format("%s: form-data; name=\"%s\"; filename=\"%s\"\r\n",
@@ -79,23 +81,19 @@ public final class UrlFetchUtils {
     multipart.append("\r\n");
     multipart.append(format("--%s--", boundary));
     byte[] payload = multipart.toString().getBytes(UTF_8);
-    request.addHeader(new HTTPHeader(CONTENT_TYPE, "multipart/form-data; boundary=" + boundary));
+    request.addHeader(
+        new HTTPHeader(CONTENT_TYPE, format("multipart/form-data; boundary=\"%s\"", boundary)));
     request.addHeader(new HTTPHeader(CONTENT_LENGTH, Integer.toString(payload.length)));
     request.setPayload(payload);
   }
 
   private static String createMultipartBoundary() {
-    byte[] rand = new byte[5];  // Avoid base32 padding since `5 * 8 % log2(32) == 0`
+    // Generate 192 random bits (24 bytes) to produce 192/log_2(64) = 192/6 = 32 base64 digits.
-    secureRandom.nextBytes(rand);
+    byte[] rand = new byte[24];
-    return "------------------------------" + base32().encode(rand);
+    random.nextBytes(rand);
-  }
+    // Boundary strings can be up to 70 characters long, so use 30 hyphens plus 32 random digits.
-
+    // See https://tools.ietf.org/html/rfc2046#section-5.1.1
-  private static SecureRandom initSecureRandom() {
+    return Strings.repeat("-", 30) + base64().encode(rand);
-    try {
-      return SecureRandom.getInstance("NativePRNG");
-    } catch (NoSuchAlgorithmException e) {
-      throw new ProviderException(e);
-    }
   }
 
   /** Sets the HTTP Basic Authentication header on an {@link HTTPRequest}. */

javatests/google/registry/util/UrlFetchUtilsTest.java

@@ -17,27 +17,30 @@ package google.registry.util;
 import static com.google.common.net.HttpHeaders.CONTENT_LENGTH;
 import static com.google.common.net.HttpHeaders.CONTENT_TYPE;
 import static com.google.common.net.MediaType.CSV_UTF_8;
+import static com.google.common.truth.Truth.assertThat;
 import static google.registry.util.UrlFetchUtils.setPayloadMultipart;
 import static java.nio.charset.StandardCharsets.UTF_8;
 import static org.mockito.Matchers.any;
-import static org.mockito.Matchers.argThat;
 import static org.mockito.Mockito.doAnswer;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.verifyNoMoreInteractions;
 
 import com.google.appengine.api.urlfetch.HTTPHeader;
 import com.google.appengine.api.urlfetch.HTTPRequest;
 import google.registry.testing.AppEngineRule;
+import google.registry.testing.ExceptionRule;
 import google.registry.testing.InjectRule;
-import java.security.SecureRandom;
 import java.util.Arrays;
+import java.util.List;
+import java.util.Random;
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
-import org.mockito.ArgumentMatcher;
+import org.mockito.ArgumentCaptor;
 import org.mockito.invocation.InvocationOnMock;
 import org.mockito.stubbing.Answer;
 
@@ -49,55 +52,57 @@ public class UrlFetchUtilsTest {
   public final AppEngineRule appEngine = AppEngineRule.builder()
       .build();
 
+  @Rule
+  public final ExceptionRule thrown = new ExceptionRule();
+
   @Rule
   public final InjectRule inject = new InjectRule();
 
   @Before
   public void setupRandomZeroes() throws Exception {
-    SecureRandom secureRandom = mock(SecureRandom.class);
+    Random random = mock(Random.class);
-    inject.setStaticField(UrlFetchUtils.class, "secureRandom", secureRandom);
+    inject.setStaticField(UrlFetchUtils.class, "random", random);
     doAnswer(new Answer<Void>() {
       @Override
       public Void answer(InvocationOnMock info) throws Throwable {
-        byte[] bytes = (byte[]) info.getArguments()[0];
+        Arrays.fill((byte[]) info.getArguments()[0], (byte) 0);
-        Arrays.fill(bytes, (byte) 0);
         return null;
-      }}).when(secureRandom).nextBytes(any(byte[].class));
+      }}).when(random).nextBytes(any(byte[].class));
   }
 
   @Test
   public void testSetPayloadMultipart() throws Exception {
-    String payload = "--------------------------------AAAAAAAA\r\n"
+    HTTPRequest request = mock(HTTPRequest.class);
+    setPayloadMultipart(
+        request, "lol", "cat", CSV_UTF_8, "The nice people at the store say hello. ヘ(◕。◕ヘ)");
+    ArgumentCaptor<HTTPHeader> headerCaptor = ArgumentCaptor.forClass(HTTPHeader.class);
+    verify(request, times(2)).addHeader(headerCaptor.capture());
+    List<HTTPHeader> addedHeaders = headerCaptor.getAllValues();
+    assertThat(addedHeaders.get(0).getName()).isEqualTo(CONTENT_TYPE);
+    assertThat(addedHeaders.get(0).getValue())
+        .isEqualTo(
+            "multipart/form-data; "
+                + "boundary=\"------------------------------AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA\"");
+    assertThat(addedHeaders.get(1).getName()).isEqualTo(CONTENT_LENGTH);
+    assertThat(addedHeaders.get(1).getValue()).isEqualTo("292");
+    String payload = "--------------------------------AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA\r\n"
         + "Content-Disposition: form-data; name=\"lol\"; filename=\"cat\"\r\n"
         + "Content-Type: text/csv; charset=utf-8\r\n"
         + "\r\n"
         + "The nice people at the store say hello. ヘ(◕。◕ヘ)\r\n"
-        + "--------------------------------AAAAAAAA--";
+        + "--------------------------------AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA--";
-    HTTPRequest request = mock(HTTPRequest.class);
-    setPayloadMultipart(
-        request, "lol", "cat", CSV_UTF_8, "The nice people at the store say hello. ヘ(◕。◕ヘ)");
-    verify(request).addHeader(argThat(new HTTPHeaderMatcher(
-        CONTENT_TYPE, "multipart/form-data; boundary=------------------------------AAAAAAAA")));
-    verify(request).addHeader(argThat(new HTTPHeaderMatcher(CONTENT_LENGTH, "244")));
     verify(request).setPayload(payload.getBytes(UTF_8));
     verifyNoMoreInteractions(request);
   }
 
-  /** Mockito matcher for {@link HTTPHeader}. */
+  @Test
-  public static class HTTPHeaderMatcher extends ArgumentMatcher<HTTPHeader> {
+  public void testSetPayloadMultipart_boundaryInPayload() throws Exception {
-    private final String name;
+    HTTPRequest request = mock(HTTPRequest.class);
-    private final String value;
+    String payload = "I screamed------------------------------AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAHHH";
-
+    thrown.expect(
-    public HTTPHeaderMatcher(String name, String value) {
+        IllegalStateException.class,
-      this.name = name;
+        "Multipart data contains autogenerated boundary: "
-      this.value = value;
+            + "------------------------------AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA");
-    }
+    setPayloadMultipart(request, "lol", "cat", CSV_UTF_8, payload);
-
-    @Override
-    public boolean matches(Object arg) {
-      HTTPHeader header = (HTTPHeader) arg;
-      return name.equals(header.getName())
-          && value.equals(header.getValue());
-    }
   }
 }