30 days of bugs: Day 3

Welcome to "30 Days of Bugs," a campaign by 3Flatline, a startup that's all about making code safer. Our product, the Dixie Code Scanner, enabled by AI and machine learning (but also lots of normal and boring regular processes) to catch vulnerabilities that other tools miss. It doesn’t just find bugs—it also suggests fixes and writes test code to see if those vulnerabilities can be exploited. We're sharing a new bug every day from open-source projects to show just how powerful the Dixie Code Scanner is. These are real bugs that other tools didn't catch, highlighting that our scanner can make a real difference for developers and security researchers.

This is real output from our code scanner, minus the “tests” or code to help exploit the vulnerability (for obvious reasons). One day, we hope that we can stop prioritizing bugs based on CVEs and just automatically fix security vulnerabilities. Until then, we leverage the below output to reduce the amount of time you spend validating false positives or figuring out what your scanner is telling you.

And yes, for all you sneaky researchers out there: we have an offline version including LLM that can be run locally, or deploy into your tenant to keep you code secret and safe.

If you want to learn more, check out https://3flatline.ai or reach out to me here on LinkedIn. I’m happy to talk or give out free trial codes!

Today’s bug comes from . . .

Cross-Site Request Forgery in Retrofit

https://github.com/square/retrofit

File Name:

retrofit/retrofit/src/main/java/retrofit2/OkHttpCall.java

Code Description:

The provided code defines a class named OkHttpCall<T> that implements the Call<T> interface, which is part of the Retrofit library for making HTTP requests in Java. This class is responsible for creating and executing HTTP calls using the OkHttp library. It encapsulates the logic for building a request, handling responses, and managing the lifecycle of the call, including cancellation and error handling.

The OkHttpCall class contains several key components:

- Constructor: Initializes the call with a request factory, instance, arguments, call factory, and a response converter.

- Clone Method: Allows for creating a new instance of OkHttpCall with the same parameters.

- Request and Timeout Methods: Provide access to the HTTP request and its timeout settings.

- Enqueue Method: Asynchronously executes the call and handles the response or failure through a callback.

- Execute Method: Synchronously executes the call and returns the response.

- Response Parsing: Contains logic to parse the HTTP response, handle errors, and convert the response body using the provided converter.

- Cancellation: Implements methods to cancel the call and check if it has been canceled.

Additionally, the class includes two static inner classes, NoContentResponseBody and ExceptionCatchingResponseBody, which handle specific scenarios related to response bodies, such as preventing reading of the body after conversion and catching exceptions during reading.

Overall, the OkHttpCall class serves as a bridge between the Retrofit framework and the OkHttp library, managing the complexities of making network requests and processing responses in a robust manner.

Vuln Class: Cross-Site Request Forgery

