ESAPI / esapi-java-legacy

ESAPI (The OWASP Enterprise Security API) is a free, open source, web application security control library that makes it easier for programmers to write lower-risk applications.
https://owasp.org/www-project-enterprise-security-api/
Other
613 stars 366 forks source link

CodecTest unit tests never test with a populated char array. #359

Closed xeno6696 closed 8 years ago

xeno6696 commented 8 years ago

The following unit test from @jeremiahjstacey reproduces the problem: If you mark a percent symbol as "immune" as in "do not encode or decode" the API will encode it anyway.

package org.owasp.esapi;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.List;

import org.junit.Assert;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
import org.junit.runners.Parameterized.Parameters;
import org.owasp.esapi.codecs.*;

/**
 * Parameterized test to verify that the Immunity parameter for a codec encode/decode event works as expected on a series of special characters.
 *  
 * @author jeremiah.j.stacey@gmail.com
 * @since Jan 29, 2016
 *
 */
@RunWith(Parameterized.class)
public class CodecImmunityTest {
    /** character arrays used as immunity lists from Default Encoder.*/
    private final static char[] IMMUNE_HTML = { ',', '.', '-', '_', ' ' };
    private final static char[] IMMUNE_HTMLATTR = { ',', '.', '-', '_' };
    private final static char[] IMMUNE_CSS = {};
    private final static char[] IMMUNE_JAVASCRIPT = { ',', '.', '_' };
    private final static char[] IMMUNE_VBSCRIPT = { ',', '.', '_' };
    private final static char[] IMMUNE_XML = { ',', '.', '-', '_', ' ' };
    private final static char[] IMMUNE_SQL = { ' ' };
    private final static char[] IMMUNE_OS = { '-' };
    private final static char[] IMMUNE_XMLATTR = { ',', '.', '-', '_' };
    private final static char[] IMMUNE_XPATH = { ',', '.', '-', '_', ' ' };
    // These are inline in the encode methods, but same principle.
   // private final static char[] IMMUNE_LDAP = { '\\', '*', '(', ')', '\0' };
   // private final static char[] IMMUNE_DN = { '\\', ',', '+', '"', '<', '>', ';' };

    @Parameters(name = "{0}")
    public static Collection<Object[]> getParams() {
        Collection<Codec> knownCodecs = new ArrayList<Codec>();
        knownCodecs.add(new CSSCodec());
        knownCodecs.add(new DB2Codec());
        knownCodecs.add(new HTMLEntityCodec());
        knownCodecs.add(new JavaScriptCodec());
        knownCodecs.add(new MySQLCodec(0)); //Standard
        knownCodecs.add(new MySQLCodec(1)); //ANSI
        knownCodecs.add(new OracleCodec());
        knownCodecs.add(new PercentCodec());
        knownCodecs.add(new UnixCodec());
        knownCodecs.add(new VBScriptCodec());
        knownCodecs.add(new WindowsCodec());
        knownCodecs.add(new XMLEntityCodec());

        //XXX:  Add more strings here!!
        List<String> sampleStrings = Arrays.asList("%De");

        Collection<Object[]> params = new ArrayList<Object[]>();
        for (Codec codec : knownCodecs) {
            for (String sample : sampleStrings) {
                params.add(new Object[]{codec.getClass().getSimpleName() + " " + sample, codec, sample});
            }
        }

        //Add Tests for codecs against the configured ImmunityLists within the Default Encoder.
        params.addAll(buildImmunitiyValidation(new HTMLEntityCodec(), IMMUNE_HTML, "IMMUNE_HTML"));
        params.addAll(buildImmunitiyValidation(new HTMLEntityCodec(), IMMUNE_XPATH, "IMMUNE_XPATH"));
        params.addAll(buildImmunitiyValidation(new HTMLEntityCodec(), IMMUNE_HTMLATTR, "IMMUNE_HTMLATTR"));
        params.addAll(buildImmunitiyValidation(new CSSCodec(), IMMUNE_CSS, "IMMUNE_CSS"));
        //params.addAll(buildImmunitiyValidation(new DB2Codec(), IMMUNE_HTML, ""));
        params.addAll(buildImmunitiyValidation(new JavaScriptCodec(), IMMUNE_JAVASCRIPT, "IMMUNE_JAVASCRIPT"));
        params.addAll(buildImmunitiyValidation(new MySQLCodec(0), IMMUNE_SQL, "IMMUNE_SQL"));
        params.addAll(buildImmunitiyValidation(new MySQLCodec(1), IMMUNE_SQL, "IMMUNE_SQL"));
        params.addAll(buildImmunitiyValidation(new OracleCodec(), IMMUNE_HTML, "IMMUNE_HTML"));
        //params.addAll(buildImmunitiyValidation(new PercentCodec(), IMMUNITY_????, "???")); // No Immunity defined for PercentEncoder
        params.addAll(buildImmunitiyValidation(new UnixCodec(), IMMUNE_OS, "IMMUNE_OS"));
        params.addAll(buildImmunitiyValidation(new VBScriptCodec(), IMMUNE_VBSCRIPT, "IMMUNE_VBSCRIPT"));
        params.addAll(buildImmunitiyValidation(new WindowsCodec(), IMMUNE_OS, "IMMUNE_OS"));
        params.addAll(buildImmunitiyValidation(new XMLEntityCodec(), IMMUNE_XML, "IMMUNE_XML"));
        params.addAll(buildImmunitiyValidation(new XMLEntityCodec(), IMMUNE_XMLATTR, "IMMUNE_XMLATTR"));

        params.addAll(fullCharacterCodecValidation(knownCodecs));

        return params;
    }

