Exception for bad header can leak an Authorization secret

[eli-darkly]

If there's an illegal character in a header value, an IllegalArgumentException is thrown whose message includes the full header value. This is particularly bad if the header name is Authorization or Proxy-Authorization, which strongly suggests that the value is sensitive information.

Just because it contains a non-printable character that can't possibly work in HTTP doesn't mean it doesn't also contain sensitive information: consider a scenario where an application reads a credential from a configuration file or an environment variable, but fails to trim a newline that was accidentally included by whatever logic provided the value. It's the same reason why one wouldn't want to print an "invalid password" message that included the password in cleartext: because the invalid one might be just a slight mistyping of the valid one.

java.lang.IllegalArgumentException: Unexpected char 0x0a at 20 in Authorization value: very-secret-password\n

This is especially risky since it is an unchecked exception. It's common in Java for applications to have a fallback catch block to intercept any unchecked exceptions that have propagated out of lower-level code, and since those presumably represent some kind of unexpected condition that wasn't accounted for, a typical response is to log them.

I would strongly suggest that these exceptions never include the header value, but only the name (and maybe, as you're also doing now, what the illegal character was); that (plus the stacktrace) is likely to be enough to tell the developer where the defect is. But at the very least I would suppress the value if the header is Authorization or Proxy-Authorization. I realize you're not literally logging the value yourself, but an unchecked exception is like a flare gun— one has to assume that it might be visible to someone other than the person you're having a conversation with.

To reproduce:

package com.launchdarkly.eventsource;

import okhttp3.*;
import org.junit.Test;
import static org.hamcrest.MatcherAssert.*;
import static org.hamcrest.Matchers.*;

public class OkhttpHeaderExceptionTest {
  @Test
  public void invalidHeaderValueIsCapturedInException() throws Exception {
    String password = "very-secret-password";
    String badValue = password + "\n";
    
    try {
      Request req = new Request.Builder().url("http://github.com/path/doesnt/matter")
          .header("Authorization", badValue)
          .build();
    } catch (IllegalArgumentException e) {
      assertThat(e.getMessage(), not(containsString(password)));
    }
  }
}

[eli-darkly]

By the way, in case this seems like a contrived example, I had a customer run into exactly this scenario, involving a credential with a non-trimmed newline which they were dismayed to see showing up in their logs. OkHttp is not by any means the only HTTP client that has this problematic behavior: the standard HTTP client for Go does the same thing, which caused an equivalent issue for someone using the Go version of our library; that should've been our cue to check that there wasn't a similar issue in Java, but unfortunately we didn't.

[eli-darkly]

Also— even though it was pointed out in discussion of #2016 that the HTTP protocol does not actually forbid bytes >= 0x80 (such as UTF-8 multi-byte characters) in header values, it looks to me like OkHttp still considers those invalid. That would mean it is actually possible for OkHttp to throw an IllegalArgumentException for an Authorization value that is, as far as the server is concerned, a totally valid credential which would have been possible to send via HTTP— if the credential happened to include a non-ASCII printable character. So this is not only an issue in cases where the application accidentally sent garbage.

[eli-darkly]

Ah... I see that this has been fixed on the main-line branch, but I don't think the fix exists in any released version, since it was made in February and there have been no releases since then. Even if the upcoming 5.0.0 doesn't have this problem, I hope you will consider releasing a patch for existing versions. (I am still a little concerned about the multi-byte character problem I mentioned in the last comment, but that's a side issue.)

[yschimke]

We only maintain 3.12.x and 4.9.x, so I'll backport there once confirmed. Thanks for flagging this!

[yschimke]

For 4.9.x #6740

For 3.12.x it might be worthwhile discussing whether it's above the security bar since it will be a reimplementation (probably of a smaller change) instead of a cherry-pick.

[marcdejonge]

I'd like to get this fixed, but I don't see a new release since this has been merged. Any news on releasing a 4.9.2?

[jphelp32]

Second that. Any update on publishing the fix?

[jphelp32]

@yschimke @swankjesse Can you guys comment on whether or not we can expect a 4.9.x release with the above security vulnerability fixed? looks like it's been merged, but just not published?

[seamusbdonohue]

Any update on publishing the fix ?

[craigimanage]

Also awaiting this fix for this vuln. please

[swankjesse]

4.9.2 is out. Thank you for your patience.

[yschimke]

Thanks @swankjesse, I know you get hit with a lot of releases across a lot of projects and maintenance releases are out of the normal flow.

[kookee1226]

Give me a day to get everything back in motion

…

On Fri, Oct 1, 2021, 1:42 AM Yuri Schimke ***@***.***> wrote: Thanks @swankjesse <https://github.com/swankjesse>, I know you get hit with a lot of releases across a lot of projects and maintenance releases are out of the normal flow. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#6738 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ATDQXOGSMVUKT3P53LDP3HTUEVC57ANCNFSM47XBZCPQ> .

[meggers]

Any thoughts on backporting this to 3.x?

[swankjesse]

@meggers no plans. Please upgrade to 4.x!