OWASP AntiSamy Project XSS

From the OWASP AntiSamy Project page’s “What is it” section:

It’s an API that helps you make sure that clients don’t supply malicious cargo code in the HTML they supply for their profile, comments, etc., that get persisted on the server. The term “malicious code” in regards to web applications usually mean “JavaScript.” Cascading Stylesheets are only considered malicious when they invoke the JavaScript engine. However, there are many situations where “normal” HTML and CSS can be used in a malicious manner. So we take care of that too.

So as far as I understand it, it is trying to prevent Cross Site Scripting (XSS). But to be fair, the user guide is a little bit more realistic:

AntiSamy does a very good job of removing malicious HTML, CSS and JavaScript, but in security, no solution is guaranteed. AntiSamy’s success depends on the strictness of your policy file and the predictability of browsers’ behavior. AntiSamy is a sanitization framework; it is up to the user how it does its sanitization. Using AntiSamy does not guarantee filtering of all malicious code. AntiSamy simply does what is described by the policy file.

Anyway, I found a XSS that worked in the case of the web application I was testing:

<a href="http://example.com"&amp;/onclick=alert(8)>foo</a>

Version: antisamy-1.5.2.jar with all default configuration files there are.

Browsers tested (all working): Firefox 22.0 (on Mac OSX), Safari 6.0.5 (on Mac OSX), Internet Explorer 11.0.9200 (Windows 7) and Android Browser (Android 2.2).

Disclosure timeline:
July 16th, 2013: Wrote to Arshan (maintainer) about the issue
July 16th, 2013: Response, questions about version and browser compatiblity
July 16th, 2013: Clarification about versions/browser and that I only tried getNumberOfErrors(), informed that I’m planning to release this blog post end of August
July 23rd, 2013: Some more E-Mails about similar issue that was just resolved (not the same issue), including Kristian who comitted a fix
July 25th, 2013: Kristian sent a mail, he will have a look at the getNumberOfErrors() logic before releasing an update
July 31st, 2013: Asked if there are any updates on the issue, no response
Sept 09th, 2013: Asked if there are any updates on the issue, response that it should be fixed. Requested new .jar file
Oct 21st, 2013: Tested with the newest version available for download, antisamy 1.5.3. Problem still present. Public release.

Antisamy doesn’t give any error messages and getNumberOfErrors() is 0. Although the getCleanHTML() will give back sanitised code without the XSS, people relying only on the getNumberOfErrors() method to check if an input is valid or not will have a XSS.

Btw. the included configuration file names are somehow misleading (the names include company names like ebay). Those names are made up, doesn’t mean the companies use those conifg files at all. I don’t even know if they are using the antisamy project at all.

I can’t recommend relying on that project. Proper output encoding is important and is the real XSS prevention. And validating HTML with regex is hard. Very hard. Very, very hard. Don’t.

A Weird Idea – Java, Strings and Memory

Disclaimer: I’m not a Java internals specialist. Let me know if I got a point wrong here.

Wiping sensitive information when you don’t need it anymore is a very basic security rule. It makes sense to everyone when we talk about hard discs, but the same rule should be applied to memory (RAM). While memory management is quite straight forward in C derived languages (eg. allocate some memory, override it with random bytes when the sensitive information is not needed anymore), it’s quite hard to do it in Java. Is it even possible? Java Strings are immutable, that means you can not change the memory allocated for a String. The Java VM will make a copy of the String and change the copy. So from this perspective, Strings are not a good way to store sensitive information. So let’s do it in char arrays I thought and wrote the following Java class:

/**
* ----------------------------------------------------------------------------
* "THE BEER-WARE LICENSE" (Revision 42):
* <floyd at floyd dot ch> wrote this file. As long as you retain this notice you
* can do whatever you want with this stuff. If we meet some day, and you think
* this stuff is worth it, you can buy me a beer in return
* floyd http://floyd.ch @floyd_ch <floyd at floyd dot ch>
* August 2012
* ----------------------------------------------------------------------------
**/

import java.util.Arrays;
import java.io.IOException;
import java.io.Serializable;
import java.lang.Exception;

/**
 * ATTENTION: This method is only ASCII safe, not Unicode.
 * It should be used to store sensitive information which are ASCII
 * 
 * This class keeps track of the passed in char[] and wipes all the old arrays
 * whenever they are changed. This means:
 * 
 * char[] r1 = {0x41, 0x41, 0x41, 0x41, 0x41, 0x41};
 * SecureString k = new SecureString(r1);
 * k.destroy();
 * System.out.println(r1); //Attention! r1 will be {0x00, 0x00, 0x00, 0x00, 0x00, 0x00}
 * 
 * char[] r2 = {0x41, 0x41, 0x41, 0x41, 0x41, 0x41};
 * SecureString k = new SecureString(r2);
 * k.append('C');
 * System.out.println(r2); //Attention! r2 will be {0x00, 0x00, 0x00, 0x00, 0x00, 0x00}
 * System.out.println(k.getByteArray()); //correct
 * 
 * General rule for this class:
 * - Never call toString() of this class. It will just return null:
 * char[] r3 = {0x41, 0x41, 0x41, 0x41, 0x41, 0x41};
 * SecureString k = new SecureString(r3);
 * System.out.println(k); //will print null, because it calls toString()
 * 
 * - Choose the place to do the destroy() wisely. It should be done when an operation
 * is finished and the string is not needed anymore.
 */

