Skip to content

Commit

Permalink
Fix for KEYCLOAK-18914 (keycloak#9355)
Browse files Browse the repository at this point in the history
Closed keycloak#9382 

Co-authored-by: Hans-Christian Halfbrodt <hc-github42@halfbrodt.org>
  • Loading branch information
hc42 and Hans-Christian Halfbrodt authored Jan 6, 2022
1 parent 1cebd79 commit d9d77fe
Show file tree
Hide file tree
Showing 12 changed files with 240 additions and 1,164 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import org.keycloak.adapters.spi.AuthOutcome;
import org.keycloak.adapters.spi.HttpFacade;
import org.keycloak.common.VerificationException;
import org.keycloak.common.util.Base64;
import org.keycloak.common.util.KeycloakUriBuilder;
import org.keycloak.common.util.MultivaluedHashMap;
import org.keycloak.dom.saml.v2.assertion.AssertionType;
Expand All @@ -57,7 +58,6 @@
import org.keycloak.saml.common.constants.JBossSAMLURIConstants;
import org.keycloak.saml.common.exceptions.ConfigurationException;
import org.keycloak.saml.common.exceptions.ProcessingException;
import org.keycloak.saml.common.util.Base64;
import org.keycloak.saml.common.util.DocumentUtil;
import org.keycloak.saml.processing.api.saml.v2.sig.SAML2Signature;
import org.keycloak.saml.processing.core.saml.v2.common.SAMLDocumentHolder;
Expand Down
107 changes: 50 additions & 57 deletions common/src/main/java/org/keycloak/common/util/Base64.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package org.keycloak.common.util;

import java.io.IOException;

/**
* <p>Encodes and decodes to and from Base64 notation.</p>
* <p>Homepage: <a href="http://iharder.net/base64">http://iharder.net/base64</a>.</p>
Expand Down Expand Up @@ -35,6 +37,10 @@
* Change Log:
* </p>
* <ul>
* <li>v2.3.8 - Fixed automatic gzip decoding, based on the content,
* as this may lead to unexpected behaviour. Request either gzipped
* or non gzipped decoding as excepted. Automatic encoding is especially
* problematic with generated input (see KEYCLOAK-18914 for a detailed case).</li>
* <li>v2.3.7 - Fixed subtle bug when base 64 input stream contained the
* value 01111111, which is an invalid base 64 character but should not
* throw an ArrayIndexOutOfBoundsException either. Led to discovery of
Expand Down Expand Up @@ -76,7 +82,7 @@
* <a href="http://www.faqs.org/rfcs/rfc3548.html">RFC3548</a>.</li>
* <li><em>Throws exceptions instead of returning null values.</em> Because some operations
* (especially those that may permit the GZIP option) use IO streams, there
* is a possiblity of an java.io.IOException being thrown. After some discussion and
* is a possibility of an java.io.IOException being thrown. After some discussion and
* thought, I've changed the behavior of the methods to throw java.io.IOExceptions
* rather than return null if ever there's an error. I think this is more
* appropriate, though it will require some changes to your code. Sorry,
Expand Down Expand Up @@ -167,9 +173,8 @@ public class Base64
/** Specify that data should be gzip-compressed in second bit. Value is two. */
public final static int GZIP = 2;

/** Specify that gzipped data should <em>not</em> be automatically gunzipped. */
public final static int DONT_GUNZIP = 4;

/** Specify that data should be gunzipped. */
public final static int GUNZIP = 4;

/** Do break lines when encoding. Value is 8. */
public final static int DO_BREAK_LINES = 8;
Expand All @@ -179,7 +184,7 @@ public class Base64
* in Section 4 of RFC3548:
* <a href="http://www.faqs.org/rfcs/rfc3548.html">http://www.faqs.org/rfcs/rfc3548.html</a>.
* It is important to note that data encoded this way is <em>not</em> officially valid Base64,
* or at the very least should not be called Base64 without also specifying that is
* or at the very least should not be called Base64 without also specifying that it
* was encoded using the URL- and Filename-safe dialect.
*/
public final static int URL_SAFE = 16;
Expand Down Expand Up @@ -476,7 +481,7 @@ private static byte[] encode3to4( byte[] b4, byte[] threeBytes, int numSigBytes,
* anywhere along their length by specifying
* <var>srcOffset</var> and <var>destOffset</var>.
* This method does not check to make sure your arrays
* are large enough to accomodate <var>srcOffset</var> + 3 for
* are large enough to accommodate <var>srcOffset</var> + 3 for
* the <var>source</var> array or <var>destOffset</var> + 4 for
* the <var>destination</var> array.
* The actual number of significant bytes in your array is
Expand Down Expand Up @@ -548,7 +553,7 @@ private static byte[] encode3to4(
* writing it to the <code>encoded</code> ByteBuffer.
* This is an experimental feature. Currently it does not
* pass along any options (such as {@link #DO_BREAK_LINES}
* or {@link #GZIP}.
* or {@link #GZIP}).
*
* @param raw input buffer
* @param encoded output buffer
Expand Down Expand Up @@ -733,7 +738,7 @@ public static String encodeBytes( byte[] source ) {
* Example options:<pre>
* GZIP: gzip-compresses object before encoding it.
* DO_BREAK_LINES: break lines at 76 characters
* <i>Note: Technically, this makes your encoding non-compliant.</i>
* <i>Note: Technically, without line break your encoding may become non-compliant (see rfc2045 and rfc4648).</i>
* </pre>
* <p>
* Example: <code>encodeBytes( myData, Base64.GZIP )</code> or
Expand Down Expand Up @@ -1115,15 +1120,8 @@ else if( source[ srcOffset + 3 ] == EQUALS_SIGN ) {
* @return decoded data
* @since 2.3.1
*/
public static byte[] decode( byte[] source )
throws java.io.IOException {
byte[] decoded = null;
// try {
decoded = decode( source, 0, source.length, Base64.NO_OPTIONS );
// } catch( java.io.IOException ex ) {
// assert false : "IOExceptions only come from GZipping, which is turned off: " + ex.getMessage();
// }
return decoded;
public static byte[] decode( byte[] source ) throws java.io.IOException {
return decode( source, 0, source.length, Base64.NO_OPTIONS );
}


Expand Down Expand Up @@ -1231,9 +1229,9 @@ public static byte[] decode( String s ) throws java.io.IOException {
* detecting gzip-compressed data and decompressing it.
*
* @param s the string to decode
* @param options encode options such as URL_SAFE
* @param options decode options such as URL_SAFE or GUNZIP
* @return the decoded data
* @throws java.io.IOException if there is an error
* @throws java.io.IOException if there is an error (invalid character in source string or gunzip error)
* @throws NullPointerException if <tt>s</tt> is null
* @since 1.4
*/
Expand All @@ -1257,46 +1255,41 @@ public static byte[] decode( String s, int options ) throws java.io.IOException

// Check to see if it's gzip-compressed
// GZIP Magic Two-Byte Number: 0x8b1f (35615)
boolean dontGunzip = (options & DONT_GUNZIP) != 0;
if( (bytes != null) && (bytes.length >= 4) && (!dontGunzip) ) {
boolean doGunzip = (options & GUNZIP) != 0;
if( (bytes != null) && (bytes.length >= 4) && doGunzip ) {

int head = ((int)bytes[0] & 0xff) | ((bytes[1] << 8) & 0xff00);
if( java.util.zip.GZIPInputStream.GZIP_MAGIC == head ) {
java.io.ByteArrayInputStream bais = null;
java.util.zip.GZIPInputStream gzis = null;
java.io.ByteArrayOutputStream baos = null;
byte[] buffer = new byte[2048];
int length = 0;

try {
baos = new java.io.ByteArrayOutputStream();
bais = new java.io.ByteArrayInputStream( bytes );
gzis = new java.util.zip.GZIPInputStream( bais );

while( ( length = gzis.read( buffer ) ) >= 0 ) {
baos.write(buffer,0,length);
} // end while: reading input

// No error? Get new bytes.
bytes = baos.toByteArray();

} // end try
catch( java.io.IOException e ) {
if (e.getMessage().equals("Unsupported compression method")) {
System.out.println("Base64 decoding: Ignoring GZIP header and just returning originally-decoded bytes."); // Better to log as debug, but jboss logging not available in the module :/
} else {
e.printStackTrace();
}

// Just return originally-decoded bytes
} // end catch
finally {
try{ baos.close(); } catch( Exception e ){}
try{ gzis.close(); } catch( Exception e ){}
try{ bais.close(); } catch( Exception e ){}
} // end finally

} // end if: gzipped
if( java.util.zip.GZIPInputStream.GZIP_MAGIC != head ) {
throw new IOException("Provided data has no GZIP magic header.");
}
java.io.ByteArrayInputStream bais = null;
java.util.zip.GZIPInputStream gzis = null;
java.io.ByteArrayOutputStream baos = null;
byte[] buffer = new byte[2048];
int length = 0;

try {
baos = new java.io.ByteArrayOutputStream();
bais = new java.io.ByteArrayInputStream( bytes );
gzis = new java.util.zip.GZIPInputStream( bais );

while( ( length = gzis.read( buffer ) ) >= 0 ) {
baos.write(buffer,0,length);
} // end while: reading input

// No error? Get new bytes.
bytes = baos.toByteArray();

} // end try
catch( java.io.IOException e ) {
throw new IOException("Failed to gunzip", e);
} // end catch
finally {
try{ baos.close(); } catch( Exception e ){}
try{ gzis.close(); } catch( Exception e ){}
try{ bais.close(); } catch( Exception e ){}
} // end finally

} // end if: bytes.length >= 2

return bytes;
Expand Down
3 changes: 1 addition & 2 deletions common/src/main/java/org/keycloak/common/util/Base64Url.java
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,7 @@ public static String encode(byte[] bytes) {
public static byte[] decode(String s) {
s = encodeBase64UrlToBase64(s);
try {
// KEYCLOAK-2479 : Avoid to try gzip decoding as for some objects, it may display exception to STDERR. And we know that object wasn't encoded as GZIP
return Base64.decode(s, Base64.DONT_GUNZIP);
return Base64.decode(s);
} catch (Exception e) {
throw new RuntimeException(e);
}
Expand Down
135 changes: 135 additions & 0 deletions common/src/test/java/org/keycloak/common/util/Base64DecodeTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,135 @@
package org.keycloak.common.util;

import org.hamcrest.MatcherAssert;
import org.junit.Test;

import java.io.IOException;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.instanceOf;

/**
* Test for BASE64 decode implementation.
*
* @author <a href="mailto:keycloak-hc@halfbrodt.org">Hans-Christian Halfbrodt</a>
*/
public class Base64DecodeTest {

@Test
public void decode_simple() throws IOException {
// high level string variant
final String testData = "test data";
final String encoded = "dGVzdCBkYXRh";
final String decoded = new String(Base64.decode(encoded));
assertThat(decoded, equalTo(testData));

// low level byte array variant
final byte[] encoded2 = encoded.getBytes();
final String decoded2 = new String(Base64.decode(encoded2));
assertThat(decoded2, equalTo(testData));
}

@Test
public void decode_doNotUseGzip() throws IOException {
// Input has gzip magic byte by coincidence and should not be gunzipped. (KEYCLOAK-18914)
// high level string variant
final byte[] testData = new byte[]{
31, -117, 8, -56, 1, 1, 1, 1, 1, 1, 43, 73, 45, 46,
81, 72, 73, 44, 73, 4, 1, -78, -82, 8, -45, 9, 1, 1, 1};
final String encoded = "H4sIyAEBAQEBAStJLS5RSEksSQQBsq4I0wkBAQE=";
final byte[] decoded = Base64.decode(encoded);
assertThat(decoded, equalTo(testData));

// low level byte array variant
final byte[] encoded2 = encoded.getBytes();
final byte[] decoded2 = Base64.decode(encoded2);
assertThat(decoded2, equalTo(testData));
}

@Test
public void decode_gzip() throws IOException {
// high level string variant
final String testData = "test data";
final String encoded = "H4sIAAAAAAAAACtJLS5RSEksSQQAsq4I0wkAAAA=";
final String decoded = new String(Base64.decode(encoded, Base64.GUNZIP));
assertThat(decoded, equalTo(testData));

// low level byte array variant
// specified to ignore gunzip option (see javadoc)
final byte[] expected2 = new byte[]{
31, -117, 8, 0, 0, 0, 0, 0, 0, 0, 43, 73,
45, 46, 81, 72, 73, 44, 73, 4, 0, -78, -82,
8, -45, 9, 0, 0, 0};
final byte[] encoded2 = encoded.getBytes();
final byte[] decoded2 = Base64.decode(encoded2, 0, encoded2.length, Base64.GUNZIP);
assertThat(decoded2, equalTo(expected2));
}

@Test
public void decode_empty() throws IOException {
final byte[] result = Base64.decode("");
assertThat(result, equalTo(new byte[0]));
final byte[] result2 = Base64.decode(new byte[0]);
assertThat(result2, equalTo(new byte[0]));

try {
Base64.decode(" ");
MatcherAssert.assertThat("Exception excepted", false);
} catch (final Exception e) {
assertThat(e, instanceOf(IllegalArgumentException.class));
}

try {
Base64.decode(" ".getBytes());
MatcherAssert.assertThat("Exception excepted", false);
} catch (final Exception e) {
assertThat(e, instanceOf(IllegalArgumentException.class));
}

try {
Base64.decode((String) null);
MatcherAssert.assertThat("Exception excepted", false);
} catch (final Exception e) {
assertThat(e, instanceOf(NullPointerException.class));
}

try {
Base64.decode((byte[]) null);
MatcherAssert.assertThat("Exception excepted", false);
} catch (final Exception e) {
assertThat(e, instanceOf(NullPointerException.class));
}
}

@Test
public void decode_lowLevelInvalidParams() {
try {
Base64.decode(null, 0, 1, Base64.NO_OPTIONS);
MatcherAssert.assertThat("Exception excepted", false);
} catch (final Exception e){
assertThat(e, instanceOf(NullPointerException.class));
}

try {
Base64.decode(new byte[2], 0, 1, Base64.NO_OPTIONS);
MatcherAssert.assertThat("Exception excepted", false);
} catch (final Exception e){
assertThat(e, instanceOf(IllegalArgumentException.class));
}

try {
Base64.decode(new byte[8], 0, 10, Base64.NO_OPTIONS);
MatcherAssert.assertThat("Exception excepted", false);
} catch (final Exception e){
assertThat(e, instanceOf(IllegalArgumentException.class));
}

try {
Base64.decode(new byte[8], 5, 5, Base64.NO_OPTIONS);
MatcherAssert.assertThat("Exception excepted", false);
} catch (final Exception e){
assertThat(e, instanceOf(IllegalArgumentException.class));
}
}
}
Loading

0 comments on commit d9d77fe

Please sign in to comment.