Re: [aspectj-users] Plans for dflow pointcut

["Rohit Lists" <rklists@xxxxxxxxx>]

Well since you've posted yours, I guess it only makes sense to post
mine. This is very specific - for protecting against Cross-site
Scripting in Java EE applications. I think I've done a fair bit to
ease performance impact by reducing the scope to calls within
HttpServlet+ code, but there is certainly more that can be done.

Please note this is NOT AT ALL ADEQUATELY TESTED, it's just a "first
draft" that has happened to actually work in very limited instances. I
am hoping to release this as part of a larger effort for an AOP
Security Library later in the year (to be released under GPL at the
Security Compass website). It requires pre-compiling JSPs to work
effectively on real applications. Since this is my first real attempt
at a usuable Aspect, any advice is appreciated. Also, if anyone else
is interested in helping with the library please send me a message.
-------------------------------------------
package com.securitycompass.labs.aop.encoding;

/**
 * HtmlOutputEncodingAspect.aj
 * @author Rohit Sethi
 *
 * Aspect that tracks all Character Sequences returned from "dangeous methods"
 * and encodes them before they are printed to a stream in a Servlet.
 * This is done by creating a IdentityHashMap for EACH instance of
 * a doGet() or doPost() call. The memory address of every string returned
 * by a dangerous method is stored, and every argument to an output stream
 * (e.g. out.println()) is checked to see if its address exists in the hashmap.
 * If so, then the argument is HTML Encoded using your favourite encoder.
 * In the default implementation we use the OWASP Reform Library.
 *
 * Note that we support the following types of Character Sequences:
 * -String (immutable)
 * -StringBuilder (mutable, not thread safe)
 * -StringBuffer (mutable, thread safe)
 * You will need to use Java 5 or above
 *
 * Note that this will NOT protect against all known forms of
 * XSS. However, it will work to significantly reduce the number
 * of XSS vulns.
 *
 * In particular, it won't protect against:
 * 1) When printing to HTML happens OUTSIDE the scope of a Servlet
 * method
 *
 * 2) Within HTML attributes that accept URLs
 * e.g. <img src=USER_SUPPLIED_VAL>
 * Since some browsers will automatically decode HTML encoding for the
 * value of the attribute.
 *
 * 3) Any client-side/DOM-based XSS - no run-time server control will help here.
 *
 * 4) If representations outside of String, StringBuffer, or
StringBuilder are used
 * to store potentially dangerous character sequences (e.g. character arrays)
 *
 * Any others?
 */


import org.owasp.reform.Reform;

import java.util.Map;
import java.util.IdentityHashMap;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;