public class SecureString implements Serializable{
	private static final long serialVersionUID = 1L;
	private char[] charRepresentation;
	private char[] charRepresentationLower;
	private byte[] byteRepresentation;
	private boolean isDestroyed = false;
	private int length=0;
	private final static int OBFUSCATOR_BYTE = 0x55;
	
	/**
	 * After calling the constructor you should NOT wipe the char[]
	 * you just passed in (the stringAsChars reference)
	 * @param stringAsChars
	 */
	public SecureString(char[] stringAsChars){
		//Pointing to the same address as original char[]
		this.length = stringAsChars.length;
		this.initialise(stringAsChars);
	}
	
	/**
	 * After calling the constructor you should NOT wipe the byte[]
	 * you just passed in (the stringAsBytes reference)
	 * @param stringAsBytes
	 */
	public SecureString(byte[] stringAsBytes){
		//Pointing to the same address as original byte[]
		this.length = stringAsBytes.length;
		this.initialise(stringAsBytes);
	}
	
	/**
	 * @return length of the string
	 */
	public int length(){
		return this.length;
	}
	
	/**
	 * Initialising entire object based on a char array
	 * @param stringAsChars
	 */
	private void initialise(char[] stringAsChars){
		this.length = stringAsChars.length;
		charRepresentation = stringAsChars; 
		charRepresentationLower = new char[stringAsChars.length];
		byteRepresentation = new byte[stringAsChars.length];
		for(int i=0; i<stringAsChars.length; i++){
			charRepresentationLower[i] = Character.toLowerCase(charRepresentation[i]);
			byteRepresentation[i] = (byte)charRepresentation[i];
		}
	}
	
	/**
	 * Initializing entire object based on a byte array
	 * @param stringAsBytes
	 */
	private void initialise(byte[] stringAsBytes){
		this.length = stringAsBytes.length;
		byteRepresentation = stringAsBytes;
		charRepresentation = new char[stringAsBytes.length];
		charRepresentationLower = new char[stringAsBytes.length];
		for(int i=0; i<stringAsBytes.length; i++){
			charRepresentation[i] = (char) byteRepresentation[i];
			charRepresentationLower[i] = Character.toLowerCase(charRepresentation[i]);
		}
	}
	
	/**
	 * We first obfuscate the string by XORing with OBFUSCATOR_BYTE
	 * So it doesn't get serialized as plain text. THIS METHOD
	 * WILL DESTROY THIS OBJECT AT THE END.
	 * @param out
	 * @throws IOException
	 */
	private void writeObject(java.io.ObjectOutputStream out) throws IOException{		
		out.writeInt(byteRepresentation.length);
		for(int i = 0; i < byteRepresentation.length; i++ ){
			out.write((byte)(byteRepresentation[i]) ^ (byte)(OBFUSCATOR_BYTE));
		}
		//TODO: Test if the Android Intent is really only calling writeObject once,
		//otherwise this could be a problem. Then I suggest that the SecureString
		//is not passed in an intent (and never serialized)
		destroy();
	}
	
	/**
	 * We first have to deobfuscate the string by XORing with OBFUSCATOR_BYTE
	 * @param out
	 * @throws IOException
	 */
	private void readObject(java.io.ObjectInputStream in) throws IOException, ClassNotFoundException{
		//we first have to deobfuscate
		int newLength = in.readInt();
		byteRepresentation = new byte[newLength];
		for(int i = 0; i < newLength; i++ ){
			byteRepresentation[i] = (byte) ((byte)(in.read()) ^ (byte)(OBFUSCATOR_BYTE));
		}
		initialise(byteRepresentation);
	}
	
	/**
	 * Use this method wisely
	 * 
	 * When the destroy() method of this SecureString is called, the byte[] 
	 * this method had returned will also be wiped!
	 * 
	 * Never WRITE directly to this byte[], but only READ
	 * 
	 * @return reference to byte[] of this SecureString
	 */
	public byte[] getByteArray(){
		if(this.isDestroyed)
			return null;
		return byteRepresentation;
	}
	
	/**
	 * This method should never be called.
	 */
	@Deprecated
	@Override
	public String toString(){
		return null;
	}
	
