DDoS hash collision attack as reported by Jesse Wilson #416
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Jesse brings out a good point, and his point might be more applicable to Groovy. In fact, once I stopped arguing with him I realized that Groovy impacted a lot more than Boon, and Groovy is widely used and Boon is not. Also I think this impacted the old Groovy JSON parser as well but I could be wrong. http://publicobject.com/2014/05/11/the-json-benchmarks-are-wrong/ I think Boon might is less vulnerable than Groovy, but I patched it today as well. http://rick-hightower.blogspot.com/2014/05/fud-gson-100x-faster-than-jac kson-and.html If you are using Boon and Pojos (which is the most common combination that I have seen for a public API), then this is not an issue with Boon. Also Boon and Groovy if using JDK 1.7 then there is a special JDK 1.7 flag that prevents this. Groovy unlike Boon before the patch might and likely still have this issue even for Pojos. Boon Pojo support uses the API that knows about the index overlay for Pojos so that is why this was not an issue with Boon. It seems Groovy is more vulnerable because it supports JDK 1.6 (Boon does not) and the Map to Groovy object routines would not know about the index overlay API, and would not know to avoid the map (which I do for performance). You can see an example here on how to access the underlying index overlay method. http://rick-hightower.blogspot.com/2014/05/fud-gson-100x-faster-than-jac kson-and.html Groovy is impacted in pretty much every public API that uses a map (rare) or a POJO so 100% of public APIs unless you filter at payload size with an F5 or scan the JSON stream ahead of time. The same index overlay API exists (shown in blog) so there is a workaround, but since it is with both Map and Pojo, I took time off from everything else to fix this. This is a quick interim patch. It just works. With this patch it should not be possible to ever do a hash key collision attack. This patch checks for Java 1.7 and above and then checks to see jdk.map.althashing.threshold is set if 1.7. If Java is below 1.7, then it uses TreeMap, if Java is 1.7 and threshold not set then this patch uses TreeMap. If Java 1.8, this patch uses LinkedHashMap. I worked on this now because I realized it was much more of an issue with Groovy than Boon (as described above), and Groovy is much more widely used.
|
Looks good. I am wondering if it wouldn't be safer to make the |
|
Done. Tests pass. Pull it. :) On Wed, May 14, 2014 at 1:22 AM, Cédric Champeau
Rick Hightower |
melix
added a commit
that referenced
this pull request
May 14, 2014
DDoS hash collision attack as reported by Jesse Wilson
|
Thank you! |
|
Thank you. On Wed, May 14, 2014 at 2:15 AM, Cédric Champeau
Rick Hightower |
traneHead
pushed a commit
to traneHead/groovy-core
that referenced
this pull request
Jul 29, 2017
…es questionable byte code (tidy comments - closes groovy#416)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Jesse brings out a good point, and his point might be more applicable
to Groovy. In fact, once I stopped arguing with him I realized that
Groovy was impacted a lot more than Boon, and Groovy is widely used and
Boon is not.
Also I think this impacted the old Groovy JSON parser as well but I
could be wrong. (seems to impact Jackson according to Jesse).
The issue is described here:
http://publicobject.com/2014/05/11/the-json-benchmarks-are-wrong/
Boon is less vulnerable than Groovy, but I patched it
today as well as a proof of concept for this.
http://rick-hightower.blogspot.com/2014/05/fud-gson-100x-faster-than-jackson-and.html
This is not an issue for Boon and Pojos, but it is for Groovy and Pojos.
Boon Pojo support uses the API that knows about the index overlay for
Pojos so that is why this was not an issue with Boon (it avoids using a hashmap) for Pojos.
It seems Groovy is more vulnerable because it supports JDK 1.6 (Boon
does not) and the Map to Groovy object routines would not know about
the index overlay API, and would not know to avoid the map (which Boon does
for performance).
You can see an example here on how to access the underlying index
overlay method.
http://rick-hightower.blogspot.com/2014/05/fud-gson-100x-faster-than-jackson-and.html
Like Boon, Groovy is impacted in pretty much every public API that uses a map
(rare).
Unlike Boon (which is why this is more important for Groovy) this also impacts POJO
so 100% of public APIs unless you filter at payload
size with an F5 or scan the JSON stream ahead of time. The same index
overlay API exists (shown in blog) so there is a workaround, but since
with Groovy it is with both Map and Pojo, I took time off from everything else to
fix this.
This is a quick interim patch. It just works.
With this patch it should not be possible to avoid a hash key
collision attack. This patch checks for Java 1.7 and above and then
checks to see jdk.map.althashing.threshold is set if 1.7.
See http://docs.oracle.com/javase/7/docs/technotes/guides/collections/changes7.html
If Java is below 1.7, then this patch uses TreeMap, if Java 1.7 and threshold is not set
then this patch uses TreeMap. If Java 1.8, this patch uses
LinkedHashMap.