    private static Collection<Object[]> buildImmunitiyValidation(Codec codec, char[] immunities, String descriptor) {
        Collection<Object[]> params = new ArrayList<Object[]>();
        for (char c : immunities) {
            params.add(new Object[]{codec.getClass().getSimpleName() + " " + descriptor + " ("+ c + ")", codec, String.valueOf(c)});
        }
        return params;
    }

    private static Collection<Object[]> fullCharacterCodecValidation(Collection<Codec> codecs) {
        char[] holyCowTesting = StringUtilities.union(EncoderConstants.CHAR_ALPHANUMERICS, EncoderConstants.CHAR_SPECIALS); 
        Collection<Object[]> params = new ArrayList<Object[]>();
        for (Codec codec: codecs) {
            params.addAll(buildImmunitiyValidation(codec, holyCowTesting, "Full_ALPHA_AND_SPECIALS"));
        }

        return params;
    }

    private final Codec codec;
    private final String string;
    private final char[] immunityList;

    public CodecImmunityTest(String ignored, Codec codec, String toTest) {
        this.codec = codec;
        this.string = toTest;
        /**
         * The Immunity character array is every character in the String we're testing.
         * 
         */
        this.immunityList = toTest.toCharArray();
    }

    @Test
    public void testImmuneEncode() {
        String encoded = codec.encode(immunityList, string);
        Assert.assertEquals(string, encoded);
    }
    /*
    @Test
    public void testImmuneDecode() {
        String decoded = codec.decode(string);
        Assert.assertEquals(string, decoded);
    }
*/
}
kwwall commented 8 years ago

If you mark a percent symbol as "immune" as in "do not encode or decode" the API will encode it anyway.

Okay, but I guess I have to ask if that even makes sense for the PercentCodec, that it to make '%' immune, when it is '%' that does the encoding. If you did make '%' immune, then how would you decode something like "http%3A%2F%2Fwww.owasp.org%2F" ??? Maybe I'm missing something here (plus, I've never worked on encoders except for trivial bug fixes), so I don't see how you can legitimately make the actual quoting character (in this case, a '%') an character immune to encoding or decoding. If you really want something like that, then I think you really need to implement you own custom class that extends Codec to do exactly what you want and then add that to the ArrayList of codecs being used.

@planetlevel - What say you? You wrote this stuff and I've never really deeply dug into it.

-kevin

On Fri, Jan 29, 2016 at 2:37 PM, Matt Seil notifications@github.com wrote:

The following unit test from @jeremiahjstacey https://github.com/jeremiahjstacey reproduces the problem: If you mark a percent symbol as "immune" as in "do not encode or decode" the API will encode it anyway.

package org.owasp.esapi.codecs;

import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; import java.util.List; import org.junit.Assert; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.Parameterized; import org.junit.runners.Parameterized.Parameters; import org.owasp.esapi.codecs.*;

/* \ Parameterized test to verify that the Immunity parameter for a codec encode/decode event works

  • as expected on a series of special characters. * * @author jeremiah.j.stacey@gmail.com *
  • @since Jan 29, 2016 * */ @RunWith(Parameterized.class) public class CodecImmunityTest { @Parameters(name = "{0}")

public static Collection<Object[]> getParams() { Collection knownCodecs = new ArrayList(); knownCodecs.add(new CSSCodec()); knownCodecs.add(new DB2Codec()); knownCodecs.add(new HTMLEntityCodec()); knownCodecs.add(new JavaScriptCodec()); knownCodecs.add(new MySQLCodec(0)); //Standard knownCodecs.add(new MySQLCodec(1)); //ANSI knownCodecs.add(new OracleCodec()); knownCodecs.add(new PercentCodec()); knownCodecs.add(new UnixCodec()); knownCodecs.add(new VBScriptCodec()); knownCodecs.add(new WindowsCodec()); knownCodecs.add(new XMLEntityCodec()); //XXX: Add more strings here!! List sampleStrings = Arrays.asList("%De", "&"); Collection<Object[]> params = new ArrayList<Object[]>(); for (Codec codec : knownCodecs) { for (String sample : sampleStrings) { params.add(new Object[]{codec.getClass().getSimpleName() + " " + sample, codec, sample}); } }

return params;
}

private final Codec codec; private final String string; private final char[] immunityList; public CodecImmunityTest(String ignored, Codec codec, String toTest) { this.codec = codec; this.string = toTest; /* * The Immunity character array is every character in the String we're testing. * / this.immunityList = toTest.toCharArray(); } @Test public void testImmuneEncode() { String encoded = codec.encode(immunityList, string); Assert.assertEquals(string, encoded); }

@Test public void testImmuneDecode() {
    String decoded = codec.decode(string);
    Assert.assertEquals(string, decoded);
}

}

— Reply to this email directly or view it on GitHub https://github.com/ESAPI/esapi-java-legacy/issues/359.

Blog: http://off-the-wall-security.blogspot.com/ | Twitter: @KevinWWall NSA: All your crypto bit are belong to us.

jeremiahjstacey commented 8 years ago

I believe this to be an inconsistency in the Codec API. It requires that the user of the Codec class (client) be explicitly aware that if they use an instanceof a Codec interface that it will act differently in a single case. This requires that the client either never pass around the Codec as the interface type or that before using they type-check at runtime. It's worth noting that the immunity lists are being used in the DefaultEncoder implementation now for multiple types.

/**
     *  Character sets that define characters (in addition to alphanumerics) that are
     * immune from encoding in various formats
     */
    private final static char[]     IMMUNE_HTML = { ',', '.', '-', '_', ' ' };
    private final static char[] IMMUNE_HTMLATTR = { ',', '.', '-', '_' };
    private final static char[] IMMUNE_CSS = {};
    private final static char[] IMMUNE_JAVASCRIPT = { ',', '.', '_' };
    private final static char[] IMMUNE_VBSCRIPT = { ',', '.', '_' };
    private final static char[] IMMUNE_XML = { ',', '.', '-', '_', ' ' };
    private final static char[] IMMUNE_SQL = { ' ' };
    private final static char[] IMMUNE_OS = { '-' };
    private final static char[] IMMUNE_XMLATTR = { ',', '.', '-', '_' };
    private final static char[] IMMUNE_XPATH = { ',', '.', '-', '_', ' ' };

The testcase has been updated to add these validations into the process, ensuing that the Codec that each immunity list is used with inside the DefaultEncoder acts as appropriate. They each appear to, from my understanding.

I also amped it up a little and made another sequence of additions which use the combined character content of EncoderConstants.CHAR_ALPHANUMERICS and EncoderConstants.CHAR_SPECIALS, and perform the same test against each Codec available.

If I have a codec and I encode a string of a single character and apply an immunity list of the same character I expect the string to not be encoded.

PercentCodec does not follow suit with the other implementations, and in one case the WindowsCodec also fails.

16 failures of 1900 test variations and 15 of them are in the PercentCodec.

I don't think this is a bug so much with a single character, or characters, for the PercentCodec. It's more that the PercentCodec doesn't respect the immunity list parameter like the other Codec implementations.

I should also point out that I am not testing 'special' character cases for the Codecs. I'm sure they exist, but I do not presently have a finite list I could add to the testcase.

jeremiahjstacey commented 8 years ago

I also commented out the decode test. It has no bearing with the immunity list and was not providing valid feedback.With that aspect removed, we're looking at 14 failures in 950 tests, all in the PercentCodec.

It's also all special characters: ! $ * + - = ? @ ^ _ | ~ . (and the %De from before)

xeno6696 commented 8 years ago

What Jeremiah is saying, is that in all other cases we accept the escaping char as something to be ignored. The whole point of offering an immunity list is to allow a programmer the power to fix some oddball case we never considered.

Except in the percent codec.

So it's doing something unique compared to the rest of our API. If it's wrong because we determine its an invalid use case, then the other codecs need to be rewritten to fit that expectation.

We need to be consistent.

jeremiahjstacey commented 8 years ago

Thank you Matt, that is what I mean to say.

kwwall commented 8 years ago

Okay, yes @jeremiahjstacey and @xeno6696, I believe this is an inconsistency, but I think it is a deliberate one. If you look at the PercentCodec (which is probably not a good name given it's implied intended uses; maybe URICodec would have been better), you will see that it is really intended for encoding URIs. In fact there is a reference to RFC 3986 in the code for this class. Also, you see this comment for the encodeCharacter() class that is used for encoding:

    /**
     * Encode a character for URLs
     * @param immune characters not to encode
     * @param c character to encode
     * @return the encoded string representing c
     */
    public String encodeCharacter( char[] immune, Character c )
    {

From that lead-in comment of "Encode a character for URLs" we see that the intended context where this encoder should be used is for URLs (which is discussed in the RFC). Thus the only "immune" characters are alphanumeric characters plus a set of "reserved characters" mentioned in that RFC.

Therefore, I would argue that any user-defined set of "immune" characters are correctly ignored in this PercentCodec.encodeCharacter() method.

From a purely developer's perspective, sure, the results can be surprising (as seen by issue #306), but this is a security library, so security trumps developer convenience and that may imply inconsistencies or non-intuitive behavior. (No question that we could do a better job documenting it somewhere though, likely in this method's javadoc.) I would be concerned that if we did allow the "immune" characters to be passed in here, then it could make it too easy for developers to do the wrong thing. If something like you both have discussed here and as originally brought up in issue #306 is desired, it is a relatively simple matter for a developer to write a custom Codec class by extending PercentCodec and overriding encodeCharacter() to into account the 'immune' char array. But doing so is no longer going to conform to RFC 3986. Then they can use the public DefaultEncoder CTOR to pass in the fully qualified class name of their custom Codec class. That's how I would argue that this edge case be handled.

I'm interested in what @planetlevel has to say about this, but IMO, issue #306 should be closed as well as this issue, but I think that we should still add @jeremiahjstacey test case, but just with the PercentCodec class commented out with a comment referring to this issue and/or issue #306.

Thoughts? -kevin

planetlevel commented 8 years ago

I think we should be consistent. I agree it's hard to imagine a use case where % would be immune, but who knows. And I can certainly imagine cases where one wouldn't want to percent-encode other specials.

--Jeff


From: Kevin W. Wall notifications@github.com<mailto:notifications@github.com> Sent: Saturday, January 30, 2016 2:05 AM Subject: Re: [esapi-java-legacy] CodecTest unit tests never test with a populated char array. (#359) To: ESAPI/esapi-java-legacy esapi-java-legacy@noreply.github.com<mailto:esapi-java-legacy@noreply.github.com> Cc: Jeff Williams jeff.williams@contrastsecurity.com<mailto:jeff.williams@contrastsecurity.com>

If you mark a percent symbol as "immune" as in "do not encode or decode" the API will encode it anyway.

Okay, but I guess I have to ask if that even makes sense for the PercentCodec, that it to make '%' immune, when it is '%' that does the encoding. If you did make '%' immune, then how would you decode something like "http%3A%2F%2Fwww.owasp.orghttp://2fwww.owasp.org%2F" ??? Maybe I'm missing something here (plus, I've never worked on encoders except for trivial bug fixes), so I don't see how you can legitimately make the actual quoting character (in this case, a '%') an character immune to encoding or decoding. If you really want something like that, then I think you really need to implement you own custom class that extends Codec to do exactly what you want and then add that to the ArrayList of codecs being used.

@planetlevel - What say you? You wrote this stuff and I've never really deeply dug into it.

-kevin

On Fri, Jan 29, 2016 at 2:37 PM, Matt Seil notifications@github.com<mailto:notifications@github.com> wrote:

The following unit test from @jeremiahjstacey https://github.com/jeremiahjstacey reproduces the problem: If you mark a percent symbol as "immune" as in "do not encode or decode" the API will encode it anyway.

package org.owasp.esapi.codecs;

import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; import java.util.List; import org.junit.Assert; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.Parameterized; import org.junit.runners.Parameterized.Parameters; import org.owasp.esapi.codecs.*;

/* \ Parameterized test to verify that the Immunity parameter for a codec encode/decode event works

  • as expected on a series of special characters. * * @author jeremiah.j.stacey@gmail.commailto:jeremiah.j.stacey@gmail.com *
  • @since Jan 29, 2016 * */ @RunWith(Parameterized.class) public class CodecImmunityTest { @Parameters(name = "{0}")

public static Collection<Object[]> getParams() { Collection knownCodecs = new ArrayList(); knownCodecs.add(new CSSCodec()); knownCodecs.add(new DB2Codec()); knownCodecs.add(new HTMLEntityCodec()); knownCodecs.add(new JavaScriptCodec()); knownCodecs.add(new MySQLCodec(0)); //Standard knownCodecs.add(new MySQLCodec(1)); //ANSI knownCodecs.add(new OracleCodec()); knownCodecs.add(new PercentCodec()); knownCodecs.add(new UnixCodec()); knownCodecs.add(new VBScriptCodec()); knownCodecs.add(new WindowsCodec()); knownCodecs.add(new XMLEntityCodec()); //XXX: Add more strings here!! List sampleStrings = Arrays.asList("%De", "&"); Collection<Object[]> params = new ArrayList<Object[]>(); for (Codec codec : knownCodecs) { for (String sample : sampleStrings) { params.add(new Object[]{codec.getClass().getSimpleName() + " " + sample, codec, sample}); } }

return params; }

private final Codec codec; private final String string; private final char[] immunityList; public CodecImmunityTest(String ignored, Codec codec, String toTest) { this.codec = codec; this.string = toTest; /* * The Immunity character array is every character in the String we're testing. * / this.immunityList = toTest.toCharArray(); } @Test public void testImmuneEncode() { String encoded = codec.encode(immunityList, string); Assert.assertEquals(string, encoded); }

@Test public void testImmuneDecode() { String decoded = codec.decode(string); Assert.assertEquals(string, decoded); } }

Reply to this email directly or view it on GitHub https://github.com/ESAPI/esapi-java-legacy/issues/359.

Blog: http://off-the-wall-security.blogspot.com/ | Twitter: @KevinWWall NSA: All your crypto bit are belong to us.

Reply to this email directly or view it on GitHubhttps://github.com/ESAPI/esapi-java-legacy/issues/359#issuecomment-177090657.

kwwall commented 8 years ago

Okay. We can do that. Right now, the check is like this:

    public String encodeCharacter( char[] immune, Character c )
    {
        String cStr = String.valueOf(c.charValue());
        byte[] bytes;
        StringBuilder sb;

        if(UNENCODED_SET.contains(c))
            return cStr;

        bytes = toUtf8Bytes(cStr);
        sb = new StringBuilder(bytes.length * 3);
        for(byte b : bytes)
            appendTwoUpperHex(sb.append('%'), b);
        return sb.toString();
    }

If 'immune' is a char array of >= 1 character, do you think we should use it EXCLUSIVELY as the immune set or IN ADDITION TO the UNENCODED_SET. I would say "in addition to", otherwise there is probably going to be even MORE surprises, like "why are these alphanumeric characters being encoded?". Agree?

kwwall commented 8 years ago

That is, I think UNENCODED_SET should still be automatically considered "immune" in addition to any "immune" characters passed as the first argument.

planetlevel commented 8 years ago

Yes. I concur. Hopefully that's what the other codecs do? Apologies I'm AFK right now

--Jeff

On Sat, Jan 30, 2016 at 5:04 PM -0800, "Kevin W. Wall" notifications@github.com<mailto:notifications@github.com> wrote:

That is, I think UNENCODED_SET should still be automatically considered "immune" in addition to any "immune" characters passed as the first argument.

Reply to this email directly or view it on GitHubhttps://github.com/ESAPI/esapi-java-legacy/issues/359#issuecomment-177352514.

kwwall commented 8 years ago

Haven't looked at all the others, but the ones I did indeed to that. -kevin

On Sat, Jan 30, 2016 at 8:07 PM, Jeff Williams notifications@github.com wrote:

Yes. I concur. Hopefully that's what the other codecs do? Apologies I'm AFK right now

--Jeff

On Sat, Jan 30, 2016 at 5:04 PM -0800, "Kevin W. Wall" < notifications@github.commailto:notifications@github.com> wrote:

That is, I think UNENCODED_SET should still be automatically considered "immune" in addition to any "immune" characters passed as the first argument.

Reply to this email directly or view it on GitHub< https://github.com/ESAPI/esapi-java-legacy/issues/359#issuecomment-177352514

.

— Reply to this email directly or view it on GitHub https://github.com/ESAPI/esapi-java-legacy/issues/359#issuecomment-177352591 .

Blog: http://off-the-wall-security.blogspot.com/ | Twitter: @KevinWWall NSA: All your crypto bit are belong to us.

jeremiahjstacey commented 8 years ago

I'm still in the process of trying to understand how the Codec implementations function with regards to the rest of the API. It seems the immunity list has limited use internally.

That being said, I believe making use of an Exception may be another way to handle this.

If the Codec in question has a set of reserved characters which would fundamentally change the intended behavior of the implementation, then we could create a protected 'validate' method in the abstract Codec class which would check immunity for reserved characters and Exception if they're overlap.

/**
     * Checks that there is no overlap between the client-requested immunity character array and the reserved characters for this codec.
     * @param immunity character array being requested for encoding immunity.
     * @param codecReserved array of characters which are reserved based on the subclass' implementation.
     * @throws EncodingException Thrown if immunity has been requested for a reserved codec character.
     */
    protected final void validateImmunity(char[] immunity, char[] codecReserved) throws EncodingException{
        //There's a more elegant way to handle this.
       Set<Character> uniqueChars = new HashSet<Character>();
       for (char reserved : codecReserved) {
           uniqueChars.add(reserved);
       }
       for (char immune: immunity) {
           //Set.add will return false if the character already exists.
           if (!uniqueChars.add(immune)) {
               //This is a typed exception.  Maybe this should be a RuntimeException extension?
               throw new EncodingException("Encoding Failure", String.format("%s: Cannot grant character immunity for char '%s'. Character is reserved for codec.", this.getClass().getSimpleName(), immune));             
           }
       }

     }

I'm not sure I'm using the correct exception, but this feels like a good way to 'handle' the problem. We do not introduce new code into the codecs to account for escaping client-specified immunity conditions while also preventing clients from inadvertently breaking security features.

planetlevel commented 8 years ago

Feels like a sternly written javadoc to me. I'm not sure I can say with authority that any specific immune character is totally illegal in all circumstances.

--Jeff

On Sat, Jan 30, 2016 at 7:13 PM -0800, "jeremiahjstacey" notifications@github.com<mailto:notifications@github.com> wrote:

I'm still in the process of trying to understand how the Codec implementations function with regards to the rest of the API. It seems the immunity list has limited use internally.

That being said, I believe making use of an Exception may be another way to handle this.

If the Codec in question has a set of reserved characters which would fundamentally change the intended behavior of the implementation, then we could create a protected 'validate' method in the abstract Codec class which would check immunity for reserved characters and Exception if they're overlap.

/* * Checks that there is no overlap between the client-requested immunity character array and the reserved characters for this codec. * @param immunity character array being requested for encoding immunity. * @param codecReserved array of characters which are reserved based on the subclass' implementation. * @throws EncodingException Thrown if immunity has been requested for a reserved codec character. / protected final void validateImmunity(char[] immunity, char[] codecReserved) throws EncodingException{ //There's a more elegant way to handle this. Set uniqueChars = new HashSet(); for (char reserved : codecReserved) { uniqueChars.add(reserved); } for (char immune: immunity) { //Set.add will return false if the character already exists. if (!uniqueChars.add(immune)) { //This is a typed exception. Maybe this should be a RuntimeException extension? throw new EncodingException("Encoding Failure", String.format("%s: Cannot grant character immunity for char '%s'. Character is reserved for codec.", this.getClass().getSimpleName(), immune)); } }

 }

I'm not sure I'm using the specific exception, but this feels like a good way to 'handle' the problem. We do not introduce new code into the codecs to account for escaping ridiculous conditions, and we prevent clients from inadvertently breaking security features.

Reply to this email directly or view it on GitHubhttps://github.com/ESAPI/esapi-java-legacy/issues/359#issuecomment-177370558.

kwwall commented 8 years ago

This is what I'm planning for the PercentCodec.encodeCharacter() javadoc (not that anyone will actually read it):

    /**
     * Encode a character for URLs
     * @param immune Additional characters not to encode. Note this could
     *               break URL encoding as referenced in RFC 3986. You should
     *               especially be wary of including '%' in this list of immune
     *               characters since it is used as the "escape" character for
     *               the hex encoding and including it may result in subsequent
     *               and/or dangerous results when decoding.
     * @param c character to encode
     * @return the encoded string representing c
     */
    public String encodeCharacter( char[] immune, Character c )

Comments welcome.

xeno6696 commented 8 years ago

We should probably create an update issue to cover fixing the javadocs in the other codecs. The HTMLCodec will allow you to mark the "&" character as immune, and as we all know, that essentially nullifies the main purpose of that Codec in the same way we've discussed here.

Thoughts?

planetlevel commented 8 years ago

I wouldn’t go as far as “nullifies” unless I’m missing something here. The REAL danger would be if you made < > " or ' immune. Making & immune would possibly some weirdness, but certainly wouldn’t nullify the purpose of the codec.

--Jeff

From: Matt Seil notifications@github.com<mailto:notifications@github.com> Reply-To: ESAPI/esapi-java-legacy reply@reply.github.com<mailto:reply@reply.github.com> Date: Sunday, January 31, 2016 at 11:22 AM To: ESAPI/esapi-java-legacy esapi-java-legacy@noreply.github.com<mailto:esapi-java-legacy@noreply.github.com> Cc: Jeff Williams jeff.williams@contrastsecurity.com<mailto:jeff.williams@contrastsecurity.com> Subject: Re: [esapi-java-legacy] CodecTest unit tests never test with a populated char array. (#359)

We should probably create an update issue to cover fixing the javadocs in the other codecs. The HTMLCodec will allow you to mark the "&" character as immune, and as we all know, that essentially nullifies the main purpose of that Codec in the same way we've discussed here.

— Reply to this email directly or view it on GitHubhttps://github.com/ESAPI/esapi-java-legacy/issues/359#issuecomment-177541422.

xeno6696 commented 8 years ago

Fair enough, I just wanted to call attention to the fact that some kind of extra testing would be warranted when selecting certain kinds of immune characters.