	/**
	 * This method should never be called, because it means you already stored the
	 * sensitive information in a String.
	 * @param string
	 * @return
	 * @throws Exception
	 */
	
	@Deprecated
	public boolean equals(String string) throws Exception{
		throw new Exception("YOU ARE NOT SUPPOSED TO CALL equals(String string) of SecureString, "+
				"because your are not supposed to have the other value in a string in the first place!");
	}
	/**
	 * This method should never be called, because it means you already stored the
	 * sensitive information in a String.
	 * @param string
	 * @return
	 * @throws Exception
	 */
	
	@Deprecated
	public boolean equalsIgnoreCase(String string) throws Exception{
		throw new Exception("YOU ARE NOT SUPPOSED TO CALL equalsIgnoreCase(String string) of SecureString, "+
				"because your are not supposed to have the other value in a string in the first place!");
	}
	/**
	 * Comparing if the two strings stored in the SecureString objects are equal
	 * @param secureString2
	 * @return true if strings are equal
	 */
	public boolean equals(SecureString secureString2){
		if(this.isDestroyed)
			return false;
		return Arrays.equals(this.charRepresentation, secureString2.charRepresentation);
	}
	
	/**
	 * Comparing if the two strings stored in the SecureString objects are equalsIgnoreCase
	 * @param secureString2
	 * @return true if strings are equal ignoring case
	 */
	public boolean equalsIgnoreCase(SecureString secureString2){
		if(this.isDestroyed)
			return false;
		return Arrays.equals(this.charRepresentationLower, secureString2.charRepresentationLower);
	}
	
	/**
	 * Delete a char at the given position
	 * @param index
	 */
	public void deleteCharAt(int index){
		if(this.isDestroyed)
			return;
		if(this.length == 0 || index >= length)
			return;
		wipe(charRepresentationLower);
		wipe(byteRepresentation);
		char[] newCharRepresentation = new char[length-1];
		for(int i=0; i<index; i++)
			newCharRepresentation[i] = charRepresentation[i];
		for(int i=index+1; i<this.length-1; i++)
			newCharRepresentation[i-1] = charRepresentation[i];
		wipe(charRepresentation);
		initialise(newCharRepresentation);
	}
	
	/**
	 * Append a char at the end of the string
	 * @param c
	 */
	public void append(char c){
		wipe(charRepresentationLower);
		wipe(byteRepresentation);
		char[] newCharRepresentation = new char[length+1];
		for(int i=0; i<this.length; i++)
			newCharRepresentation[i] = charRepresentation[i];
		newCharRepresentation[length-1] = c;
		wipe(charRepresentation);
		initialise(newCharRepresentation);
	}
	
	/**
	 * Internal method to wipe a char[]
	 * @param chars
	 */
	private void wipe(char[] chars){
		for(int i=0; i<chars.length; i++){
			//set to hex AA
			int val = 0xAA;
			chars[i] = (char) val; 
			//set to hex 55
			val = 0x55;
			chars[i] = (char) val; 
			//set to hex 00
			val = 0x00;
			chars[i] = (char) val; 
		}
	}
	
	/**
	 * Internal method to wipe a byte[]
	 * @param chars
	 */
	private void wipe(byte[] bytes){
		for(int i=0; i<bytes.length; i++){
			//set to hex AA
			int val = 0xAA;
			bytes[i] = (byte) val; 
			//set to hex 55
			val = 0x55;
			bytes[i] = (byte) val; 
			//set to hex 00
			val = 0x00;
			bytes[i] = (byte) val;
		}
	}
	
	/**
	 * Safely wipes all the data
	 */
	public void destroy(){
		if(this.isDestroyed)
			return;
		wipe(charRepresentation);
		wipe(charRepresentationLower);
		wipe(byteRepresentation);
		
		//loose references
		charRepresentation = null; 
		charRepresentationLower = null; 
		byteRepresentation = null; 
		
		this.length = 0;
		
		//TODO: call garbage collector?
		//Runtime.getRuntime().gc();
		
		this.isDestroyed = true;		
	}
	
	/**
	 * This method is only to check if non-sensitive data
	 * is part of the SecureString
	 * @return true if SecureString contains the given String
	 */
	public boolean contains(String nonSensitiveData){
		if(this.isDestroyed)
			return false;
		if(nonSensitiveData.length() > this.length)
			return false;
		char[] nonSensitiveDataChars = nonSensitiveData.toCharArray();
		int positionInNonSensitiveData = 0;
		for(int i=0; i < this.length; i++){
			if(positionInNonSensitiveData == nonSensitiveDataChars.length)
				return true;
			if(this.charRepresentation[i] == nonSensitiveDataChars[positionInNonSensitiveData])
				positionInNonSensitiveData++;
			else
				positionInNonSensitiveData = 0;
		}
		return false;
	}
}

