Issue #31 MySQLCodec Updates #472
Conversation
Implementing my understanding of the desired tests from the issue.
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.
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.
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.
Adding a decode validation to the existing tests to verify that when an ecoded string is decoded that the original value is the result.
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.
Updating the URL listed in the class documentation to direct to OWASP recommended MySQL Escaping.
Adding workflow & behavioral validations for the ANSI handling of decoding a PushBackSequence reference.
Updating ANSI PushbackSequence tests names to be better self-documenting.
Adding tests for Standard MySQLCodec Decode with PushbackSequence
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.
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.
Renaming static method to be inclusive of the creation of both mode escape maps.
Tests for handling of encoding with immunity sets for STANDARD and ANSI modes.
Updating class documentation to clarify the intended support provided by the implementation.
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.
Sample served its purpose and may be revisited later. Opting to maintain existing implementation for issue resolution.
Providing javadoc mapping from the MySQLCodec ANSI mode to the MySQL Server ANSI_QUOTES mode documentation.
|
Hold on merge. Have a test impacted in the EncoderTest. Working on it now. |
|
No worries. Lee know when you are ready.
…-kevin
--
Blog: http://off-the-wall-security.blogspot.com/ | Twitter: @KevinWWall
NSA: All your crypto bit are belong to us.
On Mon, Jan 21, 2019, 19:47 jeremiahjstacey ***@***.*** wrote:
Hold on merge. Have a test impacted in the EncoderTest. Working on it now.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#472 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AB3nm8tVqWLHHPpIsEGO0xCZtWDHvAH5ks5vFl-zgaJpZM4aLqeq>
.
|
|
The test that tripped me up is EncoderTest testMySQLANSIModeQuoteInjection It is expecting that the double quote is escaped. If we're in agreement that the double quotes don't need to be handled in this way then I believe I am going to remove the test. All of the checks for the MySQLCodec have been moved into the test specific to the class. |
From research, on issue ESAPI#31 double quotes within the single quote literals supported in ANSI_QUOTE modes do not require explicit escape handling.
|
Changset attached to PR. All tests passed, should be gtg. |
|
@jeremiahjstacey wrote:
What I meant by "fix" and what you mean by "fix" are 2 different things. What I mean was why not reform the test so that it passes and then add a comment there about how double quotes do not require special handling for ANSI_QUOTE mode (aka, MySQLCodec.Mode.ANSI). You could just do that by changing the assertion, right? E.g., something like this: and then the test passes. I think that's better than just removing the test, because someone is not going to come back and think "uh oh, an SQLi vulnerability is still present and they are trying to sneak it past us because they don't know how to fix it so they just simply removed the test and hoped no one noticed". You could argue that the test is unneeded and probably would be right, but perception is just as important as reality in these cases. People act on their perception of truth rather than on actual truth so it's important IMO not to make it appear as we are trying to do something on the sly here. |
Updating compare to match current expectation and adding comment to clarify that no special handling for double quotes is needed in ANSI_QUOTES mode.
|
You make a very good point, thank you for calling that out.. I've reintroduced the test. The comparison condition was updated to match the current impl's expectation and a comment was added to capture the intent. |
* Release notes to close issue #463 * Changes to replace deprecated method CryptoHelper.arrayCompare() with MessageDigest.isEqual() which is safe in JDK 7. * Close issue #246 * Close issue #462 * Close issue #364 and general assertion cleanup. * Close issue #417 * Close issue #465 * Add more details, especially regarding dependency updates to address CVEs. * Add WARNINGS to javadoc as noted in comment for GitHub issue #233 * Update OWASP Dependency Check from release 2.1.0 to 4.0.1 (i.e., the latest version). * General clean-up. Add paragraph to discuss CVE-2018-8088 and why ESAPI is not affected. * Update to mention PR #467 and closing of issue #360 * Fix typo in path for configuration/esapi and add 2.2.0.0 release notes. * Change release from 2.1.0.2-SNAPSHOT to 2.2.0.0-SNAPSHOT in prep for release. See GitHub issue #471 * Preparation for 2.2.0.0 release. See GitHub issue #471 for details. * Try to clarify by example the git commands used. * Added Jeremiah's PR #472 * * Reference issue 188 as being closed. * Udate status of latest PRs. * Added 'Basic ESAPI Facts' section.
Summary
Work to make the MySQLCodec compliant with the OWASP recommended practices as defined in
https://www.owasp.org/index.php/SQL_Injection_Prevention_Cheat_Sheet#MySQL_Escaping
Functional Change in MySQL Mode:
Functional change in ANSI Mode:
Removed handling for double-quote escapes
Functional change in the deprecated Constructor
Updated mode lookup to throw an exception as soon as the mode cannot be resolved as opposed to allowing the mode to be null and possibly throw NPE's at runtime.
9ad908a
All other commits are increasing basic test coverage.
Issue #31 investigation summary
Issue 31 originally targeted the context-based handling of percent (%) and underscore (_) based on where it appeared in the statement. It was stated that the behavior was corrected on the ESAPI 1.4 branch.
Upon investigating the changes on the 1.4 branch I found that the resolution was to alter the default mode applied to the MySQLCodec from ANSI to STANDARD (In that version mode had a default value). This is confirmed by later conversations in the issue.
In the current structure of the code there is no default value, and the mode configuration is required to establish an instance of the class. I believed that the issue was effectively OBE'd by the change in the constructor logic of the class.
With that in mind I went about adding tests asserting ESAPI MySQLCodec compliance to the OWASP recommended strategies, which is where the changes above originated. There were two logic cases where the modes did not perform to the recommendation, which have been addressed in this effort.
I believe this work resolves Issue #31 by showing that we are following industry recommendations for the necessary cases and that no additional logic or handling is integrated which would cause divergence from that intent.