public abstract aspect HtmlOutputEncodingAspect percflow (doMethods())  {
	
	/*================================================================
	
								Variables
								
	  ==============================================================*/

        //is this the same as static initilization in a class, or will
one object be instantiated
        //for each Aspect created as defined in the percflow()?
	protected Map<CharSequence, CharSeqHolder> dangerousVals =
		new IdentityHashMap<CharSequence, CharSeqHolder> ();
	
	
	
	
	/*================================================================
	
								Pointcuts
	
	  ==============================================================*/
	
	/**
	 * Servlet doGet, doPost, doService, etc.
	 */
	public pointcut doMethods():
		execution (* javax.servlet.http.HttpServlet+.*
				(HttpServletRequest, HttpServletResponse));
	
	
	/**
	 * Pointcut to capture all calls originating from
	 * a servlet (including compiled JSPs).
	 **/
	protected pointcut inServlet():
			within(javax.servlet.http.HttpServlet+);
	
	
	/**
	 * Captures all CharSequeneces derived from the HttpServletRequest
	 * (e.g. getParameter, getAttribute, etc.
	 */
	
	protected pointcut fromRequest():
		call (public CharSequence+ javax.servlet.http.HttpServletRequest+.get*(..));
	
	/**
	 * Captures all CharSequences derived from a Java SQL library
	 */
	protected pointcut sqlCalls():
		call (public CharSequence+ java.sql.**.*(..));
	
	/**
	 * Captures printing out to a writer,
	 * but we don't want to capture system calls
	 */
	protected pointcut writers(CharSequence arg):
		(call (public * java.io.Writer+.write(CharSequence+)) ||
		call (public * java.io.Writer+.print*(CharSequence+))) &&
		args(arg) &&
		!call (public * System.*.*(..));
	
	
	/**
	 * This pointcut should point to all calls from the rest of the code base
	 * that might return malicious content. The safe way to implement this is to
	 * consider ALL non-presentation-layer methods to return unsafe output and
	 * use an exclusion list.
	 * E.g. call(public CharSequence
com.securitycompass.application.**.*) is unsafe,
	 * but call(public String
com.securitycompass.application.SafeHTML.getString()) is safe,
	 * so we can include it in the safeCalls() pointcut
	 */
	public abstract pointcut dangerousCalls();
	
	/**
	 * This pointcut should include calls that you absolutely trust to be
safe and can print
	 * without additional HTML encoding (e.g. parts of code that only
generate string literals).
         * Note that methods should NOT appear in both safeCalls() and
htmlGenerators()
	 */
	public abstract pointcut safeCalls();

	/**
	 * This pointcut should include methods that generate HTML encode and
use parameter CharSequences.
         * Like safeCalls(), methods in this pointcut won't be
encoded. Unlike safecalls(), the parameters
         * to these methods WILL be encoded so that the result is safe HTML.
	 */
	public abstract pointcut htmlGenerators(CharSequence arg);

	/**
	 * If your code is going to end up in htmlAttributes then you need
more stringent
	 * encoding. All methods which return CharSequences that will be
included as part of HTML
	 * attributes should be included here
	 */
	public abstract pointcut htmlAttributeCalls();
	
	
	/**
	 * In case a Character Sequence is appended, we want to capture that
	 * as well and encode prior to concatenation
	 * (since the resultant CharSequence may not be considered dangerous)
	 */
	protected pointcut stringAppend(CharSequence arg, CharSequence trg) :
		inServlet() && call (* CharSequence+.append(CharSequence+))
		&& args(arg) && target(trg) && !within(HtmlOutputEncodingAspect);
	
	/**
	 * In case a Character Sequence is copied, we want to ensure that
	 * the copied Character Sequence is deemed dangerous
	 */
	protected pointcut stringCopy(CharSequence arg) :
		inServlet() && call (CharSequence+.new(CharSequence+))
		&& args(arg) && !within(HtmlOutputEncodingAspect);
	
	
	/**
	 * If a 'toString' is called from a dangerous CharSequence then
	 * we want to capture that and add the resultant string to the
	 * list of dangerous values
	 */
	protected pointcut toStrings(CharSequence trg) : inServlet()
		&& call (String CharSequence+.toString())
		&& target(trg) && !within(HtmlOutputEncodingAspect);
	
	
	/*================================================================
	
								Advice

       ==============================================================*/
	
	/**
	 * Store the memory addresses of any CharSequence derived from a
dangerous method
	 */
	after() returning (CharSequence str) :
		!safeCalls() && !htmlGenerators(CharSequence) && !htmlAttributeCalls() &&
		(dangerousCalls() || fromRequest() || sqlCalls())&& inServlet(){
		CharSeqHolder csh = new CharSeqHolder();
		csh.flag = CharSeqHolder.HTML_ENCODING;
		dangerousVals.put(str, csh);
	}
	
	/**
	 * Store the memory addresses of any CharSequence derived from a
dangerous method
	 * that will end up in an HTML attribute
	 */
	after() returning (CharSequence str) :
		!safeCalls() && !htmlGenerators(CharSequence) &&
htmlAttributeCalls() &&  inServlet(){
		CharSeqHolder csh = new CharSeqHolder();
		csh.flag = CharSeqHolder.HTML_ATTRIBUTE_ENCODING;
		dangerousVals.put(str, csh);
	}
	
	
	
	/**
	 * Complicated advice to deal with when a dangerous CharSequence
	 * is involved in appending.
	 * A CharSequenceHolder keeps a CharSquence called "modifiedVersion".
	 * This modified version is a copy of the CharSequence, except the
	 * dangerous parts of it are encoded. This is because we don't want to
	 * alter the original CharSequence until it's printed to screen, but
	 * we won't know which parts to encode if we don't keep track of the
	 * dangerous portions of the CharSequence.
	 * For efficiency reasons, we don't want to keep a modified version
	 * of every dangerous CharSequence.
	 * Instead, we ONLY use this modified version if the
	 * CharSequence is involved in appending. This
	 * modified version is encoded while the original version is left
	 * untouched. Then, during printing to screen the modified version
	 * is used.
	 * @param arg CharSequence to be appended to target
	 * @param trg StringBuilder or Stringbuffer that is being appended to
	 * In x.append(y), arg=y and trg=x
	 */
	Object around (CharSequence arg, CharSequence trg) :
		stringAppend(arg, trg) {
		CharSeqHolder targetCsh = dangerousVals.get(trg);
		if (targetCsh != null) { //the target CharSequence is dangerous
			//We need to store a modified version of this
			//CharSequence since the target is dangerous
			//If we already have a modified version, we use that
			//otherwise we create one and encode it
			if (targetCsh.modifiedVersion == null){
				targetCsh.modifiedVersion = encode (trg, targetCsh);
			}
		}
		//Save a copy of the argument
		//If it's dangerous then we want to append an encoded copy
		//instead
		CharSequence newArg = arg;
		
		//Now check to see if the argument is dangerous
		CharSeqHolder argCsh = dangerousVals.get(arg);
		
		if (argCsh != null) {
			newArg = encode(arg, argCsh);
			//If the target is NOT dangerous but the argument IS,
			//we need to now call the target dangerous and save
			//a modified copy. We also need to store how the argument
			//is going to be encoded
			if (targetCsh == null){
				targetCsh  = new CharSeqHolder();
				targetCsh.flag = argCsh.flag;
				targetCsh.modifiedVersion = getCopy(trg);		
				dangerousVals.put(trg, targetCsh);
			}
		}
		//If there is no target CharSeqHolder then both the target and argument
		//are safe
		if (targetCsh != null){
			//Now append the adjusted argument
			if (targetCsh.modifiedVersion instanceof StringBuffer){
				((StringBuffer)targetCsh.modifiedVersion).append(newArg);
			} else if (targetCsh.modifiedVersion instanceof StringBuilder){
				((StringBuilder)targetCsh.modifiedVersion).append(newArg);
			}
		}
		return proceed(arg, trg);
	}
	
	
	/**
	 * If a dangerous CharSequence is passed in as an argument
	 * to the constructor of another CharSequence, we need to
	 * mark the newly created CharSequence as dangerous.
	 * @param arg Argument to a toString call
	 */
	after (CharSequence arg) returning (CharSequence retVal):
		stringCopy(arg) {
		//Is the argument dangerous?
		CharSeqHolder argCsh = dangerousVals.get(arg);
		if (argCsh != null) {
			//Argument is dangerous, so let's mark the target as dangerous
			//and add a CharSeqHolder for it
			CharSeqHolder targetCsh  = new CharSeqHolder();
			targetCsh.flag = argCsh.flag;
			//If there is a modified version, we need to keep
			//that in the new CSH
			if (arg instanceof String && !(argCsh.modifiedVersion == null) &&
!(argCsh.modifiedVersion instanceof String)) {
				//If a String constructor is called, our modifiedVersion has to be
a String as well
				targetCsh.modifiedVersion = argCsh.modifiedVersion.toString();
			}else if (argCsh.modifiedVersion != null) {
				//Change the target to reflect the new modified version
				targetCsh.modifiedVersion = getCopy(argCsh.modifiedVersion);
			}
			dangerousVals.put(retVal, targetCsh);
			
		}
	}
	

	
	/**
	 * We need to mark the result of any toString() method
	 * called from a dangerous target
	 * @param arg Target of a toString() call (e.g. if the program
	 * called x.toString(), then x would be the arg)
	 */
	after (CharSequence arg) returning (CharSequence retVal):
		toStrings(arg){
		//Is the argument dangerous?
		CharSeqHolder argCsh = dangerousVals.get(arg);
		if (argCsh != null) {
			//Argument is dangerous, so let's mark the target as dangerous
			//and add a CharSeqHolder for it
			CharSeqHolder targetCsh  = new CharSeqHolder();
			targetCsh.flag = argCsh.flag;
			//If there is a modified version, we need to keep
			//that in the new CSH
			if (arg instanceof String && !(argCsh.modifiedVersion == null) &&
!(argCsh.modifiedVersion instanceof String)) {
				//If toString() is called, our modifiedVersion has to be a String as well
				targetCsh.modifiedVersion = argCsh.modifiedVersion.toString();
			}else if (argCsh.modifiedVersion != null) {
				//Change the target to reflect the new modified version
				targetCsh.modifiedVersion = getCopy(argCsh.modifiedVersion);
			}
			dangerousVals.put(retVal, targetCsh);
		}
	}
	
	
	
	/**
	 * Adivce that encodes any dangerous CharSequences sent to a writer or
         * htmlGenerator
	 * @param arg CharSequence that will be encoded
	 */
	Object around (CharSequence arg): (writers(arg) && inServlet()) ||
htmlGenerators(arg){
		CharSeqHolder csh = dangerousVals.get(arg);
		if (csh != null) {
			//Encode the argument and then pass it to the writer
			//Type of encoding is based on the flag stored
			//in the CharSeqHolder
			return (proceed(encode(arg, csh)));
		}
		else {
			//Comes from non-dangerous input, e.g. string literal in code
			//or from one of the safeCalls()
			return proceed(arg);
		}
	}

	/*================================================================
	
								Helper Methods

	  ==============================================================*/
	
	
	/**
	 * Encodes the given argument based on the parameter CharSeqHolder
	 * @param arg Argument to encode
	 * @param csh CharSeqHolder that stores data about how to encode arg
	 * @return Encoded version of arg
	 */
	protected CharSequence encode(CharSequence arg, CharSeqHolder csh){
		String retval;
		if (arg == null)
			return null;
		//If there is already an encoded value, we'll use that now
		if (csh.modifiedVersion != null) {
			retval = csh.modifiedVersion.toString();
		} else if (csh.flag == CharSeqHolder.HTML_ATTRIBUTE_ENCODING){
			//Otherwise we HTML attribute encode
			retval = Reform.HtmlEncode(arg.toString());
		} else {
			//Otherwise we HTML encode
			retval = Reform.HtmlAttributeEncode(arg.toString());
		}
		//While the result of the encoder is a String, we may need
		//to return back a StringBuilder or StringBuffer
		if (arg instanceof StringBuilder) {
			return new StringBuilder(retval);
		}else if (arg instanceof StringBuffer) {
			return new StringBuffer(retval);
		}else {
			//Return back string
			return retval;
		}
		
	}
	
	
	/**
	 * Helper method to create a copy of a character sequence to
	 * use as a modified version
	 * @param arg CharSequence to make a copy of. MUST be String, StringBuilder,
	 * or StringBuffer. Anything else will cause an IllegalArgumentException
	 * @return Copy of the parameter
	 */
	private CharSequence getCopy(CharSequence arg){
		if (arg instanceof String){
			return new String((String)arg);
		}if (arg instanceof StringBuffer){
			return new StringBuffer(arg);
		}if (arg instanceof StringBuilder){
			return new StringBuilder(arg);
		}
		//This shouldn't happen, need to throw an exception if it does
		throw new IllegalArgumentException("Only Strings, StringBuffers, " +
				"and String Builders are supported. "
				+ arg.getClass().getCanonicalName() +
				" is not supported");
	}
	
	
	/*================================================================
	
								Helper Classes

	  ==============================================================*/
	
	/**
	In our map, we want to store only references to the Character Sequences
	(e.g. String, StringBuffer, StringBuilder, etc.).
	In most cases, we don't need to store anything else. However, we do
need to know
	if the Character Sequence is destined for HTML-code, HMTL-Attribute code,
	or JavaScript code in order to apply the appropriate encodings.

	In the cases where a CharacterSequence is being modified (e.g.
StringBuffer.append()),
	we don't know what it will be used for. We DON'T want to encode if
it's being used
	elsewhere in the codebase, but we DO want to encode if it's being
printed to a stream.
	Our solution is to save an encoded version in the map called
"modifiedVersion", and print
	that to stream rather than the unencoded version. Note that this may
be expensive since
	the string might be large, so we only keep a modified version if
necessary. In all other
	cases the modifiedVersion is null.
	*/
	public class CharSeqHolder {
		int flag; //stores how the ChracterSequence needs to be encoded
		CharSequence modifiedVersion = null; //stores an encoded copy of the
CharacterSequence, if necessary
		
		static final int HTML_ENCODING = 0;
		static final int HTML_ATTRIBUTE_ENCODING = 1;
		//static final int JAVASCRIPT_ENCODING = 2; //Not yet suppported
	}
	
}