Does that make any sense? How does the Java VM handle this? And how does the Android Dalvik Java VM handle this? Does the Android system cache the UI inputs anyway (so there would be no point in wiping our copies)? Is the data really going to be wiped when I use this class? Where are the real Java internals heroes out there?

Update 1, 11th September 2013: Added Beerware license. Note: This was just some idea I had. Use at your own risk.

Google Ads Encryption Key

Just this last time I will talk about this encryption key, but then I’ll shut up (for me this is so 2011). I presented this thing last year at conferences, but if you just look at the slides I think it’s hard to get the point.

During my Android research in 2011 I encountered several bad practises that are wide spread. Especially hard-coded symmetric encryption keys in the source code of Android apps are often used. One special occurence of one of these encryption keys I just couldn’t forget: Why would Google (Ads) themself use such a thing? It’s not a big deal, but it just adds no real protection and I kept wondering.

445 apps I decompiled used Google Ads code and included the following AES symmetric encryption key:

byte[] arrayOfByte1 = { 10, 55, -112, -47, -6, 7, 11, 75, 
                       -7, -121, 121, 69, 80, -61, 15, 5 };

This AES key is used to encrypt the last known location of the user (all location providers: GPS, WIFI, etc) and send it to Google via the JSON uule parameter. Most of the time the key is located in com.google.ads.util.AdUtil.java, com.google.ads.LocationTracker.java or if the app uses an obfuscator in u.java or r.java. Here’s the corresponding code:

String getLocationParam(){
    List localList = getLastKnownLocations();
    StringBuilder localStringBuilder1 = new StringBuilder();
    int i = 0;
    int j = localList.size();
    if (i < j){
      Location localLocation = (Location)localList.get(i);
      String str1 = protoFromLocation(localLocation);
      String str2 = encodeProto(str1);
      if (str2 != null){
        if (i != 0)
          break label89;
        StringBuilder localStringBuilder2 = localStringBuilder1.append("e1+");
      }
      while (true){
        StringBuilder localStringBuilder3 = localStringBuilder1.append(str2);
        i += 1;
        break;
        label89: StringBuilder localStringBuilder4 = localStringBuilder1.append("+e1+");
      }
    }
    return localStringBuilder1.toString();
  }

String encodeProto(String paramString){
    try{
      Cipher localCipher = Cipher.getInstance("AES/CBC/PKCS5Padding");
      byte[] arrayOfByte1 = { 10, 55, 144, 209, 250, 7, 11, 75, 249, 135, 121, 69, 80, 195, 15, 5 };
      SecretKeySpec localSecretKeySpec = new SecretKeySpec(arrayOfByte1, "AES");
      localCipher.init(1, localSecretKeySpec);
      byte[] arrayOfByte2 = localCipher.getIV();
      byte[] arrayOfByte3 = paramString.getBytes();
      byte[] arrayOfByte4 = localCipher.doFinal(arrayOfByte3);
      int i = arrayOfByte2.length;
      int j = arrayOfByte4.length;
      byte[] arrayOfByte5 = new byte[i + j];
      int k = arrayOfByte2.length;
      System.arraycopy(arrayOfByte2, 0, arrayOfByte5, 0, k);
      int m = arrayOfByte2.length;
      int n = arrayOfByte4.length;
      System.arraycopy(arrayOfByte4, 0, arrayOfByte5, m, n);
      String str1 = Base64.encodeToString(arrayOfByte5, 11);
      str2 = str1;
      return str2;
    }
    catch (GeneralSecurityException localGeneralSecurityException){
      while (true)
        String str2 = null;
    }
  }

This code was produced by a decompiler, so it won’t compile, but you get the idea. Instead of using symmetric crypto, they should have used asymmetric crypto (public key in app, private key on server). There would be no way to intercept the last known location on the network then. Ok, here’s the timeline:

June 2011: Notified security@google.com (auto-response)
October 2011: Talked about it at 0sec / #days and spoke to two friends at Google who are not in the Android/Ads team. One of them (thank you!) sent my mail to the responsible people at Google.
Februar 2012: After a couple of emails I got the answer that they are going to switch to SSL

If you ever find the uule parameter on your network, let me know. One last thing: Depending on your decompiler, the AES key will look like this (wrong decompilation):

byte[] arrayOfByte1 = { 10, 55, 144, 209, 250, 7, 11, 75, 
                       249, 135, 121, 69, 80, 195, 15, 5 };

This is not a valid way to define a byte array in Java, because the integers must be between -127 and 128 (signed integers). That’s another funny fact: if you ever have to brute force a Java byte array like this, I would start with keys where every byte starts with a 0 bit (MSB=0). I saw a lot more of those keys. It seems that some Java developers don’t know about the instantiation with signed integers and therefore choose only numbers between 0 and 128.