Skip to content

Commit 48cb2ad

Browse files
jeremiahjstaceykwwall
authored andcommitted
Issue #31 MySQLCodec Updates (#472)
* MySQLCodec Underscore Escape Tests Implementing my understanding of the desired tests from the issue. * MySQLCodec Logic and Test Modifications I am not sure this is what is desired Tests showing the MySQLCodec acts as specified for the OWASP recommendation of MySQL encoding for both ANSI and MySQL (Standard) Modes. Logic updates to account for divergence from the recommended approach. * MySQLCodec Logic (Restoring) Putting back the logic from MySQLCodec removed last commit. Alphanumerics under 256 should be returned in their original forms based on OWASP recommendation being targeted. * MySQLCodec Test Updates Correcting tests for the STANDARD handling of characters up to 256 including numbers, upper/lower case letters, special encoded characters, and any value outside the previously defined sets. This should now assert the OWASP recommended approach to Encoding STANDARD MySQL inputs. * MySQLCodecTest Updates: DECODE Adding a decode validation to the existing tests to verify that when an ecoded string is decoded that the original value is the result. * MySQLCodec Logic Update: Bug Fix Identified a case in MySQLCodec (STANDARD) where the special-case character 0x1a was correctly encoding to '//Z', but the decode case was watching for '//z'. Encoded value did not decode to original input. Logic update in decode to alter the lower case z to the upper-case Z. * MySQLCodec Documentation Update Updating the URL listed in the class documentation to direct to OWASP recommended MySQL Escaping. * MySQLCodecTest Updates Ansi Decode PB_Seq Adding workflow & behavioral validations for the ANSI handling of decoding a PushBackSequence reference. * MySQLCodecTest Updates Ansi Decode PB_Seq Updating ANSI PushbackSequence tests names to be better self-documenting. * MySQLCodecTest Updates Standard Decode PB_Seq Adding tests for Standard MySQLCodec Decode with PushbackSequence * MySqlCodecTest Ctr using int The ctr is deprecated, but still should be tested as long as it is present. Adding cases for ANSI and Standard resolution showing mode is correctly resolved. Stub of an unsupported int also included, but currently ignored. * MySQLCodec Logic & Tests invalid int CTR Arg Found that implementations would throw NPE's if an invalid int was provided to the deprecated constructor when trying to encode or decode values. Opted to alter the constructor logic to immediately throw a runtime exception (IllegalArgument) if the constructor parameter cannot resolve to a valid reference. It would be a Runtime exception either way, it's just a matter of when it's thrown. Alternatively, the mode switches in the encode/decode could have been surrounded by an if block with an else/fallthrough which retuned null. It is my opinion that this is a non-obvious failure of misconfiguration and would be difficult to diagnose and resolve. The immediate class failure should provide the end-user with context on how to resolve the problem and limit the time debugging. Tests provided to validate workflow. * MySQLCodecTest Update Method Rename Renaming static method to be inclusive of the creation of both mode escape maps. * MySQLCodec Test Immunity Validations Tests for handling of encoding with immunity sets for STANDARD and ANSI modes. * MySQLCodec Documentation Updates Updating class documentation to clarify the intended support provided by the implementation. * MySQLCodec Code Structure Updates Making a copy of MySQLCodec in its own subpackage of the codec group. Breaking apart the implementation into the codec and a strategy pattern controlled by the MySQLMode enumeration. Each piece of this implementation retained the original tests; however, like the code, the tests have been split up as well. Now the responsibilty of the ANSI handing is in the MySQLAnsiSupport class. Standard behavior is held an tested from the MySQLStandardSupport class. I feel like this change increases readability and usability of the baseline. I am committing this at this time to get another set of opinions on this approach. In the interim, the original MySQLCodec and tests have remained untouched in this effort. * Cleanup: Removing MySqlCodec Strategy impl Sample served its purpose and may be revisited later. Opting to maintain existing implementation for issue resolution. * Updating documentation: ANSI_QUOTES Mode Providing javadoc mapping from the MySQLCodec ANSI mode to the MySQL Server ANSI_QUOTES mode documentation. * Removing testMySQLANSIModeQuoteInjection From research, on issue #31 double quotes within the single quote literals supported in ANSI_QUOTE modes do not require explicit escape handling. * Reintroducing testMySQLANSIModeQuoteInjection Updating compare to match current expectation and adding comment to clarify that no special handling for double quotes is needed in ANSI_QUOTES mode.
1 parent f68341f commit 48cb2ad

File tree

3 files changed

+421
-13
lines changed

3 files changed

+421
-13
lines changed

src/main/java/org/owasp/esapi/codecs/MySQLCodec.java

Lines changed: 53 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -18,13 +18,47 @@
1818

1919