On Mon, Aug 11, 2008 at 4:46 PM, Rohit Lists <rklists@xxxxxxxxx> wrote:
> Hi Simone, thanks for the posting. Yes this is very much based on the
> idea of perl taint mode. It's funny I actually implemented something
> very similar to what you've posted for the time being. My concerns are
> the same as yours though.
>
> Another interesting point is that in the paper I've linked, you simply
> remove taint prior to encoding. For the case of XSS, this isn't so
> easy.Take for example:
>
> String dangerous = request.getParameter("something");
> StringBuffer sb = "<b> This tag was found " +  dangerous + "</b>"
>
> In this case, you want to encode dangerous but not encode the String
> literals. In my solution, I saved a copy of the StringBuffer (actually
> I used CharSequences rather Strings/StringBuffers/StringBuilders since
> all three classes implement the CharSequence interface in java 5+) and
> encoded only the dangerous parts in the copy. When it came time to
> print, I used the safe encoded copy rather than the original
> CharSequence. This of course has performance penalty, but I don't see
> a clear alternative to it ...
>
> On Mon, Aug 11, 2008 at 3:08 PM, Simone Gianni <simoneg@xxxxxxxxxx> wrote:
>> My two cents :
>>  - +1 for it, it IS really useful.
>>  - Perl has a similar mechanism, called taint mode, which basically
>> flags some variables as "tainted", makes this property transitive, and
>> places restrictions to their usage. If you have access to a *nix box,
>> type "man perlsec" to look at the manual. It sounds like something very
>> very similar (but seen from an implementation perspective) to what is
>> proposed in the paper.
>>  - It is possible (thought very expensive) to write aspects doing
>> similar (but far less sophisticated) things right now. For very
>> expensive I mean both for the programmer and for the runtime.
>>
>> If you are really, really, really interested, have nothing better to do,
>> it's raining and you cannot go to the beach ... a "simple" example follows :
>>
>> import java.util.WeakHashMap;
>>
>> public aspect Tainting {
>>    // This will hold the tainted objects (only Strings and
>> StringBuilders right now).
>>    // I use a WeakHashMap so that the GC can reclaim them
>>    private WeakHashMap<Object, Boolean> tainteds = new
>> WeakHashMap<Object, Boolean>();
>>
>>    // This pointcut tells which method returns insecure stuff
>>    pointcut taintIt() : call(String TestClass.getParameter(..));
>>
>>    after() returning (String s) : taintIt() {
>>        tainteds.put(s, Boolean.TRUE);
>>    }
>>
>>    // Now we have to apply transitivity. This is a PITA with strings,
>> cause they are immutable objects defined in the JRE so not accessible by
>> AspectJ.
>>    // Anyway we have basically three types of transitivity :
>>    //  ... New objects created with  tainted objects as parameters (the
>> case for a constructor having a tainted parameter)
>>    //  ... Objects modified with tainted objects (the case for a
>> setter, in this case the append method)
>>    //  ... New objects PRODUCED BY tainted objects (the case for the
>> toString() method of a "contaminated" StringBuilder, it's what declare
>> propagate does in the paper)
>>
>>    // This is what a recent compiler generated when two strings are
>> concatenated
>>    pointcut stringAddition(Object isTainted, Object toTaint) : call(*
>> StringBuilder.append(String)) && args(isTainted) && target(toTaint);
>>
>>    // more pointcuts like this could be added, there are endless
>> possiblities :)
>>
>>    // The case for modifications (append)
>>    pointcut transitives(Object isTainted, Object toTaint) :
>> stringAddition(isTainted, toTaint);
>>
>>    after(Object isTainted, Object toTaint) returning :
>> transitives(isTainted, toTaint) {
>>        if (tainteds.containsKey(isTainted))
>>            tainteds.put(toTaint, Boolean.TRUE);
>>    }
>>
>>    // The case for new objects produced by tainted objects (the toString)
>>    pointcut transitiveReturns(Object isTainted) : call(String
>> StringBuilder.toString()) && target(isTainted);
>>
>>    after(Object isTainted) returning (Object toTaint):
>> transitiveReturns(isTainted) {
>>        if (tainteds.containsKey(isTainted))
>>            tainteds.put(toTaint, Boolean.TRUE);
>>    }
>>
>>    // The case for constructors having taninted arguments
>>    pointcut transitiveCreations(Object isTainted) : call(public
>> StringBuilder.new(String)) && args(isTainted);
>>
>>    Object around(Object isTainted) : transitiveCreations(isTainted) {
>>        Object ret = proceed(isTainted);
>>        if (tainteds.containsKey(isTainted))
>>            tainteds.put(ret, Boolean.TRUE);
>>        return ret;
>>    }
>>
>>    // Here is where we check if a method is called with tainted parameters
>>    pointcut toProtect() : execution(* TestClass.do*(..));
>>
>>    before() : toProtect() {
>>        Object[] params = thisJoinPoint.getArgs();
>>        for (Object object : params) {
>>            if (tainteds.containsKey(object))
>>                throw new RuntimeException("Using tainted stuff");
>>        }
>>    }
>>
>> }
>>
>> If you want to compile it, and you are lazy, this is the TestClass :
>>
>> public class TestClass {
>>
>>    public String getParameter(String name) {
>>        return "ciao";
>>    }
>>
>>    public void doSecureStuff(String value) {
>>        System.out.println("Doing secure stuff with '" + value + "'");
>>    }
>>
>>    public void tryIt() {
>>        String secure = "Created here";
>>        doSecureStuff(secure);
>>
>>        String tainted = getParameter("a");
>>        try {
>>            doSecureStuff(tainted);
>>        } catch (Exception e) {
>>            System.out.println("Thrown exeception cause tainted");
>>        }
>>
>>        String transitive = tainted + " ciao";
>>        try {
>>            doSecureStuff(transitive);
>>        } catch (Exception e) {
>>            System.out.println("Thrown exeception cause tainted");
>>        }
>>    }
>>
>>
>>    public static void main(String[] args) {
>>        TestClass t = new TestClass();
>>        t.tryIt();
>>
>>        StringBuilder b = new StringBuilder("ciao");
>>        b.toString();
>>    }
>> }
>>
>> Obviously this is a toy, it is implementation dependant (use another
>> compiler, StringBuilder not in place, nothing will work), and it's very
>> heavy, both on performances and on code itself (every string
>> concatenation is adviced, try to make a more generic aspect which
>> intecepts any set/get and you'll get thousands of advices), and it will
>> never work on primitive types since they don't have an identity.
>>
>> hope you enjoied :)
>>
>> Simone
>>
>> Rohit Lists wrote:
>>> Hello, are there are any plans to implement the data flow pointcut as
>>> defined here www.graco.c.u-tokyo.ac.jp/~masuhara/papers/aplas2003.pdf
>>> in AspectJ? This would be an invaluable resource to application
>>> security since large classes of security problems deal with tracing
>>> data from source to sink (e.g. cross site scripting,
>>> SQL/XML/Xpath/Ldap injection, OS interaction vulnerabilities, etc.).
>>>
>>> If there are no such plans, is it because there are not enough
>>> resources to work on this? Has there already been a discussion on
>>> implementing dflow pointcut and a decision was made not to implement
>>> it?
>>>
>>> Thanks,
>>>
>>>
>>
>>
>> --
>> Simone Gianni
>> http://www.simonegianni.it/
>> CEO Semeru s.r.l.
>> Apache Committer
>>
>> _______________________________________________
>> aspectj-users mailing list
>> aspectj-users@xxxxxxxxxxx
>> https://dev.eclipse.org/mailman/listinfo/aspectj-users
>>
>
>
>
> --
> Rohit Sethi
> Security Compass
> http://www.securitycompass.com
>



-- 
Rohit Sethi
Security Compass
http://www.securitycompass.com