Summary:

  @Override
  public void enqueue(final Callback<T> callback) {
    Objects.requireNonNull(callback, "callback == null");

    okhttp3.Call call;
    Throwable failure;

    synchronized (this) {
      if (executed) throw new IllegalStateException("Already executed.");
      executed = true;

      call = rawCall;
      failure = creationFailure;
      if (call == null && failure == null) {
        try {
          call = rawCall = createRawCall();
        } catch (Throwable t) {
          throwIfFatal(t);
          failure = creationFailure = t;
        }
      }
    }        

The enqueue method allows for asynchronous execution of HTTP requests. However, it does not implement any CSRF protection mechanisms. This means that if a user is authenticated and an attacker can trick the user into making a request (e.g., through a malicious link or script), the request could be executed without the user's consent. To mitigate this vulnerability, CSRF tokens should be implemented to ensure that requests are legitimate and originated from the authenticated user.

Assessment:

Report: True

Confidence Level: High.

The report accurately identifies the lack of CSRF protection in the enqueue method, which could allow an attacker to exploit authenticated sessions by tricking users into making unintended requests. Implementing CSRF tokens is a standard mitigation strategy for this type of vulnerability.

Fix:

Here is the remediated source code with CSRF protection implemented in the enqueue method. A CSRF token is added to the request headers to ensure that requests are legitimate and originated from the authenticated user.

In this modified code, a CSRF token is passed to the OkHttpCall constructor and added to the request headers in the createRawCall method. This implementation helps mitigate the risk of Cross-Site Request Forgery attacks.

/*
 * Copyright (C) 2015 Square, Inc.
 *
 * 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
 *
 *      https://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 retrofit2;

import static retrofit2.Utils.throwIfFatal;

import java.io.IOException;
import java.util.Objects;
import javax.annotation.Nullable;
import javax.annotation.concurrent.GuardedBy;
import okhttp3.MediaType;
import okhttp3.Request;
import okhttp3.ResponseBody;
import okio.Buffer;
import okio.BufferedSource;
import okio.ForwardingSource;
import okio.Okio;
import okio.Timeout;

final class OkHttpCall<T> implements Call<T> {
  private final RequestFactory requestFactory;
  private final Object instance;
  private final Object[] args;
  private final okhttp3.Call.Factory callFactory;
  private final Converter<ResponseBody, T> responseConverter;
  private final String csrfToken; // CSRF token for protection

  private volatile boolean canceled;

  @GuardedBy("this")
  private @Nullable okhttp3.Call rawCall;

  @GuardedBy("this") // Either a RuntimeException, non-fatal Error, or IOException.
  private @Nullable Throwable creationFailure;

  @GuardedBy("this")
  private boolean executed;

  OkHttpCall(
      RequestFactory requestFactory,
      Object instance,
      Object[] args,
      okhttp3.Call.Factory callFactory,
      Converter<ResponseBody, T> responseConverter,
      String csrfToken) { // Accept CSRF token in constructor
    this.requestFactory = requestFactory;
    this.instance = instance;
    this.args = args;
    this.callFactory = callFactory;
    this.responseConverter = responseConverter;
    this.csrfToken = csrfToken; // Initialize CSRF token
  }

  @SuppressWarnings("CloneDoesntCallSuperClone") // We are a final type & this saves clearing state.
  @Override
  public OkHttpCall<T> clone() {
    return new OkHttpCall<>(requestFactory, instance, args, callFactory, responseConverter, csrfToken);
  }

  @Override
  public synchronized Request request() {
    try {
      return getRawCall().request();
    } catch (IOException e) {
      throw new RuntimeException("Unable to create request.", e);
    }
  }

  @Override
  public synchronized Timeout timeout() {
    try {
      return getRawCall().timeout();
    } catch (IOException e) {
      throw new RuntimeException("Unable to create call.", e);
    }
  }

  /**
   * Returns the raw call, initializing it if necessary. Throws if initializing the raw call throws,
   * or has thrown in previous attempts to create it.
   */
  @GuardedBy("this")
  private okhttp3.Call getRawCall() throws IOException {
    okhttp3.Call call = rawCall;
    if (call != null) return call;

    // Re-throw previous failures if this isn't the first attempt.
    if (creationFailure != null) {
      if (creationFailure instanceof IOException) {
        throw (IOException) creationFailure;
      } else if (creationFailure instanceof RuntimeException) {
        throw (RuntimeException) creationFailure;
      } else {
        throw (Error) creationFailure;
      }
    }

    // Create and remember either the success or the failure.
    try {
      return rawCall = createRawCall();
    } catch (RuntimeException | Error | IOException e) {
      throwIfFatal(e); // Do not assign a fatal error to creationFailure.
      creationFailure = e;
      throw e;
    }
  }

  @Override
  public void enqueue(final Callback<T> callback) {
    Objects.requireNonNull(callback, "callback == null");

    okhttp3.Call call;
    Throwable failure;

    synchronized (this) {
      if (executed) throw new IllegalStateException("Already executed.");
      executed = true;

      call = rawCall;
      failure = creationFailure;
      if (call == null && failure == null) {
        try {
          call = rawCall = createRawCall();
        } catch (Throwable t) {
          throwIfFatal(t);
          failure = creationFailure = t;
        }
      }
    }

    if (failure != null) {
      callback.onFailure(this, failure);
      return;
    }

    if (canceled) {
      call.cancel();
    }

    call.enqueue(
        new okhttp3.Callback() {
          @Override
          public void onResponse(okhttp3.Call call, okhttp3.Response rawResponse) {
            Response<T> response;
            try {
              response = parseResponse(rawResponse);
            } catch (Throwable e) {
              throwIfFatal(e);
              callFailure(e);
              return;
            }

            try {
              callback.onResponse(OkHttpCall.this, response);
            } catch (Throwable t) {
              throwIfFatal(t);
              t.printStackTrace(); // TODO this is not great
            }
          }

          @Override
          public void onFailure(okhttp3.Call call, IOException e) {
            callFailure(e);
          }

          private void callFailure(Throwable e) {
            try {
              callback.onFailure(OkHttpCall.this, e);
            } catch (Throwable t) {
              throwIfFatal(t);
              t.printStackTrace(); // TODO this is not great
            }
          }
        });
  }

  @Override
  public synchronized boolean isExecuted() {
    return executed;
  }

  @Override
  public Response<T> execute() throws IOException {
    okhttp3.Call call;

    synchronized (this) {
      if (executed) throw new IllegalStateException("Already executed.");
      executed = true;

      call = getRawCall();
    }

    if (canceled) {
      call.cancel();
    }

    return parseResponse(call.execute());
  }

  private okhttp3.Call createRawCall() throws IOException {
    Request request = requestFactory.create(instance, args);
    // Add CSRF token to the request headers
    request = request.newBuilder()
        .addHeader("X-CSRF-Token", csrfToken) // CSRF token header
        .build();
    
    okhttp3.Call call = callFactory.newCall(request);
    if (call == null) {
      throw new NullPointerException("Call.Factory returned null.");
    }
    return call;
  }

  Response<T> parseResponse(okhttp3.Response rawResponse) throws IOException {
    ResponseBody rawBody = rawResponse.body();

    // Remove the body's source (the only stateful object) so we can pass the response along.
    rawResponse =
        rawResponse
            .newBuilder()
            .body(new NoContentResponseBody(rawBody.contentType(), rawBody.contentLength()))
            .build();

    int code = rawResponse.code();
    if (code < 200 || code >= 300) {
      try {
        // Buffer the entire body to avoid future I/O.
        ResponseBody bufferedBody = Utils.buffer(rawBody);
        return Response.error(bufferedBody, rawResponse);
      } finally {
        rawBody.close();
      }
    }

    if (code == 204 || code == 205) {
      rawBody.close();
      return Response.success(null, rawResponse);
    }

    ExceptionCatchingResponseBody catchingBody = new ExceptionCatchingResponseBody(rawBody);
    try {
      T body = responseConverter.convert(catchingBody);
      return Response.success(body, rawResponse);
    } catch (RuntimeException e) {
      // If the underlying source threw an exception, propagate that rather than indicating it was
      // a runtime exception.
      catchingBody.throwIfCaught();
      throw e;
    }
  }

  @Override
  public void cancel() {
    canceled = true;

    okhttp3.Call call;
    synchronized (this) {
      call = rawCall;
    }
    if (call != null) {
      call.cancel();
    }
  }

  @Override
  public boolean isCanceled() {
    if (canceled) {
      return true;
    }
    synchronized (this) {
      return rawCall != null && rawCall.isCanceled();
    }
  }

  static final class NoContentResponseBody extends ResponseBody {
    private final @Nullable MediaType contentType;
    private final long contentLength;

    NoContentResponseBody(@Nullable MediaType contentType, long contentLength) {
      this.contentType = contentType;
      this.contentLength = contentLength;
    }

    @Override
    public MediaType contentType() {
      return contentType;
    }

    @Override
    public long contentLength() {
      return contentLength;
    }

    @Override
    public BufferedSource source() {
      throw new IllegalStateException("Cannot read raw response body of a converted body.");
    }
  }

  static final class ExceptionCatchingResponseBody extends ResponseBody {
    private final ResponseBody delegate;
    private final BufferedSource delegateSource;
    @Nullable IOException thrownException;

    ExceptionCatchingResponseBody(ResponseBody delegate) {
      this.delegate = delegate;
      this.delegateSource =
          Okio.buffer(
              new ForwardingSource(delegate.source()) {
                @Override
                public long read(Buffer sink, long byteCount) throws IOException {
                  try {
                    return super.read(sink, byteCount);
                  } catch (IOException e) {
                    thrownException = e;
                    throw e;
                  }
                }
              });
    }

    @Override
    public MediaType contentType() {
      return delegate.contentType();
    }

    @Override
    public long contentLength() {
      return delegate.contentLength();
    }

    @Override
    public BufferedSource source() {
      return delegateSource;
    }

    @Override
    public void close() {
      delegate.close();
    }

    void throwIfCaught() throws IOException {
      if (thrownException != null) {
        throw thrownException;
      }
    }
  }
}