2020
/**
21-
* Implementation of the Codec interface for MySQL strings. See http://mirror.yandex.ru/mirrors/ftp.mysql.com/doc/refman/5.0/en/string-syntax.html
22-
* for more information.
21+
* Codec implementation which can be used to escape string literals in MySQL.
22+
* </br>
23+
* Implementation accepts 2 Modes as identified by the OWASP Recommended
24+
* escaping strategies:
25+
* <ul>
26+
* <li><b>ANSI</b> <br>
27+
* Simply encode all ' (single tick) characters with '' (two single ticks)</li>
28+
* <br>
29+
* <li><b>Standard</b>
2330
*
24-
* @author Jeff Williams (jeff.williams .at. aspectsecurity.com) <a
25-
* href="http://www.aspectsecurity.com">Aspect Security</a>
31+
* <pre>
32+
* NUL (0x00) --> \0 [This is a zero, not the letter O]
33+
* BS (0x08) --> \b
34+
* TAB (0x09) --> \t
35+
* LF (0x0a) --> \n
36+
* CR (0x0d) --> \r
37+
* SUB (0x1a) --> \Z
38+
* " (0x22) --> \"
39+
* % (0x25) --> \%
40+
* ' (0x27) --> \'
41+
* \ (0x5c) --> \\
42+
* _ (0x5f) --> \_
43+
* <br>
44+
* all other non-alphanumeric characters with ASCII values less than 256 --> \c
45+
* where 'c' is the original non-alphanumeric character.
46+
* </pre>
47+
*
48+
* </li>
49+
*
50+
* </ul>
51+
*
52+
* @author Jeff Williams (jeff.williams .at. aspectsecurity.com)
53+
* <a href="http://www.aspectsecurity.com">Aspect Security</a>
2654
* @since June 1, 2007
27-
* @see org.owasp.esapi.Encoder
55+
*
56+
* @see <a href=
57+
* "https://dev.mysql.com/doc/refman/8.0/en/string-literals.html">MySQL 8.0
58+
* String Literals</a>
59+
* @see <a href=
60+
* "https://www.owasp.org/index.php/SQL_Injection_Prevention_Cheat_Sheet#MySQL_Escaping">OWASP
61+
* SQL_Injection_Prevention_Cheat_Sheet#MySQL_Escaping</a>
2862
*/
2963
public class MySQLCodec extends AbstractCharacterCodec {
3064
/**
@@ -46,13 +80,20 @@ static Mode findByKey(int key) {
4680
if ( m.key == key )
4781
return m;
4882
}
49-
return null;
83+
String message = String.format("No Mode for %s. Valid references are MySQLStandard: %s or ANSI: %s", key, MYSQL_MODE, ANSI_MODE);
84+
throw new IllegalArgumentException(message);
5085
}
5186
}
5287

5388
/** Target MySQL Server is running in Standard MySQL (Default) mode. */
5489
public static final int MYSQL_MODE = 0;
55-
/** Target MySQL Server is running in {@link "http://dev.mysql.com/doc/refman/5.0/en/ansi-mode.html"} ANSI Mode */
90+
/**
91+
* Target MySQL Server is running in ANSI_QUOTES Mode
92+
*
93+
* @see <a href=
94+
* "https://dev.mysql.com/doc/refman/8.0/en/sql-mode.html#sqlmode_ansi_quotes">
95+
* https://dev.mysql.com/doc/refman/8.0/en/sql-mode.html#sqlmode_ansi_quotes</a>
96+
*/
5697
public static final int ANSI_MODE = 1;
5798

5899
//private int mode = 0;
@@ -100,11 +141,12 @@ public String encodeCharacter( char[] immune, Character c ) {
100141
return ""+ch;
101142
}
102143

144+
103145
switch( mode ) {
104146
case ANSI: return encodeCharacterANSI( c );
105147
case STANDARD: return encodeCharacterMySQL( c );
148+
default: return null;
106149
}
107-
return null;
108150
}
109151

110152
/**
@@ -123,8 +165,7 @@ public String encodeCharacter( char[] immune, Character c ) {
123165
private String encodeCharacterANSI( Character c ) {
124166
if ( c == '\'' )
125167
return "\'\'";
126-
if ( c == '\"' )
127-
return "";
168+
128169
return ""+c;
129170
}
130171

@@ -166,8 +207,8 @@ public Character decodeCharacter( PushbackSequence<Character> input ) {
166207
switch( mode ) {
167208
case ANSI: return decodeCharacterANSI( input );
168209
case STANDARD: return decodeCharacterMySQL( input );
210+
default: return null;
169211
}
170-
return null;
171212
}
172213

173214
/**
@@ -244,7 +285,7 @@ private Character decodeCharacterMySQL( PushbackSequence<Character> input ) {
244285
return Character.valueOf( (char)0x0a );
245286
} else if ( second.charValue() == 'r' ) {
246287
return Character.valueOf( (char)0x0d );
247-
} else if ( second.charValue() == 'z' ) {
288+
} else if ( second.charValue() == 'Z' ) {
248289
return Character.valueOf( (char)0x1a );
249290
} else if ( second.charValue() == '\"' ) {
250291
return Character.valueOf( (char)0x22 );

0 commit comments

Comments
 (0)